Re: [PATCH v2] usb: uas: validate status URB length before parsing the IU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb/uas: short STATUS IUs (4-15 bytes) falsely complete as SUCCESS
--

commit 1a208f3daab26d0bf253bfe97e68d1636d90a96c
Author: Olivier Kaloudoff <olivier.kaloudoff@gmail.com>

usb: uas: validate status URB length before parsing the IU

The commit attempts to fix log spam caused by parsing a short or empty status
URB transfer. It does so by bailing out if actual_length is smaller than
sizeof(struct iu), which is only 4 bytes long.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..b5d3071d86c84 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -315,6 +315,13 @@ static void uas_stat_cmplt(struct urb *urb)
>  		goto bail;
>  	}
>  
> +	if (urb->actual_length < sizeof(struct iu)) {

[Severity: High]
Does this allow 4-15 byte packets to proceed to type-specific processing?

Because sizeof(struct iu) is only 4 bytes, a packet of 4-15 bytes with
iu_id == IU_ID_STATUS (3) passes this check.

If that occurs, uas_stat_cmplt() delegates to uas_sense():

drivers/usb/storage/uas.c:uas_sense() {
...
	if (urb->actual_length > 16) {
...
	}

	cmnd->result = sense_iu->status;
}

Since the transfer buffer is zero-allocated and not completely overwritten by
the short transfer, sense_iu->status at offset 6 reads as 0 (SAM_STAT_GOOD).

Does this result in the command falsely completing as SUCCESS and hiding
device errors from the SCSI block layer?

> +		dev_err_ratelimited(&urb->dev->dev,
> +			"stat urb: short transfer (%u bytes)\n",
> +			urb->actual_length);
> +		goto bail;
> +	}
> +
>  	idx = be16_to_cpup(&iu->tag) - 1;
>  
>  	spin_lock_irqsave(&devinfo->lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014105526.16392-1-olivier.kaloudoff@gmail.com?part=1