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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] usb: gadget: gadgetfs: unbind race with dev_req_complete() and
  ep0_complete()
- [High] usb: gadget: gadgetfs: unbounded hang in gadgetfs_unbind() when
  ep0 dequeue fails
- [Medium] usb: gadget: gadgetfs: ep0_poll() consumes setup_abort via
  ep0_check_abort()
- [Medium] usb: gadget: gadgetfs: early returns in gadgetfs_setup() skip
  setup_abort
- [Medium] usb: gadget: gadgetfs: ep0_read() wait misses state and
  generation changes
- [Medium] usb: gadget: gadgetfs: 0-length ep0_read() clobbers control
  OUT payload state
- [Medium] usb: gadget: gadgetfs: ep0_write() sets STATE_DEV_CONNECTED
  before user copy
- [Medium] usb: gadget: gadgetfs: ep0_read() overwrites
  STATE_DEV_UNBOUND on setup event
--

commit 143b1de0fb83afcb84b9c88be79a0db5944a14a3
Author: Jing Lin <kayano050408@gmail.com>

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

This commit serializes ep0 request lifetime and buffer ownership across
control transfers, userspace ep0 reads and writes, and gadget unbind.

> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 7f6d8460152f9..1277982f194c2 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c

[ ... ]

> @@ -984,54 +989,82 @@ static inline void ep0_readable (struct dev_data *dev)
>  	kill_fasync (&dev->fasync, SIGIO, POLL_IN);
>  }
>  
> -static void clean_req (struct usb_ep *ep, struct usb_request *req)
> +static void dev_req_complete(struct usb_ep *ep, struct usb_request *req);
> +
> +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);
> -		req->buf = dev->rbuf;
> -	}
> -	req->complete = epio_complete;
> -	dev->setup_out_ready = 0;
> +	/*
> +	 * req->buf may also point into dev->buf, which holds the
> +	 * descriptors; only the buffer allocated by setup_req() belongs
> +	 * to the request and may be freed here.
> +	 */
> +	kfree(dev->ep0_req_buf);
> +	dev->ep0_req_buf = NULL;
> +	req->buf = dev->rbuf;
> +	req->complete = dev_req_complete;
> +}
> +
> +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);
> +	clean_req(ep, req);
> +	dev->ep0_req_queued = false;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	/* gadgetfs_unbind() waits for this before freeing the request */
> +	wake_up(&dev->wait);
>  }
>  
>  static void ep0_complete (struct usb_ep *ep, struct usb_request *req)
>  {
>  	struct dev_data		*dev = ep->driver_data;
>  	unsigned long		flags;
> -	int			free = 1;
>  
>  	/* for control OUT, data must still get to userspace */
>  	spin_lock_irqsave(&dev->lock, flags);
> -	if (!dev->setup_in) {
> +	if (!dev->setup_in && dev->setup_wLength) {
>  		dev->setup_out_error = (req->status != 0);
> -		if (!dev->setup_out_error)
> -			free = 0;
> +		if (dev->setup_out_error) {
> +			dev->setup_out_len = 0;
> +		} else {
> +			dev->setup_out_len = min_t(unsigned int, req->actual,
> +						   sizeof(dev->setup_buf));
> +			memcpy(dev->setup_buf, req->buf, dev->setup_out_len);
> +		}
>  		dev->setup_out_ready = 1;
> -		ep0_readable (dev);
> +		ep0_readable(dev);
>  	}
>  
> -	/* clean up as appropriate */
> -	if (free && req->buf != &dev->rbuf)
> -		clean_req (ep, req);
> -	req->complete = epio_complete;
> +	clean_req(ep, req);
> +	dev->ep0_req_queued = false;
>  	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	/* gadgetfs_unbind() waits for this before freeing the request */
> +	wake_up(&dev->wait);
>  }
>  
>  static int setup_req (struct usb_ep *ep, struct usb_request *req, u16 len)
>  {
>  	struct dev_data	*dev = ep->driver_data;
>  
> -	if (dev->setup_out_ready) {
> +	if (dev->ep0_req_queued) {
>  		DBG (dev, "ep0 request busy!\n");
>  		return -EBUSY;
>  	}

[ ... ]

> @@ -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);
> +				}
> +			} else {
> +				dev->state = STATE_DEV_CONNECTED;
>  			}
> -			dev->state = STATE_DEV_CONNECTED;

[Severity: Medium]
Can a zero-length ep0_read() during a delegated control OUT transfer queue
a bogus status-stage request and clobber dev->setup_out_len?

Previously, setup_req() rejected requests while dev->setup_out_ready was
set.  Now ep0_complete() clears dev->ep0_req_queued as soon as the OUT data
stage completes, and the len == 0 branch in ep0_read() does not check that
dev->setup_wLength == 0.  If userspace calls read(fd, buf, 0) after the OUT
data stage finishes, setup_req() succeeds and queues a 0-length request
whose ep0_complete() callback sees !dev->setup_in && dev->setup_wLength,
overwriting dev->setup_out_len with 0 and setting dev->setup_out_ready = 1
after dev->state has already returned to STATE_DEV_CONNECTED.

> @@ -1102,32 +1159,43 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  			}
>  			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 */
>  			spin_lock_irq (&dev->lock);
>  			if (retval)
>  				goto done;
>  
> -			if (dev->state != STATE_DEV_SETUP) {
> +			if (ep0_check_abort(dev)) {
> +				retval = -EIDRM;
> +				goto done;
> +			}
> +			if (dev->state != STATE_DEV_SETUP ||
> +			    dev->setup_in || !dev->setup_out_ready) {
>  				retval = -ECANCELED;
>  				goto done;
>  			}

[Severity: Medium]
Can wait_event_interruptible() in ep0_read() miss a disconnect, unbind, or
superseded control transfer?

The wait condition only checks dev->setup_out_ready || dev->setup_abort, so
if dev->state leaves STATE_DEV_SETUP without setting either flag (such as
in gadgetfs_disconnect() or gadgetfs_unbind()), ep0_read() stays blocked
instead of waking to return -ECANCELED.  In addition, ep0_read() does not
snapshot dev->setup_generation before dropping dev->lock, so if a new
delegated control OUT request is read and completes while the first reader
is asleep, the first ep0_read() can wake and consume the new transfer's
dev->setup_buf.

[ ... ]

> @@ -1267,29 +1334,45 @@ 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;
> +			unsigned int generation = dev->setup_generation;
> +
> +			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);
> +			/*
> +			 * A control request that arrived while dev->lock was
> +			 * dropped supersedes this data stage; do not queue the
> +			 * stale response for it.
> +			 */
> +			if (dev->setup_generation != generation)
> +				return -EIDRM;
> +			if (dev->state != STATE_DEV_CONNECTED)
> +				return -ECANCELED;

[Severity: Medium]
Does setting dev->state = STATE_DEV_CONNECTED before copy_from_user()
leave the state machine out of STATE_DEV_SETUP if the copy faults or races
with gadgetfs_setup()?

If copy_from_user() returns -EFAULT, ep0_write() returns without restoring
dev->state to STATE_DEV_SETUP even though no response was queued.  Also, if
a new SETUP packet arrives in gadgetfs_setup() while dev->lock is dropped
for copy_from_user(), gadgetfs_setup() sees dev->state ==
STATE_DEV_CONNECTED instead of STATE_DEV_SETUP and skips setting
dev->setup_abort = 1.

[ ... ]

> @@ -1360,8 +1443,7 @@ ep0_poll (struct file *fd, poll_table *wait)
>  	spin_lock_irq(&dev->lock);
>  
>  	/* report fd mode change before acting on it */
> -	if (dev->setup_abort) {
> -		dev->setup_abort = 0;
> +	if (ep0_check_abort(dev)) {
>  		mask = EPOLLHUP;
>  		goto out;
>  	}

[Severity: Medium]
Does calling ep0_check_abort() in ep0_poll() consume dev->setup_abort and
reset dev->state before ep0_read() or ep0_write() can see the abort?

ep0_check_abort() clears dev->setup_abort and transitions dev->state from
STATE_DEV_SETUP to STATE_DEV_CONNECTED.  If a process polls ep0 before
issuing read() or write(), or if a concurrent poll runs while ep0_read() is
waking from wait_event_interruptible(), ep0_poll() consumes the abort:
a waiting ep0_read() can miss dev->setup_abort and stay asleep, while a
subsequent control-data ep0_read() sees STATE_DEV_CONNECTED and falls
through into the event-reading path instead of returning -EIDRM.

> @@ -1470,6 +1552,10 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  	u16				w_value = le16_to_cpu(ctrl->wValue);
>  	u16				w_length = le16_to_cpu(ctrl->wLength);
>  
> +	spin_lock(&dev->lock);
> +	/* invalidate userspace copies started for an older control request */
> +	dev->setup_generation++;
> +
>  	if (w_length > RBUF_SIZE) {
>  		if (ctrl->bRequestType & USB_DIR_IN) {
>  			/* Cast away the const, we are going to overwrite on purpose. */
> @@ -1478,11 +1564,15 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  			*temp = cpu_to_le16(RBUF_SIZE);
>  			w_length = RBUF_SIZE;
>  		} else {
> +			spin_unlock(&dev->lock);
>  			return value;
>  		}
>  	}
>  
> -	spin_lock (&dev->lock);
> +	if (dev->ep0_req_queued) {
> +		spin_unlock(&dev->lock);
> +		return -EBUSY;
> +	}
>  	dev->setup_abort = 0;
>  	if (dev->state == STATE_DEV_UNCONNECTED) {
>  		if (gadget_is_dualspeed(gadget)
> @@ -1505,8 +1595,11 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  	 * then ep0_{read,write} will report the wrong status. controller
>  	 * driver will have aborted pending i/o.
>  	 */
> -	} else if (dev->state == STATE_DEV_SETUP)
> +	} else if (dev->state == STATE_DEV_SETUP) {
>  		dev->setup_abort = 1;
> +		/* wake an ep0_read() waiting for the aborted data stage */
> +		ep0_readable(dev);
> +	}

[Severity: Medium]
If gadgetfs_setup() returns early on w_length > RBUF_SIZE or
dev->ep0_req_queued while dev->state == STATE_DEV_SETUP, does it leave the
aborted transfer in STATE_DEV_SETUP without setting dev->setup_abort?

Both early returns happen after dev->setup_generation++ but before the
dev->state == STATE_DEV_SETUP check.  An ep0_read() waiting for the prior
transfer's OUT data stage is never woken, and a subsequent ep0_write()
called to answer the aborted IN transfer sees dev->setup_abort == 0,
snapshots the already-incremented dev->setup_generation, and queues its
stale response onto ep0.

[ ... ]

> @@ -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);
> +		}
> +		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);
>  	gadget->ep0->driver_data = NULL;
>  	set_gadget_data (gadget, NULL);
>  
> -	/* we've already been disconnected ... no i/o is active */
> -	if (dev->req)
> -		usb_ep_free_request (gadget->ep0, dev->req);
>  	DBG (dev, "%s done\n", __func__);
>  	put_dev (dev);
>  }

[Severity: High]
Can gadgetfs_unbind() race with dev_req_complete() or ep0_complete() on
dev->req and dev->wait?

CPU0                                    CPU1
dev_req_complete() / ep0_complete()     gadgetfs_unbind()
  spin_lock_irqsave(&dev->lock, flags)
  clean_req(ep, req)
  dev->ep0_req_queued = false
                                          wait_event(dev->wait,
                                                     !dev->ep0_req_queued)
                                          clean_req(gadget->ep0, dev->req)
                                          usb_ep_free_request(..., dev->req)
                                          ...
                                          put_dev(dev) -> kfree(dev)
  spin_unlock_irqrestore(&dev->lock, ...)
  wake_up(&dev->wait)  <-- use-after-free

wait_event() can observe !dev->ep0_req_queued and return before CPU0
releases dev->lock or calls wake_up(&dev->wait) (especially in
ep0_complete(), where ep0_readable() wakes dev->wait before clearing
dev->ep0_req_queued).  gadgetfs_unbind() then runs clean_req() and
usb_ep_free_request() without holding dev->lock while the completion
handler is still executing, and if the last reference on dev is dropped,
wake_up(&dev->wait) touches freed memory or triggers the
BUG_ON(waitqueue_active(&data->wait)) in put_dev().

[Severity: High]
Can wait_event(dev->wait, !dev->ep0_req_queued) hang forever in
gadgetfs_unbind() when usb_ep_dequeue() fails on ep0?

usb_ep_dequeue() only guarantees a completion callback when it returns 0,
and several UDC drivers (such as musb_g_ep0_dequeue(), mtu3_ep0_dequeue(),
pxa27x, pxa25x, and at91) do not support dequeuing ep0 requests and return
-EINVAL without giving back the request.  When pullup() on disconnect does
not nuke ep0 before unbind(), ignoring the usb_ep_dequeue() return value
leaves gadgetfs_unbind() sleeping in TASK_UNINTERRUPTIBLE forever.

[Severity: Medium]
Can setting dev->req = NULL and gadget->ep0->driver_data = NULL in
gadgetfs_unbind() lead to a NULL pointer dereference in setup_req() if a
GADGETFS_SETUP event was already queued?

gadgetfs_unbind() sets dev->state = STATE_DEV_UNBOUND but leaves queued
events in dev->event[].  If userspace calls ep0_read() after unbind, the
scan loop still transitions dev->state to STATE_DEV_SETUP when delivering a
queued GADGETFS_SETUP event:

drivers/usb/gadget/legacy/inode.c:ep0_read() {
    ...
		for (i = 0; i < n; i++) {
			if (dev->event [i].type == GADGETFS_SETUP) {
				dev->state = STATE_DEV_SETUP;
				n = i + 1;
				break;
			}
		}
    ...
}

A subsequent ep0_write() or 0-length ep0_read() then sees STATE_DEV_SETUP
and calls setup_req(dev->gadget->ep0, dev->req, ...), where ep->driver_data
and dev->req are now NULL.

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