[PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

[PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

[=?gb18030?B?Zmx5bm5uY2hlbiizwszss/4p?=]

From c63dc814b5e51713222462e6bf27d7956a933834 Mon Sep 17 00:00:00 2001
From: Tianchu Chen <flynnnchen@tencent.com>
Date: Sun, 16 Nov 2025 12:46:18 +0800
Subject: [PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

Discovered by Atuin - Automated Vulnerability Discovery Engine.

new_pba comes from the status packet returned after each write.
A bogus device could report values beyond the block count derived
from info->capacity, letting the driver walk off the end of
pba_to_lba[] and corrupt heap memory.

Reject PBAs that exceed the computed block count and fail the
transfer so we avoid touching out-of-range mapping entries.

Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
 drivers/usb/storage/sddr55.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index b323f0a36..9d813727e 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -469,6 +469,12 @@ static int sddr55_write_data(struct us_data *us,
 		new_pba = (status[3] + (status[4] << 8) + (status[5] << 16))
 						  >> info->blockshift;
 
+		/* check if device-reported new_pba is out of range */
+		if (new_pba >= (info->capacity >> (info->blockshift + info->pageshift))) {
+			result = USB_STOR_TRANSPORT_FAILED;
+			goto leave;
+		}
+
 		/* check status for error */
 		if (status[0] == 0xff && status[1] == 0x4) {
 			info->pba_to_lba[new_pba] = BAD_BLOCK;
-- 
2.39.5

Re: [PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

[gregkh]

On Sun, Nov 16, 2025 at 01:22:01PM +0800, flynnnchen(陈天楚) wrote:
> From c63dc814b5e51713222462e6bf27d7956a933834 Mon Sep 17 00:00:00 2001
> From: Tianchu Chen <flynnnchen@tencent.com>
> Date: Sun, 16 Nov 2025 12:46:18 +0800
> Subject: [PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

You can use git send-email to not have this show up in the body of the
email in the future :)

> Discovered by Atuin - Automated Vulnerability Discovery Engine.
> 
> new_pba comes from the status packet returned after each write.
> A bogus device could report values beyond the block count derived
> from info->capacity, letting the driver walk off the end of
> pba_to_lba[] and corrupt heap memory.
> 
> Reject PBAs that exceed the computed block count and fail the
> transfer so we avoid touching out-of-range mapping entries.
> 
> Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
> ---
>  drivers/usb/storage/sddr55.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
> index b323f0a36..9d813727e 100644
> --- a/drivers/usb/storage/sddr55.c
> +++ b/drivers/usb/storage/sddr55.c
> @@ -469,6 +469,12 @@ static int sddr55_write_data(struct us_data *us,
>  		new_pba = (status[3] + (status[4] << 8) + (status[5] << 16))
>  						  >> info->blockshift;
>  
> +		/* check if device-reported new_pba is out of range */
> +		if (new_pba >= (info->capacity >> (info->blockshift + info->pageshift))) {
> +			result = USB_STOR_TRANSPORT_FAILED;
> +			goto leave;
> +		}

Any chance you tested this with real hardware?

thanks,

greg k-h

Re: Re: [PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

[=?gb18030?B?Zmx5bm5uY2hlbiizwszss/4p?=]

On Sun, Nov 16, 2025 at 07:07:57AM -0500, gregkh wrote:

>On Sun, Nov 16, 2025 at 01:22:01PM +0800, flynnnchen(陈天楚) wrote:
>> Discovered by Atuin - Automated Vulnerability Discovery Engine.
>> 
>> new_pba comes from the status packet returned after each write.
>> A bogus device could report values beyond the block count derived
>> from info->capacity, letting the driver walk off the end of
>> pba_to_lba[] and corrupt heap memory.
>> 
>> Reject PBAs that exceed the computed block count and fail the
>> transfer so we avoid touching out-of-range mapping entries.
>> 
>> Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
>> ---
>>  drivers/usb/storage/sddr55.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
>> index b323f0a36..9d813727e 100644
>> --- a/drivers/usb/storage/sddr55.c
>> +++ b/drivers/usb/storage/sddr55.c
>> @@ -469,6 +469,12 @@ static int sddr55_write_data(struct us_data *us,
>>  		new_pba = (status[3] + (status[4] << 8) + (status[5] << 16))
>>  						  >> info->blockshift;
>>  
>> +		/* check if device-reported new_pba is out of range */
>> +		if (new_pba >= (info->capacity >> (info->blockshift + info->pageshift))) {
>> +			result = USB_STOR_TRANSPORT_FAILED;
>> +			goto leave;
>> +		}
>
>Any chance you tested this with real hardware?
>
>thanks,
>
>greg k-h


Unfortunately, I do not own the real hardware, and have not tested it on a
a real device.

Some of my thoughts on this patch, from static analysis:

Q: What if this code is called, before info->capacity is set?
A: If sddr55_write_data is called before sddr55_get_capacity,
info->capacity and pba_to_lba should both be 0, in the patched code it will
always fail, while in the original code it will cause NULL-deref.

Q: Is checking new_pba here essential in all cases?
A: Yes, in all the code path beneath it, new_pba is always used to index
the array, with no exception.

Q: What if device reports write success, and gives back an OOB new_pba?
A: For the patched code, it will fail the write transfer. In the original
one, it is very likely to fail as well, because:

		/* check that new_pba wasn't already being used */
		if (info->pba_to_lba[new_pba] != UNUSED_BLOCK) {  <--- Very likely to fail here
			printk(KERN_ERR "sddr55 error: new PBA %04X already in use for LBA %04X\n",
				new_pba, info->pba_to_lba[new_pba]);
			info->fatal_error = 1;
			set_sense_info (3, 0x31, 0);
			result = USB_STOR_TRANSPORT_FAILED;
			goto leave;
		}

The original code OOB-reads info->pba_to_lba[new_pba], which is very
unlikely to equal to UNUSED_BLOCK (i32 0x3ff) by chance, thus the original
code will also fail the transfer in most cases.

From my understanding, the patched code solves the OOB issue, while being
aligned with the original behavior in most cases I have imagined.

Re: Re: [PATCH] usb: storage: sddr55: Reject out-of-bound new_pba

[gregkh]

On Sun, Nov 16, 2025 at 09:15:05PM +0800, flynnnchen(陈天楚) wrote:
> On Sun, Nov 16, 2025 at 07:07:57AM -0500, gregkh wrote:
> 
> >On Sun, Nov 16, 2025 at 01:22:01PM +0800, flynnnchen(陈天楚) wrote:
> >> Discovered by Atuin - Automated Vulnerability Discovery Engine.
> >> 
> >> new_pba comes from the status packet returned after each write.
> >> A bogus device could report values beyond the block count derived
> >> from info->capacity, letting the driver walk off the end of
> >> pba_to_lba[] and corrupt heap memory.
> >> 
> >> Reject PBAs that exceed the computed block count and fail the
> >> transfer so we avoid touching out-of-range mapping entries.
> >> 
> >> Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
> >> ---
> >>  drivers/usb/storage/sddr55.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
> >> index b323f0a36..9d813727e 100644
> >> --- a/drivers/usb/storage/sddr55.c
> >> +++ b/drivers/usb/storage/sddr55.c
> >> @@ -469,6 +469,12 @@ static int sddr55_write_data(struct us_data *us,
> >>  		new_pba = (status[3] + (status[4] << 8) + (status[5] << 16))
> >>  						  >> info->blockshift;
> >>  
> >> +		/* check if device-reported new_pba is out of range */
> >> +		if (new_pba >= (info->capacity >> (info->blockshift + info->pageshift))) {
> >> +			result = USB_STOR_TRANSPORT_FAILED;
> >> +			goto leave;
> >> +		}
> >
> >Any chance you tested this with real hardware?
> >
> >thanks,
> >
> >greg k-h
> 
> 
> Unfortunately, I do not own the real hardware, and have not tested it on a
> a real device.
> 
> Some of my thoughts on this patch, from static analysis:
> 
> Q: What if this code is called, before info->capacity is set?
> A: If sddr55_write_data is called before sddr55_get_capacity,
> info->capacity and pba_to_lba should both be 0, in the patched code it will
> always fail, while in the original code it will cause NULL-deref.

But how can that out-of-order calling happen?

> Q: Is checking new_pba here essential in all cases?
> A: Yes, in all the code path beneath it, new_pba is always used to index
> the array, with no exception.

Fair enough :)

> Q: What if device reports write success, and gives back an OOB new_pba?
> A: For the patched code, it will fail the write transfer. In the original
> one, it is very likely to fail as well, because:
> 
> 		/* check that new_pba wasn't already being used */
> 		if (info->pba_to_lba[new_pba] != UNUSED_BLOCK) {  <--- Very likely to fail here
> 			printk(KERN_ERR "sddr55 error: new PBA %04X already in use for LBA %04X\n",
> 				new_pba, info->pba_to_lba[new_pba]);
> 			info->fatal_error = 1;
> 			set_sense_info (3, 0x31, 0);
> 			result = USB_STOR_TRANSPORT_FAILED;
> 			goto leave;
> 		}
> 
> The original code OOB-reads info->pba_to_lba[new_pba], which is very
> unlikely to equal to UNUSED_BLOCK (i32 0x3ff) by chance, thus the original
> code will also fail the transfer in most cases.
> 
> From my understanding, the patched code solves the OOB issue, while being
> aligned with the original behavior in most cases I have imagined.

Ok, seems sane, I'll be glad to take this, just wanted to be sure.  I
kind of doubt anyone has this hardware anymore anyway :)

thanks,

greg k-h