usb: gadget: dummy_hcd: prevent fifo_req reuse during giveback

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 covering the shared request's whole lifetime:
set it in dummy_queue() when the FIFO fast-path takes fifo_req (making
it the fast-path guard, replacing the list_empty(&fifo_req.queue)
test), and clear it after the completion callback has returned, via a
dummy_giveback() helper used at all four gadget-request giveback
sites.  The shared slot can no longer be reused until its completion
callback has finished.

Reported-by: syzbot+faf3a6cf579fc65591ca@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=faf3a6cf579fc65591ca
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@kernel.org>
Signed-off-by: Jinchao Wang <wangjinchao600@gmail.com>
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Link: https://patch.msgid.link/5db8bba5b3499a86cd2e776f9918126b68b2508b.1784198306.git.wangjinchao600@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index f47903461ed5ca2744d4b48409bd6168e5615a2c..c0e40fa6dde586c33c606984fd60ccaffaeb9a09 100644 (file)
--- 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
@@ -329,6 +330,26 @@ static inline struct dummy *gadget_dev_to_dummy(struct device *dev)
 
 /* DEVICE/GADGET SIDE UTILITY ROUTINES */
 
+/*
+ * Give back a gadget request with dum->lock dropped around the callback.
+ * If @req is the shared fifo_req, clear fifo_req_busy afterward: the flag
+ * was set in dummy_queue() when the shared request was taken and must stay
+ * set until its completion callback has returned; list_del_init() alone
+ * makes the request look idle while the callback is still running.
+ * 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;
+
+       spin_unlock(&dum->lock);
+       usb_gadget_giveback_request(_ep, &req->req);
+       spin_lock(&dum->lock);
+       if (fifo)
+               dum->fifo_req_busy = 0;
+}
+
 /* called with spinlock held */
 static void nuke(struct dummy *dum, struct dummy_ep *ep)
 {
@@ -339,9 +360,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);
        }
 }
 
@@ -728,10 +747,11 @@ 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;
+               dum->fifo_req_busy = 1;
                req->req = *_req;
                req->req.buf = dum->fifo_buf;
                memcpy(dum->fifo_buf, _req->buf, _req->length);
@@ -785,9 +805,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 +1541,7 @@ top:
                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 +1926,7 @@ restart:
                                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;
                        }