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

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/07/01 04:17), sashiko-bot@kernel.org wrote:
> 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.

[..]

> > +	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.

Sounds reasonable.

> 
> > +		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()?

Yes, this looks wrong.  Let me take a look.