Re: [PATCH v7 2/4] usb: gadget: gadgetfs: fix ep0 setup abort and unbind state races

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: gadgetfs: stale GADGETFS_SETUP event not aborted
  in gadgetfs_setup()
--

commit 774082278f145f38c9e9ce530cac34febc2f5393
Author: Jing Lin <kayano050408@gmail.com>

usb: gadget: gadgetfs: fix ep0 setup abort and unbind state races

This commit fixes ep0 state machine races when control requests,
disconnects, or unbinds overlap with userspace ep0 read, write, and poll
operations.

> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 1e7e8f4bb4916..6514220efc821 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
[ ... ]
> @@ -1545,6 +1589,21 @@ 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++;
> +
> +	/*
> +	 * Host may have given up waiting for a response to a previous
> +	 * delegated transfer.  Record the abort and return to the connected
> +	 * state before any early return or in-kernel response handling.
> +	 */
> +	if (dev->state == STATE_DEV_SETUP) {
> +		dev->setup_abort = 1;
> +		dev->state = STATE_DEV_CONNECTED;
> +		ep0_readable(dev);
> +	}

[Severity: Medium]
What happens if a new SETUP packet arrives while a previous delegated
GADGETFS_SETUP event is still unread in dev->event[]?

When gadgetfs_setup() delegates a control request, dev->state stays in
STATE_DEV_CONNECTED until ep0_read() consumes the queued GADGETFS_SETUP
event:

drivers/usb/gadget/legacy/inode.c:gadgetfs_setup() {
    ...
delegate:
			dev->setup_in = (ctrl->bRequestType & USB_DIR_IN)
						? 1 : 0;
			dev->setup_wLength = w_length;
    ...
			/* state changes when reader collects event */
			event = next_event (dev, GADGETFS_SETUP);
    ...
}

If a second SETUP packet arrives before ep0_read() runs and is handled in
the kernel (such as USB_REQ_GET_DESCRIPTOR) or returns early without
calling next_event(dev, GADGETFS_SETUP), the STATE_DEV_SETUP check above
is skipped, leaving dev->setup_abort = 0 and the old GADGETFS_SETUP event
in dev->event[].  If the second request fails in setup_req() or
queue_ep0_req() at the delegate label, it also overwrites dev->setup_in,
dev->setup_wLength, and dev->setup_can_stall before aborting.

When userspace later reads the stale GADGETFS_SETUP event, ep0_read()
transitions dev->state to STATE_DEV_SETUP, and the subsequent ep0_read()
or ep0_write() samples dev->setup_generation after it was already
incremented, so userspace queues a stale ep0 response or stalls the
newer transfer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-gadgetfs-v7-v7-0-6d05409cc61a@gmail.com?part=2