diff mbox series

usb: storage: sddr55: avoid integer overflow

Message ID 20230203201821.483477-1-k.yankevich@omp.ru
State New
Headers
Series usb: storage: sddr55: avoid integer overflow |

Commit Message

Karina Yankevich Feb. 3, 2023, 8:18 p.m. UTC 
  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(-)

Comments

Alan Stern Feb. 3, 2023, 8:48 p.m. UTC | #1 
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
>
Sergei Shtylyov Feb. 6, 2023, 8:04 p.m. UTC | #2 
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
Alan Stern Feb. 6, 2023, 8:17 p.m. UTC | #3 
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 mbox series

Patch

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);