For code: trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) | TRB_LEN(length));
TRB_BURST_LEN(priv_ep->trb_burst_size) may be overflow for int 32 if priv_ep->trb_burst_size is equal or larger than 0x80;
Below is the Coverity warning: sign_extension: Suspicious implicit sign extension: priv_ep->trb_burst_size with type u8 (8 bits, unsigned) is promoted in priv_ep->trb_burst_size << 24 to type int (32 bits, signed), then sign-extended to type unsigned long (64 bits, unsigned). If priv_ep->trb_burst_size << 24 is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
Cc: stable@vger.kernel.org #v5.8+ Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver") Signed-off-by: Peter Chen peter.chen@nxp.com --- drivers/usb/cdns3/gadget.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h index 1ccecd237530..020936cb9897 100644 --- a/drivers/usb/cdns3/gadget.h +++ b/drivers/usb/cdns3/gadget.h @@ -1072,7 +1072,7 @@ struct cdns3_trb { #define TRB_TDL_SS_SIZE_GET(p) (((p) & GENMASK(23, 17)) >> 17)
/* transfer_len bitmasks - bits 31:24 */ -#define TRB_BURST_LEN(p) (((p) << 24) & GENMASK(31, 24)) +#define TRB_BURST_LEN(p) (unsigned int)(((p) << 24) & GENMASK(31, 24)) #define TRB_BURST_LEN_GET(p) (((p) & GENMASK(31, 24)) >> 24)
/* Data buffer pointer bitmasks*/
Hi,
Peter Chen peter.chen@nxp.com writes:
For code: trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) | TRB_LEN(length));
TRB_BURST_LEN(priv_ep->trb_burst_size) may be overflow for int 32 if priv_ep->trb_burst_size is equal or larger than 0x80;
Below is the Coverity warning: sign_extension: Suspicious implicit sign extension: priv_ep->trb_burst_size with type u8 (8 bits, unsigned) is promoted in priv_ep->trb_burst_size << 24 to type int (32 bits, signed), then sign-extended to type unsigned long (64 bits, unsigned). If priv_ep->trb_burst_size << 24 is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
looks like a false positive...
Cc: stable@vger.kernel.org #v5.8+ Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver") Signed-off-by: Peter Chen peter.chen@nxp.com
drivers/usb/cdns3/gadget.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h index 1ccecd237530..020936cb9897 100644 --- a/drivers/usb/cdns3/gadget.h +++ b/drivers/usb/cdns3/gadget.h @@ -1072,7 +1072,7 @@ struct cdns3_trb { #define TRB_TDL_SS_SIZE_GET(p) (((p) & GENMASK(23, 17)) >> 17) /* transfer_len bitmasks - bits 31:24 */ -#define TRB_BURST_LEN(p) (((p) << 24) & GENMASK(31, 24)) +#define TRB_BURST_LEN(p) (unsigned int)(((p) << 24) & GENMASK(31, 24))
... because TRB_BURST_LEN() is used to intialize a __le32 type. Even if it ends up being sign extended, the top 32-bits will be ignored.
I'll apply, but it looks like a pointless fix. We shouldn't need it for stable
On 20-10-27 11:03:29, Felipe Balbi wrote:
Hi,
Peter Chen peter.chen@nxp.com writes:
For code: trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) | TRB_LEN(length));
TRB_BURST_LEN(priv_ep->trb_burst_size) may be overflow for int 32 if priv_ep->trb_burst_size is equal or larger than 0x80;
Below is the Coverity warning: sign_extension: Suspicious implicit sign extension: priv_ep->trb_burst_size with type u8 (8 bits, unsigned) is promoted in priv_ep->trb_burst_size << 24 to type int (32 bits, signed), then sign-extended to type unsigned long (64 bits, unsigned). If priv_ep->trb_burst_size << 24 is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
looks like a false positive...
Cc: stable@vger.kernel.org #v5.8+ Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver") Signed-off-by: Peter Chen peter.chen@nxp.com
drivers/usb/cdns3/gadget.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h index 1ccecd237530..020936cb9897 100644 --- a/drivers/usb/cdns3/gadget.h +++ b/drivers/usb/cdns3/gadget.h @@ -1072,7 +1072,7 @@ struct cdns3_trb { #define TRB_TDL_SS_SIZE_GET(p) (((p) & GENMASK(23, 17)) >> 17) /* transfer_len bitmasks - bits 31:24 */ -#define TRB_BURST_LEN(p) (((p) << 24) & GENMASK(31, 24)) +#define TRB_BURST_LEN(p) (unsigned int)(((p) << 24) & GENMASK(31, 24))
... because TRB_BURST_LEN() is used to intialize a __le32 type. Even if it ends up being sign extended, the top 32-bits will be ignored.
I'll apply, but it looks like a pointless fix. We shouldn't need it for stable
At my v2:
It is: #define TRB_BURST_LEN(p) ((unsigned int)((p) << 24) & GENMASK(31, 24))
It is not related to high 32-bits issue, it is sign extended issue for 32 bits. If p is a unsigned char data, the compiler will consider (p) << 24 is int, but not unsigned int. So, if the p is larger than 0x80, the (p) << 24 will be overflow.
If you compile the code:
unsigned int k = 0x80 << 24 + 0x81;
It will report build warning: warning: left shift count >= width of type [-Wshift-count-overflow]
From: Peter Chen
Sent: 27 October 2020 09:49
On 20-10-27 11:03:29, Felipe Balbi wrote:
Hi,
Peter Chen peter.chen@nxp.com writes:
For code: trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) | TRB_LEN(length));
TRB_BURST_LEN(priv_ep->trb_burst_size) may be overflow for int 32 if priv_ep->trb_burst_size is equal or larger than 0x80;
Below is the Coverity warning: sign_extension: Suspicious implicit sign extension: priv_ep->trb_burst_size with type u8 (8 bits, unsigned) is promoted in priv_ep->trb_burst_size << 24 to type int (32 bits, signed), then sign-extended to type unsigned long (64 bits, unsigned). If priv_ep->trb_burst_size << 24 is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
looks like a false positive...
Cc: stable@vger.kernel.org #v5.8+ Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver") Signed-off-by: Peter Chen peter.chen@nxp.com
drivers/usb/cdns3/gadget.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h index 1ccecd237530..020936cb9897 100644 --- a/drivers/usb/cdns3/gadget.h +++ b/drivers/usb/cdns3/gadget.h @@ -1072,7 +1072,7 @@ struct cdns3_trb { #define TRB_TDL_SS_SIZE_GET(p) (((p) & GENMASK(23, 17)) >> 17)
/* transfer_len bitmasks - bits 31:24 */ -#define TRB_BURST_LEN(p) (((p) << 24) & GENMASK(31, 24)) +#define TRB_BURST_LEN(p) (unsigned int)(((p) << 24) & GENMASK(31, 24))
... because TRB_BURST_LEN() is used to intialize a __le32 type. Even if it ends up being sign extended, the top 32-bits will be ignored.
I'll apply, but it looks like a pointless fix. We shouldn't need it for stable
At my v2:
It is: #define TRB_BURST_LEN(p) ((unsigned int)((p) << 24) & GENMASK(31, 24))
It is not related to high 32-bits issue, it is sign extended issue for 32 bits. If p is a unsigned char data, the compiler will consider (p) << 24 is int, but not unsigned int. So, if the p is larger than 0x80, the (p) << 24 will be overflow.
If you compile the code:
unsigned int k = 0x80 << 24 + 0x81;
It will report build warning: warning: left shift count >= width of type [-Wshift-count-overflow]
Something like: #define TRB_BURST_LEN(p) (((p) + 0u) << 24) will remove the warning.
The GENMASK() is pretty pointless - the compiler may fail to optimise it away. If p is 64bit the high bits should get discarded pretty quickly.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Oct 27, 2020 at 09:48:54AM +0000, Peter Chen wrote:
If you compile the code:
unsigned int k = 0x80 << 24 + 0x81;
It will report build warning: warning: left shift count >= width of type [-Wshift-count-overflow]
That's a separate issue. I believe (but haven't checked) that the << operator has lower precedence than +, so the compiler interprets the expression as:
unsigned int k = 0x80 << (24 + 0x81);
and it's pretty obvious why this causes an error. Instead, try compiling:
unsigned int k = (0x80 << 24) + 0x81;
You may get an error message about signed-integer overflow, but not about shift-count overflow.
Alan Stern
That's a separate issue. I believe (but haven't checked) that the << operator has lower precedence than +, so the compiler interprets the expression as:
unsigned int k = 0x80 << (24 + 0x81);
and it's pretty obvious why this causes an error. Instead, try compiling:
unsigned int k = (0x80 << 24) + 0x81;
You may get an error message about signed-integer overflow, but not about shift-count overflow.
Hi Alan,
Your analysis is correct, I did not check the warning message correctly.
Peter
Hi,
Peter Chen peter.chen@nxp.com writes:
For code: trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) | TRB_LEN(length));
TRB_BURST_LEN(priv_ep->trb_burst_size) may be overflow for int 32 if priv_ep->trb_burst_size is equal or larger than 0x80;
Below is the Coverity warning: sign_extension: Suspicious implicit sign extension: priv_ep->trb_burst_size with type u8 (8 bits, unsigned) is promoted in priv_ep->trb_burst_size << 24 to type int (32 bits, signed), then sign-extended to type unsigned long (64 bits, unsigned). If priv_ep->trb_burst_size << 24 is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
looks like a false positive...
Cc: stable@vger.kernel.org #v5.8+ Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver") Signed-off-by: Peter Chen peter.chen@nxp.com
drivers/usb/cdns3/gadget.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h index 1ccecd237530..020936cb9897 100644 --- a/drivers/usb/cdns3/gadget.h +++ b/drivers/usb/cdns3/gadget.h @@ -1072,7 +1072,7 @@ struct cdns3_trb { #define TRB_TDL_SS_SIZE_GET(p) (((p) & GENMASK(23, 17)) >> 17) /* transfer_len bitmasks - bits 31:24 */ -#define TRB_BURST_LEN(p) (((p) << 24) & GENMASK(31, 24)) +#define TRB_BURST_LEN(p) (unsigned int)(((p) << 24) & GENMASK(31, 24))
... because TRB_BURST_LEN() is used to intialize a __le32 type. Even if it ends up being sign extended, the top 32-bits will be ignored.
I'll apply, but it looks like a pointless fix. We shouldn't need it for stable.
linux-stable-mirror@lists.linaro.org
-
Alan Stern -
David Laight -
Felipe Balbi -
Peter Chen