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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: gadget: gadgetfs: unchecked reuse of dev->req clears
  ep0_req_queued
- [High] usb: gadget: gadgetfs: race on dev->req->buf frees descriptor
  in clean_req()
--

commit 6cf737aca44046a58d9c7ba5151c4864f9f42306
Author: Jing Lin <kayano050408@gmail.com>

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

This commit buffers delegated control OUT payloads in a separate
setup_buf array so dev->req can be reused before ep0_read() consumes the
data, and tracks whether dev->req is queued so gadgetfs_unbind() can
dequeue it before freeing.

> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 7f6d8460152f9..87aff568aa596 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
[ ... ]
> @@ -984,12 +987,23 @@ static inline void ep0_readable (struct dev_data *dev)
>  	kill_fasync (&dev->fasync, SIGIO, POLL_IN);
>  }
>  
> +static void dev_req_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct dev_data		*dev = ep->driver_data;
> +	unsigned long		flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	dev->ep0_req_queued = false;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +}
> +
>  static void clean_req (struct usb_ep *ep, struct usb_request *req)
>  {
>  	struct dev_data		*dev = ep->driver_data;
>  
>  	if (req->buf != dev->rbuf) {
> -		kfree(req->buf);
> +		if (req->buf != dev->setup_buf)
> +			kfree(req->buf);
>  		req->buf = dev->rbuf;
>  	}
>  	req->complete = epio_complete;
[ ... ]
> @@ -1074,10 +1093,15 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  
>  			if ((retval = setup_req (ep, req, 0)) == 0) {
>  				++dev->udc_usage;
> +				dev->ep0_req_queued = true;
>  				spin_unlock_irq (&dev->lock);
>  				retval = usb_ep_queue (ep, req, GFP_KERNEL);
>  				spin_lock_irq (&dev->lock);
>  				--dev->udc_usage;
> +				if (retval < 0) {
> +					dev->ep0_req_queued = false;
> +					clean_req(ep, req);
> +				}
>  			}
>  			dev->state = STATE_DEV_CONNECTED;
>  

[Severity: High]
Can reusing dev->req without checking dev->ep0_req_queued clear
dev->ep0_req_queued while dev->req is still queued on gadget->ep0?

When gadgetfs_setup() delegates a control OUT transfer with w_length > 0,
it queues dev->req on gadget->ep0, sets dev->ep0_req_queued = true, and
queues a GADGETFS_SETUP event.  Once userspace reads that event (entering
STATE_DEV_SETUP with dev->setup_in == 0), a call to ep0_read() with
len == 0 enters this branch while the OUT data stage request is still in
flight, overwrites dev->req in setup_req(), and calls usb_ep_queue().
The UDC rejects the already-queued request with -EINVAL, and the error
path sets dev->ep0_req_queued = false even though dev->req remains on the
endpoint queue.  The same false clearing happens in ep0_write() and
gadgetfs_setup() if a previous ep0 request is still queued when a new
transfer starts, or if a second ep0_read() / ep0_write() races while
dev->lock is dropped before dev->state is updated.

When gadgetfs_unbind() later runs, it sees dev->ep0_req_queued == false,
skips usb_ep_dequeue(), and frees the active request, triggering the
WARN_ON(!list_empty(&req->queue)) in dummy_free_request() and a
use-after-free when dummy_udc_stop() drains the queue.

[ ... ]

> @@ -1271,6 +1300,7 @@ ep0_write (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
>  			if (retval == 0) {
>  				dev->state = STATE_DEV_CONNECTED;
>  				++dev->udc_usage;
> +				dev->ep0_req_queued = true;
>  				spin_unlock_irq (&dev->lock);
>  				if (copy_from_user (dev->req->buf, buf, len))
>  					retval = -EFAULT;
> @@ -1284,6 +1314,7 @@ ep0_write (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
>  				spin_lock_irq(&dev->lock);
>  				--dev->udc_usage;
>  				if (retval < 0) {
> +					dev->ep0_req_queued = false;
>  					clean_req (dev->gadget->ep0, dev->req);
>  				} else
>  					retval = len;

[Severity: High]
Can a concurrent USB_REQ_GET_DESCRIPTOR setup request while dev->lock is
dropped here cause copy_from_user() to overwrite descriptor memory or
clean_req() to kfree() it?

ep0_write() sets dev->state = STATE_DEV_CONNECTED and drops dev->lock
before copy_from_user() and usb_ep_queue().  If a USB_REQ_GET_DESCRIPTOR
request arrives in gadgetfs_setup() during that window, it sees
dev->state != STATE_DEV_SETUP and points dev->req->buf at dev->dev,
dev->config, or dev->hs_config (all inside dev->buf), and
dev_req_complete() leaves dev->req->buf pointing there:

CPU0                                  CPU1
ep0_write()
  setup_req()
    dev->req->buf = dev->rbuf
  dev->state = STATE_DEV_CONNECTED
  spin_unlock_irq(&dev->lock)
                                      gadgetfs_setup()
                                        case USB_DT_CONFIG:
                                          config_buf()
                                            dev->req->buf = dev->config
                                        usb_ep_queue(..., dev->req, ...)
  copy_from_user(dev->req->buf, ...)
    --> overwrites dev->config or
        returns -EFAULT
  spin_lock_irq(&dev->lock)
  clean_req()
    kfree(req->buf)                   <-- frees dev->config (dev->buf)

If copy_from_user() succeeds, up to RBUF_SIZE (256) bytes are written into
the descriptor buffer, overflowing the 18-byte dev->dev at the end of
dev->buf or corrupting dev->config.  If copy_from_user() or usb_ep_queue()
fails (here or in the new clean_req() error path in ep0_read()),
clean_req() sees req->buf is neither dev->rbuf nor dev->setup_buf and
calls kfree(req->buf), either freeing dev->buf while the gadget is still
active (leading to use-after-free and double free in dev_release()) or
passing an interior slab pointer (dev->hs_config or dev->dev) to kfree().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-b4-gadgetfs-ep0-v3-1-62089dc0ec07@gmail.com?part=1