Re: [PATCH v3 1/2] usb: uas: implement .change_queue_depth to allow per-device queue depth override

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: uas: unsigned underflow in block queue depth via negative qdepth
- [High] usb: uas: negative qdepth corrupts block layer queue depth via sysfs
- [Medium] locking: data race on devinfo->qdepth between sysfs uas_change_queue_depth and uas_configure_endpoints
--

commit 4c7e73054e14e754e6e1a2516899bccc20bf7379
Author: Luca Cecchi <luca.cecchi.info@gmail.com>

    usb: uas: implement .change_queue_depth to allow per-device queue depth override

This commit implements the .change_queue_depth callback for the UAS driver.
This allows users to dynamically lower the per-device queue depth via sysfs
to work around bridge firmware lockups without having to disable UAS
entirely.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..0a9ae15be2bd6 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c

[ ... ]

> @@ -910,6 +910,20 @@ static int uas_sdev_configure(struct scsi_device *sdev,
>  	return 0;
>  }
>  
> +/*
> + * Implementing .change_queue_depth allows users to lower the per-device
> + * queue depth via sysfs.
> + */
> +static int uas_change_queue_depth(struct scsi_device *sdev, int depth)
> +{
> +	struct uas_dev_info *devinfo = sdev->hostdata;
> +	int max_depth = devinfo->qdepth - 2;

[Severity: Medium]
Could this read of devinfo->qdepth race with a concurrent USB bus reset?

If a user writes to the queue_depth sysfs attribute while a reset
(like uas_post_reset() or uas_reset_resume()) is occurring,
uas_configure_endpoints() can write to devinfo->qdepth before any locks
are acquired by the reset caller:

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

Because uas_change_queue_depth() reads devinfo->qdepth without holding
shost->host_lock or devinfo->lock, can this lack of mutual exclusion lead
to an inconsistent observation of the queue depth?

[Severity: High]
Does this code expose the block layer to queue depth underflows?

If uas_configure_endpoints() fails during a USB reset, it can return a
negative error code (such as -ENOMEM or -22) which is assigned directly to
devinfo->qdepth.

If this happens, sdev->host->can_queue still retains its positive probe-time
value, so sysfs writes to queue_depth still pass validation in
sdev_store_queue_depth().

When this happens, max_depth is calculated as a negative value (e.g. -24).

> +
> +	if (depth > max_depth)
> +		depth = max_depth;
> +	return scsi_change_queue_depth(sdev, depth);

The requested depth is then capped to this negative max_depth and passed to
scsi_change_queue_depth() in drivers/scsi/scsi.c.

scsi_change_queue_depth() forwards this negative value to
blk_set_queue_depth(..., unsigned int depth) in block/blk-settings.c, where
it will implicitly cast to a massive unsigned integer.

Can this heavily corrupt the block layer's queue limits and lead to tag
exhaustion or memory corruption? Even if the underlying negative qdepth is
a separate issue, does wiring up change_queue_depth expose this previously
unreachable vulnerability to sysfs?

> +}
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009081225.304319-1-luca.cecchi.info@gmail.com?part=1