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)