Re: [PATCH v5 1/3] usb: gadget: gadgetfs: fix ep0 request lifetime on disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] usb: gadget: gadgetfs: invalid kfree of descriptors in
  gadgetfs_unbind()
- [Medium] usb: gadget: gadgetfs: premature state change in ep0_write()
  hides abort
- [Medium] usb: gadget: gadgetfs: incomplete setup_abort handling in
  ep0_read()
- [Medium] usb: gadget: gadgetfs: setup_abort in ep0_read() clobbers
  unbound state
--

commit 3d2d9c4891365d79ece2e8243c8a799fb412498e
Author: Jing Lin <kayano050408@gmail.com>

usb: gadget: gadgetfs: fix ep0 request lifetime on disconnect

This commit tracks whether the preallocated ep0 request is queued, stages
delegated control payloads through private buffers so copy_to_user() and
copy_from_user() do not race with gadgetfs_setup() on dev->req->buf, and
dequeues dev->req in gadgetfs_unbind() before freeing it.

> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 7f6d8460152f9..e9380538e0b98 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
[ ... ]
> @@ -1109,25 +1134,37 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  			if (retval)
>  				goto done;
>  
> -			if (dev->state != STATE_DEV_SETUP) {
> +			if (dev->setup_abort) {
> +				dev->setup_abort = 0;
> +				dev->state = STATE_DEV_CONNECTED;
> +				retval = -EIDRM;
> +				goto done;
> +			}

[Severity: Medium]
Can ep0_read() ever reach this dev->setup_abort check when a delegated
control OUT transfer is aborted before its data stage completes?

Immediately above this hunk, ep0_read() sleeps in:

drivers/usb/gadget/legacy/inode.c:ep0_read() {
    ...
			retval = wait_event_interruptible (dev->wait,
					dev->setup_out_ready != 0);
    ...
}

When a new SETUP packet aborts the pending OUT transfer in
gadgetfs_setup(), dev->setup_abort is set to 1 while dev->setup_out_ready
remains 0 (or is reset to 0 for a new delegated setup), so the wait
condition is still false and ep0_read() stays asleep instead of returning
-EIDRM.  If the earlier OUT transfer had already set dev->setup_out_ready
before an in-kernel setup aborted it, this branch also returns -EIDRM
without clearing dev->setup_out_ready, dev->setup_out_error, or
dev->setup_out_len.

In addition, if the abort arrives before userspace calls ep0_read(),
ep0_write(), or ep0_poll(), the entry checks in those functions clear
dev->setup_abort = 0 without resetting dev->state from STATE_DEV_SETUP to
STATE_DEV_CONNECTED, so the next event read is still misdirected into the
control data stage path.

> +			if (dev->state != STATE_DEV_SETUP ||
> +			    dev->setup_in || !dev->setup_out_ready) {
>  				retval = -ECANCELED;
>  				goto done;
>  			}

[Severity: Medium]
Does handling dev->setup_abort before checking dev->state overwrite
STATE_DEV_UNBOUND if gadgetfs_unbind() ran while ep0_read() was sleeping?

CPU0                                    CPU1
ep0_read()
  spin_unlock_irq(&dev->lock)
  wait_event_interruptible(...)
                                        gadgetfs_setup()
                                          dev->setup_abort = 1
                                          ...
                                        ep0_complete()
                                          dev->setup_out_ready = 1
                                          ep0_readable(dev)
                                        gadgetfs_unbind()
                                          dev->state = STATE_DEV_UNBOUND
                                          usb_ep_free_request(..., dev->req)
                                          dev->req = NULL
                                          gadget->ep0->driver_data = NULL
  spin_lock_irq(&dev->lock)
  if (dev->setup_abort)
    dev->state = STATE_DEV_CONNECTED  <-- clobbers STATE_DEV_UNBOUND

Because wait_event_interruptible() runs with dev->lock dropped and without
incrementing dev->udc_usage, gadgetfs_unbind() can finish tearing down
dev->req and gadget->ep0->driver_data before ep0_read() reacquires
dev->lock.  Setting dev->state = STATE_DEV_CONNECTED before verifying that
dev->state is still STATE_DEV_SETUP allows subsequent ep0_read() and
ep0_write() calls to pass the dev->state <= STATE_DEV_OPENED check and
dereference NULL ep->driver_data and dev->req in setup_req().

[ ... ]

> @@ -1267,29 +1304,41 @@ ep0_write (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
>  
>  		len = min_t(size_t, len, dev->setup_wLength);
>  		if (dev->setup_in) {
> -			retval = setup_req (dev->gadget->ep0, dev->req, len);
> +			u8	tmp[RBUF_SIZE];
> +			bool	zero = len < dev->setup_wLength;
> +
> +			dev->state = STATE_DEV_CONNECTED;
> +			spin_unlock_irq(&dev->lock);
> +			if (copy_from_user(tmp, buf, len)) {
> +				spin_lock_irq(&dev->lock);
> +				return -EFAULT;
> +			}
> +			spin_lock_irq(&dev->lock);
> +			if (dev->setup_abort) {
> +				dev->setup_abort = 0;
> +				return -EIDRM;
> +			}
> +			if (dev->state != STATE_DEV_CONNECTED)
> +				return -ECANCELED;
> +			retval = setup_req(dev->gadget->ep0, dev->req, len);

[Severity: Medium]
Does setting dev->state = STATE_DEV_CONNECTED before dropping dev->lock
prevent ep0_write() from detecting an abort during copy_from_user()?

CPU0                                    CPU1
ep0_write()
  dev->state = STATE_DEV_CONNECTED
  spin_unlock_irq(&dev->lock)
  copy_from_user(tmp, buf, len)
                                        gadgetfs_setup()
                                          dev->setup_abort = 0
                                          if (dev->state == STATE_DEV_SETUP)
                                            dev->setup_abort = 1  <-- skipped
                                          /* new delegated setup queued */
  spin_lock_irq(&dev->lock)
  if (dev->setup_abort)  <-- false
  if (dev->state != STATE_DEV_CONNECTED)  <-- false
  setup_req(dev->gadget->ep0, dev->req, len)
  usb_ep_queue(dev->gadget->ep0, dev->req, ...)  <-- queues stale response

gadgetfs_setup() and next_event() only set dev->setup_abort = 1 when
dev->state == STATE_DEV_SETUP, and gadgetfs_setup() clears
dev->setup_abort = 0 at entry.  Because ep0_write() has already changed
dev->state to STATE_DEV_CONNECTED before sleeping in copy_from_user(), a
new SETUP packet arriving in that window leaves dev->setup_abort == 0 and
dev->state == STATE_DEV_CONNECTED, so ep0_write() queues the stale IN
payload onto ep0 for the new control transfer (and leaves dev->state as
STATE_DEV_CONNECTED even if copy_from_user() fails with -EFAULT).

[ ... ]

> @@ -1781,15 +1843,21 @@ gadgetfs_unbind (struct usb_gadget *gadget)
>  		usleep_range(1000, 2000);
>  		spin_lock_irq(&dev->lock);
>  	}
> +	queued = dev->ep0_req_queued;
>  	spin_unlock_irq (&dev->lock);
>  
> +	if (dev->req) {
> +		if (queued)
> +			usb_ep_dequeue(gadget->ep0, dev->req);
> +		clean_req(gadget->ep0, dev->req);
> +		usb_ep_free_request(gadget->ep0, dev->req);
> +		dev->req = NULL;
> +	}

[Severity: High]
Can clean_req() here call kfree() on dev->config, dev->hs_config, or
dev->dev when an in-kernel GET_DESCRIPTOR request is active during unbind?

In gadgetfs_setup(), GET_DESCRIPTOR points dev->req->buf at dev->dev,
dev->hs_config, or dev->config (which all live inside the single dev->buf
allocation freed in dev_release()), while clean_req() frees any buffer
other than dev->rbuf:

drivers/usb/gadget/legacy/inode.c:clean_req() {
    ...
	if (req->buf != dev->rbuf) {
		kfree(req->buf);
		req->buf = dev->rbuf;
	}
    ...
}

If usb_ep_dequeue() on ep0 returns an error without invoking
dev_req_complete() (for example on UDCs whose ep0 does not support
dequeue, or when the controller is already completing the request on
another CPU) or completes asynchronously, dev->req->buf is not reset to
dev->rbuf before clean_req() runs.  clean_req() then passes dev->config
(causing a double free when dev_release() frees dev->buf) or interior
pointers dev->hs_config / dev->dev to kfree(), and usb_ep_free_request()
frees dev->req while the UDC may still complete it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-b4-gadgetfs-ep0-v5-0-f16d0b648bc2@gmail.com?part=1