Re: [RFC PATCH] usb: storage: uas: limit consecutive device resets in error handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Prematurely clearing `devinfo->resetting` and dropping the lock before calling `uas_zap_pending()` creates a race window with `uas_do_work()`, leading to a kernel warning and Use-After-Free.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race when reading the `uas_reset_limit` module parameter.
--

commit 6406fc4d6f651e14233de5d86645319b7366244d
Author: Sergey Senozhatsky <senozhatsky@chromium.org>

usb: storage: uas: limit consecutive device resets in error handling

When a UAS storage device experiences persistent wire or hardware IO failures,
it can cause an infinite loop in the SCSI error handler. This commit
introduces a runtime-configurable module parameter 'reset_limit' to track
consecutive resets and abort the loop by returning FAILED, allowing SCSI EH
to offline the unresponsive device.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 2651629812694..a63c66c8bbad2 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c

[ ... ]

> @@ -796,6 +803,21 @@ static int uas_eh_host_reset_handler(struct scsi_cmnd *cmnd)
>  	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
>  	usb_kill_anchored_urbs(&devinfo->sense_urbs);
>  	usb_kill_anchored_urbs(&devinfo->data_urbs);
> +
> +	spin_lock_irqsave(&devinfo->lock, flags);
> +	if (uas_reset_limit > 0 && devinfo->reset_cnt >= uas_reset_limit) {

[Severity: Medium]
Can a concurrent sysfs write cause a Time-of-Check to Time-of-Use race here?

The module parameter uas_reset_limit has 0644 permissions. It is read twice
in this condition without a READ_ONCE() wrapper. If it is modified from a
positive value to zero or negative strictly between the two evaluations, the
first check (uas_reset_limit > 0) could pass, while the second check could
evaluate against the new zero or negative value, prematurely failing the reset
and offlining the device.

> +		devinfo->resetting = 0;
> +		spin_unlock_irqrestore(&devinfo->lock, flags);
> +		uas_zap_pending(devinfo, DID_NO_CONNECT);

[Severity: High]
Does clearing devinfo->resetting and dropping the lock before calling
uas_zap_pending() introduce a race condition with uas_do_work()?

If uas_do_work() is scheduled while the lock is dropped, it will see
devinfo->resetting is 0:

drivers/usb/storage/uas.c:uas_do_work()
	if (devinfo->resetting)
		goto out;

This allows it to bypass the safety check and submit new URBs for pending
commands. Then, when uas_zap_pending() runs, it will fail to complete these
commands because they have newly inflight URBs:

drivers/usb/storage/uas.c:uas_zap_pending()
	err = uas_try_complete(cmnd, __func__);
	WARN_ON(err != 0);

uas_try_complete() will return -EBUSY. The error handler then returns FAILED,
which prompts the SCSI EH to offline the device and forcibly complete and free
the scsi_cmnd structures. When the newly submitted URBs later complete, their
callbacks might access the freed scsi_cmnd, causing a use-after-free.

> +		usb_unlock_device(udev);
> +		shost_printk(KERN_ERR, sdev->host,
> +			     "%s FAILED reset limit %d exceeded\n",
> +			     __func__, uas_reset_limit);
> +		return FAILED;
> +	}
> +	devinfo->reset_cnt++;
> +	spin_unlock_irqrestore(&devinfo->lock, flags);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701040335.810297-1-senozhatsky@chromium.org?part=1