Re: [PATCH 2/3] usb: uas: fail command submission on fatal USB errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] usb: uas: command hang and cmd_urb leak in
  uas_queuecommand_lck()
- [Medium] usb: uas: orphaned sense URB on error in
  uas_queuecommand_lck()
- [Low] usb: uas: missing Fixes trailer for uas_queuecommand_lck() fix
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] usb: uas: use-after-free in uas_queuecommand_lck() via
  negative qdepth [1]
--

commit d4b0bad1326a981836c51fc29f205193a0a134fb
Author: Sarthak Kukreti <sarthakkukreti@google.com>

> usb: uas: fail command submission on fatal USB errors
> 
> uas_queuecommand_lck() only treats -ENODEV from uas_submit_urbs() as a
> fatal submission error. When a link drops or an endpoint is disabled
> while hub_wq is frozen, usb_submit_urb() can also fail with -ESHUTDOWN,
> -ENOENT, or -EHOSTUNREACH, causing uas_queuecommand_lck() to return
> SCSI_MLQUEUE_DEVICE_BUSY or queue retry work. In addition, if status URB
> submission succeeds before command URB submission fails with no inflight
> URBs, calling scsi_done() directly leaks the unsubmitted cmd_urb.
> 
> Fail commands with DID_NO_CONNECT on -ESHUTDOWN, -ENOENT, and
> -EHOSTUNREACH in uas_queuecommand_lck(), and free unsubmitted URBs
> before calling scsi_done().
> 
> Signed-off-by: Sarthak Kukreti <sarthakkukreti@google.com>

[Severity: Low]
Since this fixes the incomplete fatal submission error handling and URB
leak in uas_queuecommand_lck(), should it include Fixes: tags for
eb2a86ae8c54 ("USB: UAS: fix disconnect by unplugging a hub") and
cd5432c71235 ("USB: UAS: return ENODEV when submit urbs fail with device
not attached")?

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3aadf4c88c5c2..843860687542d 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -743,11 +743,13 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)
>  	 * a command that has finished is a success for the purpose
>  	 * of queueing, no matter how fatal the error
>  	 */
> -	if (err == -ENODEV) {
> +	if (err == -ENODEV || err == -ESHUTDOWN || err == -ENOENT ||
> +	    err == -EHOSTUNREACH) {
>  		if (cmdinfo->state & (COMMAND_INFLIGHT | DATA_IN_URB_INFLIGHT |
>  				DATA_OUT_URB_INFLIGHT))
>  			goto out;

[Severity: Medium]
Can jumping to out here leak cmdinfo->cmd_urb and hang the command when
uas_submit_urbs() fails after submitting a data URB on a streams-enabled
device?

In uas_submit_urbs(), data URBs are submitted before cmd_urb, and
SUBMIT_CMD_URB is only cleared (and COMMAND_INFLIGHT set) once cmd_urb is
submitted:

drivers/usb/storage/uas.c:uas_submit_urbs() {
	...
	if (cmdinfo->state & SUBMIT_CMD_URB) {
		usb_anchor_urb(cmdinfo->cmd_urb, &devinfo->cmd_urbs);
		err = usb_submit_urb(cmdinfo->cmd_urb, GFP_ATOMIC);
		if (err) {
			usb_unanchor_urb(cmdinfo->cmd_urb);
			uas_log_cmd_state(cmnd, "cmd submit err", err);
			return err;
		}
		cmdinfo->cmd_urb = NULL;
		cmdinfo->state &= ~SUBMIT_CMD_URB;
		cmdinfo->state |= COMMAND_INFLIGHT;
	}
	return 0;
}

When cmd_urb submission fails after DATA_IN_URB_INFLIGHT or
DATA_OUT_URB_INFLIGHT is set, jumping to out skips
uas_free_unsubmitted_urbs(cmnd) and leaves SUBMIT_CMD_URB set and
COMMAND_INFLIGHT clear.  When the in-flight data URB later completes while
devinfo->resetting is 0, uas_data_cmplt() sees SUBMIT_CMD_URB set and
returns early without calling uas_try_complete(), while uas_stat_cmplt()
skips commands without COMMAND_INFLIGHT.  The command remains stuck in
devinfo->cmnd[idx] until the SCSI error handler times out, and
uas_wait_for_pending_cmnds() in uas_suspend() times out after 5 seconds
with -ETIME.

>  
> +		uas_free_unsubmitted_urbs(cmnd);
>  		set_host_byte(cmnd, DID_NO_CONNECT);
>  		scsi_done(cmnd);
>  		goto zombie;

[Severity: Medium]
Does completing cmnd here leave an orphaned sense URB anchored on
devinfo->sense_urbs when uas_submit_sense_urb() succeeded before a later
URB submission failed?

uas_submit_urbs() submits the sense URB first and anchors it to
devinfo->sense_urbs.  If cmd_urb (or the first data URB) then fails with
no data URB in flight, uas_free_unsubmitted_urbs() frees only cmd_urb and
the data URBs, and scsi_done() completes cmnd without unlinking the sense
URB.  If status_pipe has not been flushed while hub_wq is frozen,
usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000) in
uas_wait_for_pending_cmnds() will time out with -ETIME during suspend or
reset, and the tag can be reused while the old sense URB is still queued.

[Severity: Critical]
This problem wasn't introduced by this patch, but can a negative
devinfo->qdepth [1] cause a use-after-free in uas_queuecommand_lck()?

If usb_alloc_streams() fails during reset or resume,
uas_configure_endpoints() stores the negative error code directly in
devinfo->qdepth:

drivers/usb/storage/uas.c:uas_configure_endpoints() {
	...
	} else {
		devinfo->qdepth = usb_alloc_streams(devinfo->intf, eps + 1,
						    3, MAX_CMNDS, GFP_NOIO);
		if (devinfo->qdepth < 0)
			return devinfo->qdepth;
	...
}

When subsequent commands enter uas_queuecommand_lck(), the loop condition
idx < devinfo->qdepth is immediately false with idx == 0, and the
queue-full check idx == devinfo->qdepth is bypassed:

drivers/usb/storage/uas.c:uas_queuecommand_lck() {
	...
	for (idx = 0; idx < devinfo->qdepth; idx++) {
		if (!devinfo->cmnd[idx])
			break;
	}
	if (idx == devinfo->qdepth) {
		spin_unlock_irqrestore(&devinfo->lock, flags);
		return SCSI_MLQUEUE_DEVICE_BUSY;
	}
	...
out:
	devinfo->cmnd[idx] = cmnd;
	...
}

Every new command then overwrites devinfo->cmnd[0] and reuses tag 1,
abandoning any in-flight command at slot 0 and causing a use-after-free
when its URBs complete; uas_sdev_configure() also passes devinfo->qdepth - 2
to scsi_change_queue_depth().

[1] https://sashiko.dev/#/bug/linux-30cdc430-800b-42d3-908f-c7a0b944e7f9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113003536.3622755-1-sarthakkukreti@google.com?part=2