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

[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