| Message ID | 20230203201821.483477-1-k.yankevich@omp.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: storage: sddr55: avoid integer overflow | expand |
On Fri, Feb 03, 2023 at 11:18:21PM +0300, Karina Yankevich wrote: > We're possibly losing information by shifting an int. > Fix it by adding the necessary cast. Nonsense. The card's _total_ capacity is no larger than 128 MB, so a page address can't possibly overflow an int. Alan Stern > Found by OMP on behalf of Linux Verification Center > (linuxtesting.org) with SVACE. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Karina Yankevich <k.yankevich@omp.ru> > --- > drivers/usb/storage/sddr55.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c > index 15dc25801cdc..4aeff73de147 100644 > --- a/drivers/usb/storage/sddr55.c > +++ b/drivers/usb/storage/sddr55.c > @@ -236,7 +236,7 @@ static int sddr55_read_data(struct us_data *us, > memset (buffer, 0, len); > } else { > > - address = (pba << info->blockshift) + page; > + address = ((unsigned long)pba << info->blockshift) + page; > > command[0] = 0; > command[1] = LSB_of(address>>16); > @@ -411,7 +411,7 @@ static int sddr55_write_data(struct us_data *us, > command[4] = 0x40; > } > > - address = (pba << info->blockshift) + page; > + address = ((unsigned long)pba << info->blockshift) + page; > > command[1] = LSB_of(address>>16); > command[2] = LSB_of(address>>8); > -- > 2.39.1 >
Hello! On 2/3/23 11:48 PM, Alan Stern wrote: [...] >> We're possibly losing information by shifting an int. >> Fix it by adding the necessary cast. > > Nonsense. The card's _total_ capacity is no larger than 128 MB, so a > page address can't possibly overflow an int. Then the 'address' variables shouldn't be declared *unsigned long*, right? That should fix the SVACE's report as well. Would you accept such a patch? > Alan Stern [...] MBR, Sergey
On Mon, Feb 06, 2023 at 11:04:54PM +0300, Sergei Shtylyov wrote: > Hello! > > On 2/3/23 11:48 PM, Alan Stern wrote: > [...] > >> We're possibly losing information by shifting an int. > >> Fix it by adding the necessary cast. > > > > Nonsense. The card's _total_ capacity is no larger than 128 MB, so a > > page address can't possibly overflow an int. > > Then the 'address' variables shouldn't be declared *unsigned long*, right? > That should fix the SVACE's report as well. Would you accept such a patch? Yes. Alan Stern
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c index 15dc25801cdc..4aeff73de147 100644 --- a/drivers/usb/storage/sddr55.c +++ b/drivers/usb/storage/sddr55.c @@ -236,7 +236,7 @@ static int sddr55_read_data(struct us_data *us, memset (buffer, 0, len); } else { - address = (pba << info->blockshift) + page; + address = ((unsigned long)pba << info->blockshift) + page; command[0] = 0; command[1] = LSB_of(address>>16); @@ -411,7 +411,7 @@ static int sddr55_write_data(struct us_data *us, command[4] = 0x40; } - address = (pba << info->blockshift) + page; + address = ((unsigned long)pba << info->blockshift) + page; command[1] = LSB_of(address>>16); command[2] = LSB_of(address>>8);
We're possibly losing information by shifting an int. Fix it by adding the necessary cast. Found by OMP on behalf of Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Karina Yankevich <k.yankevich@omp.ru> --- drivers/usb/storage/sddr55.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)