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

[Alan Stern]

On Mon, Nov 09, 2026 at 02:50:51PM +0800, Jing Lin via B4 Relay wrote:
> From: Jing Lin <kayano050408@xxxxxxxxx>
> 
> 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.
> 
> gadgetfs_setup() and the userspace copy helpers also race on dev->req and
> req->buf:
> 
> 1. ep0_read() versus gadgetfs_setup().  A delegated control OUT request
>    is queued by gadgetfs_setup() and completed by ep0_complete(), which
>    leaves the payload in dev->req->buf and sets setup_out_ready.
>    ep0_read() copies that payload with copy_to_user() after dropping
>    dev->lock, so the copy can fault and sleep.  If a new control transfer
>    arrives during that window, gadgetfs_setup() reuses dev->req for it;
>    for example, GET_DESCRIPTOR(config) points req->buf at dev->config.
>    When ep0_read() resumes, clean_req() frees whatever req->buf points to
>    at that moment, which is now dev->config.  The following
>    GET_DESCRIPTOR(config) dereferences the freed descriptor.
> 
> 2. ep0_write() versus gadgetfs_setup().  For a delegated control IN
>    request, ep0_write() prepares dev->req and copies the response from
>    userspace with copy_from_user() after dropping dev->lock.  A control
>    request that arrives during that copy can point req->buf at dev->dev
>    or dev->config, so the userspace copy overwrites a descriptor buffer,
>    or it can reset req->buf to dev->rbuf, so the request is queued with
>    the wrong buffer.  Completion then calls clean_req() on the wrong
>    object.
> 
> Copy completed control OUT payloads into a separate setup_buf in
> ep0_complete(), copy control IN payloads from userspace into a stack
> buffer before preparing dev->req in ep0_write(), and reset req->buf when
> control transfers finish.  Track whether dev->req is queued, reject reuse
> while it is queued, and dequeue it without dev->lock before freeing it in
> gadgetfs_unbind().
> 
> Only the buffer allocated by setup_req() belongs to the request, while
> descriptor responses point req->buf into dev->buf.  Track the allocation
> explicitly so clean_req() cannot free a descriptor buffer when a request
> is destroyed while an in-kernel GET_DESCRIPTOR response is active, and
> wait for the asynchronous usb_ep_dequeue() completion before freeing the
> request.
> 
> Count setup callbacks and recheck the count in ep0_write() after
> copy_from_user(), so a control request that arrives while dev->lock is
> dropped invalidates the stale response instead of having it queued for
> the new transfer.
> 
> Wake an ep0_read() that waits for a delegated OUT data stage when a new
> setup aborts it, and include setup_abort in the wait condition so the
> abort is not missed.
> 
> Consume setup_abort through one helper that returns to
> STATE_DEV_CONNECTED only when the state is still STATE_DEV_SETUP.  This
> keeps the following event read out of the control data path without
> overwriting STATE_DEV_UNBOUND when gadgetfs_unbind() ran concurrently.

This seems like a lot of material for a single patch.  You could break 
it into two separate patches, one to handle the ep0 abort races and one 
to wait for outstanding requests to complete before freeing dev->req.  
Maybe subdivide it even more, if you think that would help.

> The setup() callback runs in interrupt context and may not sleep, so a
> mutex cannot be used to serialize it against ep0_read() and ep0_write().
> The shared request state is protected by dev->lock and the payloads are
> handed off through private buffers instead.

This sort of material (saying what you decided _not_ to do) doesn't 
belong in the patch description.  If anywhere, it should go in the 0/N 
summary message.

> @@ -1073,13 +1123,20 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  			struct usb_request	*req = dev->req;
>  
>  			if ((retval = setup_req (ep, req, 0)) == 0) {
> +				dev->state = STATE_DEV_CONNECTED;
>  				++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);
> +				}

Here, and every other place where dev->ep0_req_queued is changed to 
false, you need to call wake_up(&dev->wait).

>  			spin_unlock_irq (&dev->lock);
>  			retval = wait_event_interruptible (dev->wait,
> -					dev->setup_out_ready != 0);
> +					dev->setup_out_ready ||
> +					dev->setup_abort);
>  
>  			/* FIXME state could change from under us */
> @@ -1781,15 +1883,29 @@ 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);
> +			/*
> +			 * usb_ep_dequeue() may return before the completion
> +			 * routine runs.  Do not free the request until the
> +			 * controller has given it back.
> +			 */
> +			wait_event(dev->wait, !dev->ep0_req_queued);

Otherwise, how will this wait_event() call be triggered, if it is 
waiting for a failed submission?

Also, it would be worthwhile to create a separate subroutine just for 
submitting dev->req.  The same code is duplicated several times in the 
driver.

> +		}
> +		dev->req->buf = dev->rbuf;
> +		clean_req(gadget->ep0, dev->req);
> +		usb_ep_free_request(gadget->ep0, dev->req);
> +		dev->req = NULL;
> +	}
> +
>  	destroy_ep_files (dev);

Logically, this destroy_ep_files() call belongs before the dev->req 
stuff, because ep0 is created before and destroyed after all the other 
endpoints.

Alan Stern