Re: [PATCH] usb: gadget: dummy_hcd: prevent fifo_req reuse during giveback

[Jinchao Wang <wangjinchao600@gmail.com>]

On 7/14/2026 5:13 PM, Alan Stern wrote:
> On Tue, Jul 14, 2026 at 02:48:29PM +0800, Jinchao Wang wrote:
>> dummy_hcd embeds a single shared usb_request (dum->fifo_req) that the
>> "emulated single-request FIFO" fast-path in dummy_queue() reuses for
>> small IN transfers: it copies the caller's request into it
>> (req->req = *_req) and queues it, treating list_empty(&fifo_req.queue)
>> as "the slot is free".
>>
>> The completion side (dummy_timer/transfer/nuke/dummy_dequeue) follows
>> the standard pattern: list_del_init(&req->queue) unlinks the request,
>> then the lock is dropped and usb_gadget_giveback_request() invokes
>> req->complete().  But list_del_init() makes fifo_req.queue look empty
>> *before* the completion callback returns, so a concurrent dummy_queue()
>> on another CPU sees the slot as free, reuses fifo_req and runs
>> req->req = *_req -- overwriting req->complete while dummy_timer is
>> mid-calling it.  The indirect call then jumps to a clobbered pointer,
>> causing a general protection fault / page fault in dummy_timer
>> (syzkaller extid faf3a6cf579fc65591ca).  The clobbering write is an
>> in-bounds memcpy on a live shared object, so KASAN cannot flag it.
>>
>> Add a fifo_req_busy bit, set across the lockless giveback window via a
>> dummy_giveback() helper used at all four gadget-request giveback sites,
>> and require !fifo_req_busy in the FIFO fast-path guard so the shared
>> slot cannot be reused until its completion callback has returned.
>>
>> Reported-by: syzbot+faf3a6cf579fc65591ca@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=faf3a6cf579fc65591ca
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
> 
> Wow!  I'm impressed.  How did you figure this out?

With a hardware watchpoint: I armed one on the victim field
(req->complete, at arg2+56 of usb_gadget_giveback_request) only while
usb_gadget_giveback_request() was running, and it caught the writing
memcpy with a full stack - usb_ep_queue <- raw_process_ep_io <-
raw_ioctl - on the same request that crashed an instant later.

The watchpoint setup came from a small tool I am working on; I posted
it as an RFC in case it is useful to others:

https://lore.kernel.org/all/20260714182243.10687-1-wangjinchao600@gmail.com/

> 
>> ---
>>  drivers/usb/gadget/udc/dummy_hcd.c | 40 +++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
>> index f47903461ed5..fce3c3ba7a63 100644
>> --- a/drivers/usb/gadget/udc/dummy_hcd.c
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -278,6 +278,7 @@ struct dummy {
>>  	unsigned			ints_enabled:1;
>>  	unsigned			udc_suspended:1;
>>  	unsigned			pullup:1;
>> +	unsigned			fifo_req_busy:1;
>>  
>>  	/*
>>  	 * HOST side support
>> @@ -330,6 +331,28 @@ static inline struct dummy *gadget_dev_to_dummy(struct device *dev)
>>  /* DEVICE/GADGET SIDE UTILITY ROUTINES */
>>  
>>  /* called with spinlock held */
> 
> That comment line is supposed to come immediately before nuke().  Your 
> new code got inserted below the comment instead of above it.

Right, will fix in v2.

> 
>> +/*
>> + * Give back a gadget request with dum->lock dropped around the callback.
>> + * If @req is the shared fifo_req, mark it busy across the callback so
>> + * dummy_queue()'s FIFO fast-path (keyed on list_empty(&fifo_req.queue))
>> + * cannot reuse it mid-giveback: list_del_init() already made the queue look
>> + * empty, but the request is in flight until the completion callback returns.
>> + * Caller holds dum->lock and has already done list_del_init() + status.
>> + */
>> +static void dummy_giveback(struct dummy *dum, struct usb_ep *_ep,
>> +			   struct dummy_request *req)
>> +{
>> +	bool fifo = req == &dum->fifo_req;
>> +
>> +	if (fifo)
>> +		dum->fifo_req_busy = 1;
> 
> Don't set the new flag here...
> 
>> +	spin_unlock(&dum->lock);
>> +	usb_gadget_giveback_request(_ep, &req->req);
>> +	spin_lock(&dum->lock);
>> +	if (fifo)
>> +		dum->fifo_req_busy = 0;
>> +}
>> +
>>  static void nuke(struct dummy *dum, struct dummy_ep *ep)
>>  {
>>  	while (!list_empty(&ep->queue)) {
> 
>> @@ -729,6 +750,7 @@ static int dummy_queue(struct usb_ep *_ep, struct usb_request *_req,
>>  	/* implement an emulated single-request FIFO */
>>  	if (ep->desc && (ep->desc->bEndpointAddress & USB_DIR_IN) &&
>>  			list_empty(&dum->fifo_req.queue) &&
>> +			!dum->fifo_req_busy &&
>>  			list_empty(&ep->queue) &&
>>  			_req->length <= FIFO_SIZE) {
>>  		req = &dum->fifo_req;
> 
> Set it here instead, so the flag is set during the entire time that 
> dum->fifo_req is in use.  As a bonus, you can then remove the 
> list_empty(&dum->fifo_req.queue) test above.

Indeed better - the flag then covers the whole lifetime of the shared
request instead of just the giveback window. Will do in v2, with the
list_empty() test removed.
 

> Otherwise this seems fine.

Thanks for the review, v2 shortly.

Thanks,
Jinchao

> 
> Alan Stern