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

[=?gb18030?B?Zmx5bm5uY2hlbiizwszss/4p?= <flynnnchen@tencent.com>]

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.