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

[Jinchao Wang]

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+faf3a6...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=faf3a6cf579fc65591ca
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: sta...@vger.kernel.org
Signed-off-by: Jinchao Wang <wangjin...@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 */
+/*
+ * 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;
+ 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)) {
@@ -339,9 +362,7 @@ static void nuke(struct dummy *dum, struct dummy_ep *ep)
list_del_init(&req->queue);
req->req.status = -ESHUTDOWN;

- spin_unlock(&dum->lock);
- usb_gadget_giveback_request(&ep->ep, &req->req);
- spin_lock(&dum->lock);
+ dummy_giveback(dum, &ep->ep, req);
}
}

@@ -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;
@@ -785,9 +807,7 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
dev_dbg(udc_dev(dum),
"dequeued req %p from %s, len %d buf %p\n",
req, _ep->name, _req->length, _req->buf);
- spin_unlock(&dum->lock);
- usb_gadget_giveback_request(_ep, _req);
- spin_lock(&dum->lock);
+ dummy_giveback(dum, _ep, req);
}
spin_unlock_irqrestore(&dum->lock, flags);
return retval;
@@ -1523,9 +1543,7 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
if (req->req.status != -EINPROGRESS) {
list_del_init(&req->queue);

- spin_unlock(&dum->lock);
- usb_gadget_giveback_request(&ep->ep, &req->req);
- spin_lock(&dum->lock);
+ dummy_giveback(dum, &ep->ep, req);

/* requests might have been unlinked... */
rescan = 1;
@@ -1910,9 +1928,7 @@ static enum hrtimer_restart dummy_timer(struct hrtimer *t)
dev_dbg(udc_dev(dum), "stale req = %p\n",
req);

- spin_unlock(&dum->lock);
- usb_gadget_giveback_request(&ep->ep, &req->req);
- spin_lock(&dum->lock);
+ dummy_giveback(dum, &ep->ep, req);
ep->already_seen = 0;
goto restart;
}
--
2.53.0

[Alan Stern]

Wow! I'm impressed. How did you figure this out?

> ---
> 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.

> +/*
> + * 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.

Otherwise this seems fine.

Alan Stern

[Jinchao Wang]

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.106...@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.

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