Re: WARNING in usb_composite_setup_continue

Alan Stern
2020-11-10T15:56:53Z

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.

Alan Stern