Re: [PATCH v2] usb: raw-gadget: fix double-free race during UDC unbind and rebind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: raw-gadget: unlocked endpoint teardown in gadget_unbind() causes UAF
--

commit 7b3966f708938c8c77f1729ecbafe22aa581cd3b
Author: Igor Putko <igorpetindev@gmail.com>

usb: raw-gadget: fix double-free race during UDC unbind and rebind

This patch moves active endpoint disabling, request freeing, and descriptor
cleanup into gadget_unbind(), and stores the endpoint descriptor pointer in
struct raw_ep to prevent a double-free race when a UDC is unbound and rebound
to a new raw_gadget instance.

> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index 4febf8dac7ca5..a6b42679acc74 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
[ ... ]
> @@ -328,6 +331,20 @@ static int gadget_bind(struct usb_gadget *gadget,
>  static void gadget_unbind(struct usb_gadget *gadget)
>  {
>  	struct raw_dev *dev = get_gadget_data(gadget);
> +	int i;
> +
> +	if (dev) {
> +		for (i = 0; i < dev->eps_num; i++) {
> +			if (dev->eps[i].state == STATE_EP_DISABLED)
> +				continue;
> +			usb_ep_disable(dev->eps[i].ep);
> +			usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
> +			kfree(dev->eps[i].desc);
> +			dev->eps[i].desc = NULL;
> +			dev->eps[i].ep->desc = NULL;
> +			dev->eps[i].state = STATE_EP_DISABLED;
> +		}
> +	}

[Severity: Critical]
Can tearing down endpoints here without holding dev->lock or coordinating
with in-flight ioctls cause use-after-free and double-free regressions when
gadget_unbind() is called while /dev/raw-gadget is still open (for example,
via sysfs driver unbind or UDC removal)?

Specifically, it looks like several races can occur in
drivers/usb/gadget/legacy/raw_gadget.c:

1. In raw_ioctl_ep_disable(), dev->lock is dropped around usb_ep_disable()
while dev->eps[i].state is still STATE_EP_ENABLED:

    dev->eps[i].disabling = true;
    spin_unlock_irqrestore(&dev->lock, flags);

    usb_ep_disable(dev->eps[i].ep);

    spin_lock_irqsave(&dev->lock, flags);
    usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
    kfree(dev->eps[i].desc);

If gadget_unbind() runs concurrently, it does not check dev->eps[i].disabling
or take dev->lock, so both paths can free dev->eps[i].req and
dev->eps[i].desc.

2. In raw_process_ep_io(), dev->lock is dropped with ep->urb_queued = true
before calling usb_ep_queue(ep->ep, ep->req, GFP_KERNEL) and
usb_ep_dequeue(ep->ep, ep->req). If gadget_unbind() runs concurrently and
calls usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req), wouldn't
raw_process_ep_io() pass a freed ep->req to usb_ep_queue() or
usb_ep_dequeue()?

3. Because gadget_unbind() frees and clears dev->eps[i].desc before setting
dev->eps[i].state = STATE_EP_DISABLED without holding dev->lock (and
raw_ioctl_ep_enable() sets ep->state = STATE_EP_ENABLED before
ep->desc = desc), a concurrent raw_process_ep_io() or
raw_ioctl_ep_set_clear_halt_wedge() holding dev->lock can observe
STATE_EP_ENABLED while ep->desc is already freed or NULL in
usb_endpoint_dir_in(ep->desc) or usb_endpoint_xfer_isoc(dev->eps[i].desc).

4. Since gadget_unbind() resets dev->eps[i].state = STATE_EP_DISABLED while
leaving dev->state as STATE_DEV_RUNNING and dev->gadget non-NULL, can a
subsequent raw_ioctl_ep_enable() on the open file descriptor re-enable
endpoints on the unbound UDC, leaving them to be torn down later in
dev_free() after a new instance has bound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024190758.2526-1-igorpetindev@gmail.com?part=1