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

[gregkh <gregkh@linuxfoundation.org>]

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