[PATCH v2] usb: gadget: udc: Fix use-after-free in gadget_match_driver

[PATCH v2] usb: gadget: udc: Fix use-after-free in gadget_match_driver

[Jimmy Hu]

The udc structure acts as the management structure for the gadget,
but their lifecycles are decoupled. A race condition exists where
usb_del_gadget() frees the udc memory (e.g., via mode-switch work)
while gadget_match_driver() concurrently accesses the freed udc memory
(e.g., via configfs), causing a Use-After-Free (UAF) that triggers a
NULL pointer dereference when the freed memory is zeroed:

[39430.908615][ T1171] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[39430.911397][ T1171] pc : __pi_strcmp+0x20/0x140
[39430.911441][ T1171] lr : gadget_match_driver+0x34/0x60
...
[39430.911890][ T1171]  usb_gadget_register_driver_owner+0x50/0xf8
[39430.911910][ T1171]  gadget_dev_desc_UDC_store+0xf4/0x140
[39430.931308][ T1171]  configfs_write_iter+0xec/0x134

[39430.957058][ T1171] Workqueue: events_freezable __dwc3_set_mode
[39430.957287][ T1171]  dwc3_gadget_exit+0x34/0x8c
[39430.957304][ T1171]  __dwc3_set_mode+0xc0/0x664

Fix this by ensuring the udc structure remains allocated during the
match. To achieve this, introduce a new usb_gadget_release() routine
to the core. When the gadget is added, usb_add_gadget() stores the
gadget's release routine in the udc structure and takes a reference
to the udc. When the gadget is released, usb_gadget_release() drops
the reference to the udc and then calls the gadget's release routine.

Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Cc: <stable@vger.kernel.org>
Signed-off-by: Jimmy Hu <hhhuuu@google.com>
---
V1 -> V2: Rework the fix using a new release routine in the core.

v1: https://lore.kernel.org/all/20260526070635.839701-1-hhhuuu@google.com/

 drivers/usb/gadget/udc/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 60340ff9edbf..f8ce8694c101 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -31,8 +31,9 @@ static const struct bus_type gadget_bus_type;
 /**
  * struct usb_udc - describes one usb device controller
  * @driver: the gadget driver pointer. For use by the class code
- * @dev: the child device to the actual controller
  * @gadget: the gadget. For use by the class code
+ * @gadget_release: the gadget's release routine
+ * @dev: the child device to the actual controller
  * @list: for use by the udc class driver
  * @vbus: for udcs who care about vbus status, this value is real vbus status;
  * for udcs who do not care about vbus status, this value is always true
@@ -53,6 +54,7 @@ static const struct bus_type gadget_bus_type;
 struct usb_udc {
 	struct usb_gadget_driver	*driver;
 	struct usb_gadget		*gadget;
+	void					(*gadget_release)(struct device *dev);
 	struct device			dev;
 	struct list_head		list;
 	bool				vbus;
@@ -1362,6 +1364,18 @@ static void usb_udc_nop_release(struct device *dev)
 	dev_vdbg(dev, "%s\n", __func__);
 }
 
+static void usb_gadget_release(struct device *dev)
+{
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	struct usb_udc *udc = gadget->udc;
+	/* Cache the gadget's release routine to prevent UAF */
+	void (*release)(struct device *dev) = udc->gadget_release;
+
+	put_device(&udc->dev);
+	if (release)
+		release(dev);
+}
+
 /**
  * usb_initialize_gadget - initialize a gadget and its embedded struct device
  * @parent: the parent device to this udc. Usually the controller driver's
@@ -1418,6 +1432,9 @@ int usb_add_gadget(struct usb_gadget *gadget)
 	mutex_init(&udc->connect_lock);
 
 	udc->started = false;
+	udc->gadget_release = gadget->dev.release;
+	gadget->dev.release = usb_gadget_release;
+	get_device(&udc->dev);
 
 	mutex_lock(&udc_lock);
 	list_add_tail(&udc->list, &udc_list);
@@ -1462,6 +1479,8 @@ int usb_add_gadget(struct usb_gadget *gadget)
 	mutex_lock(&udc_lock);
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
+	gadget->dev.release = udc->gadget_release;
+	put_device(&udc->dev);
 
  err_put_udc:
 	put_device(&udc->dev);

base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2] usb: gadget: udc: Fix use-after-free in gadget_match_driver

[Alan Stern]

On Wed, Jun 24, 2026 at 11:01:54AM +0800, Jimmy Hu wrote:
> The udc structure acts as the management structure for the gadget,
> but their lifecycles are decoupled. A race condition exists where
> usb_del_gadget() frees the udc memory (e.g., via mode-switch work)
> while gadget_match_driver() concurrently accesses the freed udc memory
> (e.g., via configfs), causing a Use-After-Free (UAF) that triggers a
> NULL pointer dereference when the freed memory is zeroed:
> 
> [39430.908615][ T1171] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [39430.911397][ T1171] pc : __pi_strcmp+0x20/0x140
> [39430.911441][ T1171] lr : gadget_match_driver+0x34/0x60
> ...
> [39430.911890][ T1171]  usb_gadget_register_driver_owner+0x50/0xf8
> [39430.911910][ T1171]  gadget_dev_desc_UDC_store+0xf4/0x140
> [39430.931308][ T1171]  configfs_write_iter+0xec/0x134
> 
> [39430.957058][ T1171] Workqueue: events_freezable __dwc3_set_mode
> [39430.957287][ T1171]  dwc3_gadget_exit+0x34/0x8c
> [39430.957304][ T1171]  __dwc3_set_mode+0xc0/0x664
> 
> Fix this by ensuring the udc structure remains allocated during the
> match. To achieve this, introduce a new usb_gadget_release() routine
> to the core. When the gadget is added, usb_add_gadget() stores the
> gadget's release routine in the udc structure and takes a reference
> to the udc. When the gadget is released, usb_gadget_release() drops
> the reference to the udc and then calls the gadget's release routine.
> 
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jimmy Hu <hhhuuu@google.com>
> ---

This is basically right, but there are a few small issues noted below...

> V1 -> V2: Rework the fix using a new release routine in the core.
> 
> v1: https://lore.kernel.org/all/20260526070635.839701-1-hhhuuu@google.com/
> 
>  drivers/usb/gadget/udc/core.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 60340ff9edbf..f8ce8694c101 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -31,8 +31,9 @@ static const struct bus_type gadget_bus_type;
>  /**
>   * struct usb_udc - describes one usb device controller
>   * @driver: the gadget driver pointer. For use by the class code
> - * @dev: the child device to the actual controller
>   * @gadget: the gadget. For use by the class code
> + * @gadget_release: the gadget's release routine
> + * @dev: the child device to the actual controller
>   * @list: for use by the udc class driver
>   * @vbus: for udcs who care about vbus status, this value is real vbus status;
>   * for udcs who do not care about vbus status, this value is always true
> @@ -53,6 +54,7 @@ static const struct bus_type gadget_bus_type;
>  struct usb_udc {
>  	struct usb_gadget_driver	*driver;
>  	struct usb_gadget		*gadget;
> +	void					(*gadget_release)(struct device *dev);

What happened to the column alignment here?

>  	struct device			dev;
>  	struct list_head		list;
>  	bool				vbus;
> @@ -1362,6 +1364,18 @@ static void usb_udc_nop_release(struct device *dev)
>  	dev_vdbg(dev, "%s\n", __func__);
>  }
>  
> +static void usb_gadget_release(struct device *dev)
> +{
> +	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
> +	struct usb_udc *udc = gadget->udc;
> +	/* Cache the gadget's release routine to prevent UAF */
> +	void (*release)(struct device *dev) = udc->gadget_release;
> +
> +	put_device(&udc->dev);
> +	if (release)
> +		release(dev);

I don't think the test is needed.  Even if the release function pointer 
was given as NULL when usb_initialize_gadget() was called, the value 
stored in gadget->dev.release would be usb_udc_nop_release(), not NULL.

(Come to mention it, that's a really dumb name -- it should be called 
usb_gadget_nop_release() because it's a release function for a 
usb_gadget, not for a usb_udc.)

> +}
> +
>  /**
>   * usb_initialize_gadget - initialize a gadget and its embedded struct device
>   * @parent: the parent device to this udc. Usually the controller driver's
> @@ -1418,6 +1432,9 @@ int usb_add_gadget(struct usb_gadget *gadget)
>  	mutex_init(&udc->connect_lock);
>  
>  	udc->started = false;
> +	udc->gadget_release = gadget->dev.release;
> +	gadget->dev.release = usb_gadget_release;
> +	get_device(&udc->dev);

What this is doing -- the whole scheme you are now implementing -- is 
sufficiently unconventional that it deserves a comment explaining the 
situation.  That is, saying why we need to take a reference to the udc 
and why we therefore need to override the gadget's release routine 
(i.e., to drop the udc reference).

>  
>  	mutex_lock(&udc_lock);
>  	list_add_tail(&udc->list, &udc_list);
> @@ -1462,6 +1479,8 @@ int usb_add_gadget(struct usb_gadget *gadget)
>  	mutex_lock(&udc_lock);
>  	list_del(&udc->list);
>  	mutex_unlock(&udc_lock);
> +	gadget->dev.release = udc->gadget_release;
> +	put_device(&udc->dev);

These two lines don't seem to be needed; usb_gadget_release() will take 
care of this for you when it runs.

I suppose you could argue that usb_gadget_release() might never be 
called if the gadget was statically allocated by a modular driver.  In 
that case the udc structure would be leaked.  So if you want to keep 
these lines here, that's okay -- provided you add a comment explaining 
why.

Alan Stern

>   err_put_udc:
>  	put_device(&udc->dev);
> 
> base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
> -- 
> 2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2] usb: gadget: udc: Fix use-after-free in gadget_match_driver

[Jimmy Hu (xWF)]

On Wed, Jun 24, 2026 at 10:38 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Jun 24, 2026 at 11:01:54AM +0800, Jimmy Hu wrote:
> > The udc structure acts as the management structure for the gadget,
> > but their lifecycles are decoupled. A race condition exists where
> > usb_del_gadget() frees the udc memory (e.g., via mode-switch work)
> > while gadget_match_driver() concurrently accesses the freed udc memory
> > (e.g., via configfs), causing a Use-After-Free (UAF) that triggers a
> > NULL pointer dereference when the freed memory is zeroed:
> >
> > [39430.908615][ T1171] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> > [39430.911397][ T1171] pc : __pi_strcmp+0x20/0x140
> > [39430.911441][ T1171] lr : gadget_match_driver+0x34/0x60
> > ...
> > [39430.911890][ T1171]  usb_gadget_register_driver_owner+0x50/0xf8
> > [39430.911910][ T1171]  gadget_dev_desc_UDC_store+0xf4/0x140
> > [39430.931308][ T1171]  configfs_write_iter+0xec/0x134
> >
> > [39430.957058][ T1171] Workqueue: events_freezable __dwc3_set_mode
> > [39430.957287][ T1171]  dwc3_gadget_exit+0x34/0x8c
> > [39430.957304][ T1171]  __dwc3_set_mode+0xc0/0x664
> >
> > Fix this by ensuring the udc structure remains allocated during the
> > match. To achieve this, introduce a new usb_gadget_release() routine
> > to the core. When the gadget is added, usb_add_gadget() stores the
> > gadget's release routine in the udc structure and takes a reference
> > to the udc. When the gadget is released, usb_gadget_release() drops
> > the reference to the udc and then calls the gadget's release routine.
> >
> > Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Jimmy Hu <hhhuuu@google.com>
> > ---
>
> This is basically right, but there are a few small issues noted below...
>
> > V1 -> V2: Rework the fix using a new release routine in the core.
> >
> > v1: https://lore.kernel.org/all/20260526070635.839701-1-hhhuuu@google.com/
> >
> >  drivers/usb/gadget/udc/core.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 60340ff9edbf..f8ce8694c101 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -31,8 +31,9 @@ static const struct bus_type gadget_bus_type;
> >  /**
> >   * struct usb_udc - describes one usb device controller
> >   * @driver: the gadget driver pointer. For use by the class code
> > - * @dev: the child device to the actual controller
> >   * @gadget: the gadget. For use by the class code
> > + * @gadget_release: the gadget's release routine
> > + * @dev: the child device to the actual controller
> >   * @list: for use by the udc class driver
> >   * @vbus: for udcs who care about vbus status, this value is real vbus status;
> >   * for udcs who do not care about vbus status, this value is always true
> > @@ -53,6 +54,7 @@ static const struct bus_type gadget_bus_type;
> >  struct usb_udc {
> >       struct usb_gadget_driver        *driver;
> >       struct usb_gadget               *gadget;
> > +     void                                    (*gadget_release)(struct device *dev);
>
> What happened to the column alignment here?
>
> >       struct device                   dev;
> >       struct list_head                list;
> >       bool                            vbus;
> > @@ -1362,6 +1364,18 @@ static void usb_udc_nop_release(struct device *dev)
> >       dev_vdbg(dev, "%s\n", __func__);
> >  }
> >
> > +static void usb_gadget_release(struct device *dev)
> > +{
> > +     struct usb_gadget *gadget = dev_to_usb_gadget(dev);
> > +     struct usb_udc *udc = gadget->udc;
> > +     /* Cache the gadget's release routine to prevent UAF */
> > +     void (*release)(struct device *dev) = udc->gadget_release;
> > +
> > +     put_device(&udc->dev);
> > +     if (release)
> > +             release(dev);
>
> I don't think the test is needed.  Even if the release function pointer
> was given as NULL when usb_initialize_gadget() was called, the value
> stored in gadget->dev.release would be usb_udc_nop_release(), not NULL.
>
> (Come to mention it, that's a really dumb name -- it should be called
> usb_gadget_nop_release() because it's a release function for a
> usb_gadget, not for a usb_udc.)
>
> > +}
> > +
> >  /**
> >   * usb_initialize_gadget - initialize a gadget and its embedded struct device
> >   * @parent: the parent device to this udc. Usually the controller driver's
> > @@ -1418,6 +1432,9 @@ int usb_add_gadget(struct usb_gadget *gadget)
> >       mutex_init(&udc->connect_lock);
> >
> >       udc->started = false;
> > +     udc->gadget_release = gadget->dev.release;
> > +     gadget->dev.release = usb_gadget_release;
> > +     get_device(&udc->dev);
>
> What this is doing -- the whole scheme you are now implementing -- is
> sufficiently unconventional that it deserves a comment explaining the
> situation.  That is, saying why we need to take a reference to the udc
> and why we therefore need to override the gadget's release routine
> (i.e., to drop the udc reference).
>
> >
> >       mutex_lock(&udc_lock);
> >       list_add_tail(&udc->list, &udc_list);
> > @@ -1462,6 +1479,8 @@ int usb_add_gadget(struct usb_gadget *gadget)
> >       mutex_lock(&udc_lock);
> >       list_del(&udc->list);
> >       mutex_unlock(&udc_lock);
> > +     gadget->dev.release = udc->gadget_release;
> > +     put_device(&udc->dev);
>
> These two lines don't seem to be needed; usb_gadget_release() will take
> care of this for you when it runs.
>
> I suppose you could argue that usb_gadget_release() might never be
> called if the gadget was statically allocated by a modular driver.  In
> that case the udc structure would be leaked.  So if you want to keep
> these lines here, that's okay -- provided you add a comment explaining
> why.
>
> Alan Stern
>
> >   err_put_udc:
> >       put_device(&udc->dev);
> >
> > base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
> > --
> > 2.55.0.rc0.799.gd6f94ed593-goog

Hi Alan,

Thanks for the review and the detailed explanation.
I've sent v3 with all your feedback incorporated (column alignment,
NULL check, and expanded comments):
https://lore.kernel.org/all/20260625073705.803880-1-hhhuuu@google.com/

Regarding usb_udc_nop_release(), I completely agree it's a dumb name.
I'll send a separate clean-up patch to rename it right after this fix lands.

Thanks,
Jimmy