Re: WARNING in usb_composite_setup_continue

[Alan Stern <stern@rowland.harvard.edu>]

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