[PATCH] usb: phy: cancel charger work in usb_remove_phy() to avoid use-after-free

[PATCH] usb: phy: cancel charger work in usb_remove_phy() to avoid use-after-free

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

usb_charger_init() (from usb_add_phy()/usb_add_phy_dev()) initializes
usb_phy->chg_work, which is scheduled from the exported
usb_phy_set_charger_current(), usb_phy_set_charger_state() and the
charger-type detection on VBUS/charger events. Its handler
(usb_phy_notify_charger_work) dereferences the usb_phy (notifier chain,
dev, chg_state). usb_remove_phy() only removes the PHY from the lookup
list and never cancels chg_work, so a work scheduled just before a
provider unbinds can still run after the provider has freed the usb_phy
-- a use-after-free.

Cancel chg_work in usb_remove_phy(), after the PHY is off the list (so no
new work can be queued) and before the caller frees the usb_phy. It is
done outside phy_lock since cancel_work_sync() may sleep.

Fixes: a9081a008f84 ("usb: phy: Add USB charger support")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/usb/phy/phy.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 5a9b935..caae749 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -731,6 +731,15 @@ void usb_remove_phy(struct usb_phy *x)
 	if (x)
 		list_del(&x->head);
 	spin_unlock_irqrestore(&phy_lock, flags);
+
+	/*
+	 * The charger-notify work, scheduled from usb_phy_set_charger_*()
+	 * and the charger-type detection, dereferences the usb_phy. The PHY
+	 * is now off the lookup list, so no new work can be queued; drain any
+	 * work still pending before the caller frees the usb_phy.
+	 */
+	if (x)
+		cancel_work_sync(&x->chg_work);
 }
 EXPORT_SYMBOL_GPL(usb_remove_phy);
 
-- 
2.43.0

Re: [PATCH] usb: phy: cancel charger work in usb_remove_phy() to avoid use-after-free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: phy: chg_work re-queued after cancel_work_sync() in
  usb_remove_phy()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] usb: phy: use-after-free in usb_get_phy() [1]
--

commit 4a4ffa5316b9cec2015871ada70b2fe24ed654b1
Author: Hyunjung Ko <hj351016@gmail.com>

usb: phy: cancel charger work in usb_remove_phy() to avoid use-after-free

This commit calls cancel_work_sync(&x->chg_work) in usb_remove_phy() after
removing the PHY from phy_list so pending charger notification work finishes
before the usb_phy is freed.

> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> index 5a9b9353f343d..caae74988fd95 100644
> --- a/drivers/usb/phy/phy.c
> +++ b/drivers/usb/phy/phy.c
> @@ -731,6 +731,15 @@ void usb_remove_phy(struct usb_phy *x)
>  	if (x)
>  		list_del(&x->head);
>  	spin_unlock_irqrestore(&phy_lock, flags);
> +
> +	/*
> +	 * The charger-notify work, scheduled from usb_phy_set_charger_*()
> +	 * and the charger-type detection, dereferences the usb_phy. The PHY
> +	 * is now off the lookup list, so no new work can be queued; drain any
> +	 * work still pending before the caller frees the usb_phy.
> +	 */
> +	if (x)
> +		cancel_work_sync(&x->chg_work);
>  }

[Severity: Medium]
Can x->chg_work still be re-queued after cancel_work_sync(&x->chg_work)
returns?

Removing x from phy_list does not stop extcon charger notifications,
because usb_add_extcon() registers x->type_nb on x->edev via
devm_extcon_register_notifier(), and usb_phy_get_charger_type() obtains
the struct usb_phy with container_of(nb, struct usb_phy, type_nb) rather
than looking it up in phy_list:

drivers/usb/phy/phy.c:usb_phy_get_charger_type() {
	struct usb_phy *usb_phy = container_of(nb, struct usb_phy, type_nb);

	__usb_phy_get_charger_type(usb_phy);
	return NOTIFY_OK;
}

PHY drivers call usb_remove_phy() from their .remove() callback before
devres_release_all() unregisters x->type_nb (and fsl_otg_remove() calls
kfree(fsl_otg_dev) immediately after usb_remove_phy()):

CPU0                              CPU1
fsl_otg_remove()
  usb_remove_phy(&fsl_otg_dev->phy)
    list_del(&x->head)
    cancel_work_sync(&x->chg_work)
                                  usb_phy_get_charger_type()
                                    __usb_phy_get_charger_type()
                                      schedule_work(&usb_phy->chg_work)
  kfree(fsl_otg_dev)
                                  usb_phy_notify_charger_work()
                                    usb_phy->chg_state ... <-- use-after-free

Unregistering the extcon notifiers in usb_remove_phy() before calling
cancel_work_sync(&x->chg_work) would prevent extcon events from
re-scheduling the work.

[Severity: Critical]
This problem wasn't introduced by this patch, but can a consumer holding a
reference from usb_get_phy() [1] still trigger a use-after-free on
struct usb_phy when the PHY provider unbinds?

usb_get_phy() pins phy->dev with get_device(phy->dev) under phy_lock:

drivers/usb/phy/phy.c:usb_get_phy() {
	...
	phy = __usb_find_phy(&phy_list, type);
	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
		...
		goto err0;
	}

	get_device(phy->dev);
}

However, most PHY drivers allocate the structure embedding struct usb_phy
with devm_kzalloc() during probe.  When the provider driver unbinds,
usb_remove_phy() removes the PHY from phy_list and devres_release_all()
frees the devm-allocated struct usb_phy even though the consumer still
holds get_device(phy->dev).  Any later call by that consumer (such as
usb_phy_set_charger_current(), PHY callbacks, or usb_put_phy()) then
dereferences freed memory.

[1] https://sashiko.dev/#/bug/linux-7f476623-2458-498a-85ca-75ee73ec5f0c

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

Re: [PATCH] usb: phy: cancel charger work in usb_remove_phy() to avoid use-after-free

[khj]

Thanks, both are valid.

You're right that cancel_work_sync() alone doesn't fully close this: the
extcon type_nb notifier reaches the usb_phy via container_of() (not the
phy_list), so an extcon event can re-queue chg_work after the cancel, and
for providers that free the containing struct in .remove() the devm extcon
notifiers are torn down only later (at the provider's devres), after the
struct is already gone.

This patch is a partial mitigation of the chg_work window; the complete
fix is an architectural one for the usb_phy framework lifetime -- the
notifiers would need to be unregistered symmetrically before the work is
drained in usb_remove_phy(), and more fundamentally struct usb_phy is
devm-allocated yet handed to consumers via usb_get_phy()/get_device(), so
a consumer holding a reference can still use it after the provider's devres
frees it (the Critical you point to). That wants proper get/put refcounting
of the usb_phy rather than devm, which affects every USB PHY provider.

I'd rather not fold a core-framework lifetime rework into this one-line
teardown fix and risk regressing every PHY driver. Happy to take this
either way you prefer: keep this as the incremental chg_work fix and do the
notifier-unregister + refcount rework as a separate series, or drop this in
favour of that series. What would you like?

Thanks,
Hyunjung

2026년 11월 8일 (일) 오후 10:50, <sashiko-bot@kernel.org>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] usb: phy: chg_work re-queued after cancel_work_sync() in
>   usb_remove_phy()
> It also noticed 1 related pre-existing issue this patch does not
> introduce:
> - [Critical] usb: phy: use-after-free in usb_get_phy() [1]
> --
>
> commit 4a4ffa5316b9cec2015871ada70b2fe24ed654b1
> Author: Hyunjung Ko <hj351016@gmail.com>
>
> usb: phy: cancel charger work in usb_remove_phy() to avoid use-after-free
>
> This commit calls cancel_work_sync(&x->chg_work) in usb_remove_phy() after
> removing the PHY from phy_list so pending charger notification work finishes
> before the usb_phy is freed.
>
> > diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> > index 5a9b9353f343d..caae74988fd95 100644
> > --- a/drivers/usb/phy/phy.c
> > +++ b/drivers/usb/phy/phy.c
> > @@ -731,6 +731,15 @@ void usb_remove_phy(struct usb_phy *x)
> >       if (x)
> >               list_del(&x->head);
> >       spin_unlock_irqrestore(&phy_lock, flags);
> > +
> > +     /*
> > +      * The charger-notify work, scheduled from usb_phy_set_charger_*()
> > +      * and the charger-type detection, dereferences the usb_phy. The PHY
> > +      * is now off the lookup list, so no new work can be queued; drain any
> > +      * work still pending before the caller frees the usb_phy.
> > +      */
> > +     if (x)
> > +             cancel_work_sync(&x->chg_work);
> >  }
>
> [Severity: Medium]
> Can x->chg_work still be re-queued after cancel_work_sync(&x->chg_work)
> returns?
>
> Removing x from phy_list does not stop extcon charger notifications,
> because usb_add_extcon() registers x->type_nb on x->edev via
> devm_extcon_register_notifier(), and usb_phy_get_charger_type() obtains
> the struct usb_phy with container_of(nb, struct usb_phy, type_nb) rather
> than looking it up in phy_list:
>
> drivers/usb/phy/phy.c:usb_phy_get_charger_type() {
>         struct usb_phy *usb_phy = container_of(nb, struct usb_phy, type_nb);
>
>         __usb_phy_get_charger_type(usb_phy);
>         return NOTIFY_OK;
> }
>
> PHY drivers call usb_remove_phy() from their .remove() callback before
> devres_release_all() unregisters x->type_nb (and fsl_otg_remove() calls
> kfree(fsl_otg_dev) immediately after usb_remove_phy()):
>
> CPU0                              CPU1
> fsl_otg_remove()
>   usb_remove_phy(&fsl_otg_dev->phy)
>     list_del(&x->head)
>     cancel_work_sync(&x->chg_work)
>                                   usb_phy_get_charger_type()
>                                     __usb_phy_get_charger_type()
>                                       schedule_work(&usb_phy->chg_work)
>   kfree(fsl_otg_dev)
>                                   usb_phy_notify_charger_work()
>                                     usb_phy->chg_state ... <-- use-after-free
>
> Unregistering the extcon notifiers in usb_remove_phy() before calling
> cancel_work_sync(&x->chg_work) would prevent extcon events from
> re-scheduling the work.
>
> [Severity: Critical]
> This problem wasn't introduced by this patch, but can a consumer holding a
> reference from usb_get_phy() [1] still trigger a use-after-free on
> struct usb_phy when the PHY provider unbinds?
>
> usb_get_phy() pins phy->dev with get_device(phy->dev) under phy_lock:
>
> drivers/usb/phy/phy.c:usb_get_phy() {
>         ...
>         phy = __usb_find_phy(&phy_list, type);
>         if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
>                 ...
>                 goto err0;
>         }
>
>         get_device(phy->dev);
> }
>
> However, most PHY drivers allocate the structure embedding struct usb_phy
> with devm_kzalloc() during probe.  When the provider driver unbinds,
> usb_remove_phy() removes the PHY from phy_list and devres_release_all()
> frees the devm-allocated struct usb_phy even though the consumer still
> holds get_device(phy->dev).  Any later call by that consumer (such as
> usb_phy_set_charger_current(), PHY callbacks, or usb_put_phy()) then
> dereferences freed memory.
>
> [1] https://sashiko.dev/#/bug/linux-7f476623-2458-498a-85ca-75ee73ec5f0c
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261108134001.536207-1-hj351016@gmail.com?part=1