Re: [PATCH RFC] usb: gadget: gadgetfs: dequeue pending ep0 request before freeing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: gadget: gadgetfs: gadgetfs_setup() reuses req while
  setup_out_ready
- [Low] usb: gadget: gadgetfs: invalid Fixes SHA and syzbot command in
  changelog
--

commit 30f3f2dca823922d4eaa362244e46b92716f82d8
Author: Jing Lin <kayano050408@gmail.com>

> usb: gadget: gadgetfs: dequeue pending ep0 request before freeing
>
> Please test this gadgetfs ep0 request lifetime fix
> The unpatched base reproduces the warning and use-after-free locally
> on arm64 QEMU with dummy_hcd and KASAN
>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f66469d9905b39d2c260a15d7d6db450f4db67c1
>
> gadgetfs_unbind() frees the preallocated ep0 request after disconnecting
> the gadget, assuming that no I/O remains active
>
> With dummy_hcd, disconnecting does not necessarily drain ep0, so a delayed
> userspace response can leave dev->req queued when it is freed
> This triggers the warning in dummy_free_request() and a use-after-free
> when dummy_udc_stop() subsequently drains the endpoint queue
>
> Track dev->req from submission through completion under dev->lock and
> reject reuse while it is busy
> After waiting for active UDC users to finish, dequeue a pending request
> before freeing it and clearing ep0->driver_data
> Drop dev->lock around dequeue because the completion callback takes it
> Only dequeue pending requests to avoid errors from UDCs that reject
> cancellation of an unqueued request
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+c73e1942197af9f54ca3@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c73e1942197af9f54ca3
> Signed-off-by: Jing Lin <kayano050408@gmail.com>

[Severity: Low]
The conversational testing notes and the "#syz test:" command at the top of
the commit message should be removed before applying.  Also, the 12-character
prefix 1da177e4c3f4 in the Fixes: trailer cannot be uniquely resolved in git
history.

> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 7f6d8460152f9..d60df4ad38a3e 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
[ ... ]
> @@ -1508,6 +1529,11 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  	} else if (dev->state == STATE_DEV_SETUP)
>  		dev->setup_abort = 1;
>  
> +	if (dev->ep0_req_queued) {
> +		spin_unlock(&dev->lock);
> +		return -EBUSY;
> +	}
> +
>  	req->buf = dev->rbuf;
>  	req->context = NULL;
>  	switch (ctrl->bRequest) {

[Severity: High]
Can gadgetfs_setup() reuse dev->req while ep0_read() is still copying out a
completed control OUT transfer?

When a delegated control OUT transfer completes, ep0_complete() clears
dev->ep0_req_queued and sets dev->setup_out_ready = 1, leaving the payload in
dev->req->buf until ep0_read() copies it to userspace and calls clean_req():

drivers/usb/gadget/legacy/inode.c:ep0_read() {
    ...
				len = min (len, (size_t)dev->req->actual);
				++dev->udc_usage;
				spin_unlock_irq(&dev->lock);
				if (copy_to_user (buf, dev->req->buf, len))
					retval = -EFAULT;
				else
					retval = len;
				spin_lock_irq(&dev->lock);
				--dev->udc_usage;
				clean_req (dev->gadget->ep0, dev->req);
    ...
}

Because gadgetfs_setup() only checks dev->ep0_req_queued (and clears
dev->setup_out_ready = 0 at the delegate label before calling setup_req()), a
new SETUP packet arriving while ep0_read() has dropped dev->lock for
copy_to_user() will reuse dev->req:

CPU0                              CPU1
ep0_read()
  spin_unlock_irq(&dev->lock)
                                  gadgetfs_setup()
                                    /* USB_REQ_GET_DESCRIPTOR */
                                    req->buf = dev->config;
                                    usb_ep_queue(gadget->ep0, req, ...)
  copy_to_user(buf, dev->req->buf, len)
  spin_lock_irq(&dev->lock)
  clean_req(gadget->ep0, dev->req)
    kfree(req->buf)               <-- frees dev->config / interior pointer

If the new SETUP request is USB_REQ_GET_DESCRIPTOR, req->buf is pointed at
dev->dev (the 18-byte descriptor at the tail of dev->buf), dev->hs_config (an
interior pointer into dev->buf), or dev->config (dev->buf itself).
copy_to_user() can then read up to 238 bytes past the end of dev->buf when
req->buf is dev->dev, and clean_req() then passes req->buf to kfree(),
either crashing SLUB on an interior pointer or freeing dev->buf while it is
queued in the UDC and before dev_release() frees it again.  If the new SETUP
request is another delegated control OUT transfer, gadgetfs_setup() clears
dev->setup_out_ready = 0 and re-queues dev->req, after which ep0_read()'s
clean_req() frees the new req->buf while queued and resets req->complete.

Checking dev->setup_out_ready alongside dev->ep0_req_queued in
gadgetfs_setup() would prevent reusing dev->req until ep0_read() finishes
with it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179397691694.27761.9543494907127681190.gadgetfs-ep0-patch-test@gmail.com?part=1