Re: WARNING in usb_composite_setup_continue

Alan Stern
2020-11-11T19:47:12Z

On Tue, Nov 10, 2020 at 10:56:50AM -0500, 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.

Okay, so as Peter pointed out, these comments were wrong. The cdev lock
is held and the scenario described above can't occur, because the lock
isn't released until after both functions have asked for delayed status.

This means that Kyungtae's analysis of the FuzzUSB report is wrong, and
we still don't know what really happened. Here's my guess...

There's still a possible race, although it's a different one: The
gadget's delayed status reply can race with the host timing out and
sending a new SETUP packet:

Host sends SETUP packet A

Function receives A and decides
to send a delayed status reply

Function thread starts to
process packet A

Host times out waiting for A status

Host sends new SETUP packet B

Composite core receives packet B
and resets cdev->delayed_status

Function thread finishes and calls
usb_composite_setup_continue()

The composite core sends a status
reply for packet A, not packet B

Host receives status for A but thinks
it is the status for B!

Function thread processes packet B

Function thread finishes and calls
usb_composite_setup_continue()

The composite core sees
cdev->delayed_status == 0 and WARNs.

At the moment I don't see how to prevent this sort of race from
happening. We may need to change the API, giving the composite core a
way to match up calls to usb_composite_setup_continue() with the
corresponding call to composite_setup(). But even that wouldn't fix
the entire problem.

Suggestions?

Alan Stern