Re: WARNING in usb_composite_setup_continue

Peter Chen
2020-11-11T07:59:40Z

On 20-11-10 10:56:50, Alan Stern wrote:
> Felipe:
>
> On Mon, Nov 09, 2020 at 02:29:42PM -0500, Kyungtae Kim wrote:
> > We report a bug (in linux-5.8.13) found by FuzzUSB (a modified version
> > of syzkaller).
> >
> > (corrected analysis)
> > This bug happens while continuing a delayed setup message in mass
> > storage gadget.
> > To be specific, composite_setup() sets FSG_STATE_CONFIG_CHANGE via
> > fsg_set_alt() (line 1793),
> > and followed by cdev->delayed_status++ (line 1798).
> > Meanwile, the mass gadget tries check cdev->delayed_status == 0
> > through handle_exception() (line 2428),
> > which occurs in between the two operations above.
> > Such a race causes invalid operations eventually.
>
> Do you know who maintains composite.c (or the composite framework) these
> days? This is a real race, and it needs to be fixed.
>
> Part of the problem seems to be that cdev->delayed_status is sometimes
> accessed without the protection of cdev->lock. But I don't know when it
> is safe to take that lock, so I can't tell what changes to make.
>
> Another part of the problem is that cdev->delayed_status doesn't count
> things properly. That is, its value is incremented each time a function
> driver asks for a delayed status and decremented each time a function
> driver calls usb_composite_setup_continue(), and the delayed status
> response is sent when the value reaches 0. But there's nothing to stop
> this from happening (imagine a gadget with two functions A and B):
>
> Function driver A asks for delayed status;
> Function driver A calls setup_continue(): Now the value
> of the counter is 0 so a status message is queued
> too early;
> Function driver B asks for delayed status;
> Function driver B calls setup_continue(): Now a second
> status message is queued.
>
> I'm willing to help fix these issues, but I need assistance from someone
> who fully understands the composite framework.
>

Hi Alan & Kyungtae,

I quite not understand why this occurs, since cdev->delayed_status's
increment and decrement are both protected by cdev->lock.

cdev->delayed_status's increment:

Place 1:
case USB_REQ_GET_CONFIGURATION:
spin_lock(&cdev->lock);
set_config(cdev, ctrl, w_value);
f->set_alt;
cdev->delayed_status++;

spin_unlock(&cdev->lock);

Place 2:
case USB_REQ_SET_INTERFACE:
spin_lock(&cdev->lock);
value = f->set_alt(f, w_index, w_value);
if (value == USB_GADGET_DELAYED_STATUS) {
DBG(cdev,
"%s: interface %d (%s) requested delayed status\n",
__func__, intf, f->name);
cdev->delayed_status++;
DBG(cdev, "delayed_status count %d\n",
cdev->delayed_status);
}
spin_unlock(&cdev->lock);

cdev->delayed_status's decrement:
function: usb_composite_setup_continue which called by fsg_main_thread
due to FSG_STATE_CONFIG_CHANGE.

spin_lock_irqsave(&cdev->lock, flags);

if (cdev->delayed_status == 0) {
WARN(cdev, "%s: Unexpected call\n", __func__);

} else if (--cdev->delayed_status == 0) {
...

spin_unlock_irqrestore(&cdev->lock, flags);
--

Thanks,
Peter Chen