| Message ID | 20201016101659.29482-2-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: cdns3: three bug fixes for v5.10 | expand |
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
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
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*/
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(-)