Re: [PATCH] usb: raw_gadget: fix use-after-free when UDC is removed

[Greg Kroah-Hartman]

On Tue, Aug 04, 2026 at 09:55:29AM +0530, Anuj Bolewar via B4 Relay wrote:
> From: Anuj Bolewar <bolewara@xxxxxxxxx>
> 
> When the UDC is removed (e.g. dummy_hcd unbind via sysfs) while the raw
> gadget fd is still open, usb_del_gadget() destroys the gadget device and
> its name. raw_gadget keeps a dangling pointer in dev->gadget, and ioctls
> dereference it after releasing dev->lock, leading to a use-after-free in
> dev_err() when usb_ep_queue() fails.
> 
> Hold a gadget reference from bind until unbind and clear dev->gadget
> under dev->lock, so ioctls either observe the gadget as unbound or keep
> the device alive through the reference.
> 
> Reported-by: syzbot+9aacea11bc70c3ddaff2@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=9aacea11bc70c3ddaff2
> Fixes: f2c2e717642c ("usb: gadget: add raw-gadget interface")
> Signed-off-by: Anuj Bolewar <bolewara@xxxxxxxxx>
> ---
> The gadget device embedded in the UDC is destroyed when the UDC is
> removed while the raw gadget fd is still open (e.g. unbinding dummy_hcd
> via sysfs). raw_gadget keeps a dangling pointer in dev->gadget and
> ioctls dereference it after dropping dev->lock, which KASAN reports as a
> slab use-after-free in raw_process_ep0_io() (dev_err with a freed device
> name).
> 
> Fix it by taking a gadget reference while dev->gadget is set and clearing
> dev->gadget under dev->lock on unbind, so ioctls either observe the
> gadget as unbound or keep the device alive through the reference.

Nice change, but did you forget to add an Assisted-by: tag?  This patch
and changelog really looks llm generated to me.


> ---
>  drivers/usb/gadget/legacy/raw_gadget.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index 4febf8dac7c..acc6e4799ee 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -303,6 +303,8 @@ static int gadget_bind(struct usb_gadget *gadget,
>  	dev->req->context = dev;
>  	dev->req->complete = gadget_ep0_complete;
>  	dev->gadget = gadget;
> +	/* Keep the gadget alive while the device holds a reference to it. */
> +	usb_get_gadget(gadget);

Comment is obviuosly not needed, AND you should rewrite this as:
	dev->gadget = usb_get_gadget(gadget);
right?

>  	gadget_for_each_ep(ep, dev->gadget) {
>  		dev->eps[i].ep = ep;
>  		dev->eps[i].addr = get_ep_addr(ep->name);
> @@ -316,6 +318,10 @@ static int gadget_bind(struct usb_gadget *gadget,
>  	ret = raw_queue_event(dev, USB_RAW_EVENT_CONNECT, 0, NULL);
>  	if (ret < 0) {
>  		dev_err(&gadget->dev, "failed to queue connect event\n");
> +		spin_lock_irqsave(&dev->lock, flags);

Please use guards() instead here and the other lock you use.

> +		dev->gadget = NULL;
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +		usb_put_gadget(gadget);
>  		set_gadget_data(gadget, NULL);
>  		return ret;
>  	}
> @@ -328,7 +334,19 @@ static int gadget_bind(struct usb_gadget *gadget,
>  static void gadget_unbind(struct usb_gadget *gadget)
>  {
>  	struct raw_dev *dev = get_gadget_data(gadget);
> +	unsigned long flags;
>  
> +	/*
> +	 * The gadget is going away (e.g. the UDC is being removed), so stop
> +	 * using it. Hold the lock to synchronize with ioctls that check
> +	 * dev->gadget and may dereference it after dropping the lock.
> +	 */
> +	spin_lock_irqsave(&dev->lock, flags);
> +	dev->state = STATE_DEV_FAILED;
> +	dev->gadget = NULL;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	/* Matches usb_get_gadget() in gadget_bind(). */
> +	usb_put_gadget(gadget);

But, how can the reference go away during the bind/unbind path?  THat
shouldn't be happening as the owner is not the driver here itself, so
the increment shouldn't be necessary, right?

thanks,

greg k-h