Re: [PATCH v3 4/4] phy: cpcap-usb: add extcon support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Missing reset of extcon state on driver removal, leading to stale connection states for consumers.

Pre-existing issues:
- [High] Pre-existing UAF race condition during device removal between the IRQ thread and remove() leading to delayed work executing after device data is freed.
--

commit a6baa41684450d481944fb6077e8db95e4f80e2e
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

phy: cpcap-usb: add extcon support

Register an Extcon device and report the detected cable state. The driver
already determines the type of cable attached during USB cable detection.
Export the detected state through the Extcon framework so other drivers can
consume it using a standard kernel interface.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index 78bb1773e5379..ba36d110df419 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c

[ ... ]

> @@ -777,7 +835,18 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
>  	if (!otg)
>  		return -ENOMEM;
>  
> +	ddata->edev = devm_extcon_dev_allocate(&pdev->dev, cpcap_extcon_cables);
> +	if (IS_ERR(ddata->edev)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(ddata->edev),
> +				     "failed to allocate extcon device\n");
> +	}
> +
> +	error = devm_extcon_dev_register(&pdev->dev, ddata->edev);
> +	if (error < 0)
> +		return error;
> +

[Severity: Medium]
When the module is removed or unbound, devres will automatically unregister
the extcon device. However, since there are no corresponding updates to
cpcap_usb_phy_remove() in phy-cpcap-usb.c, the extcon cable states are not
explicitly reset to EXTCON_NONE before the device goes away.

Will consumers of this extcon device (e.g. charger drivers) retain stale
connection states because of missed disconnect notifications during removal?

>  	ddata->mode = CPCAP_UNKNOWN;
> +	ddata->cable = EXTCON_NONE;
>  	ddata->dev = &pdev->dev;
>  	ddata->phy.dev = ddata->dev;
>  	ddata->phy.label = "cpcap_usb_phy";

[Severity: High]
This isn't a bug introduced by this patch, but is there a pre-existing
use-after-free race condition during device removal between
cpcap_phy_irq_thread() and cpcap_usb_phy_remove() in phy-cpcap-usb.c?

Looking at cpcap_usb_phy_remove(), it cancels the work:

cpcap_usb_phy_remove() {
    ...
    atomic_set(&ddata->active, 0);
    cancel_delayed_work_sync(&ddata->detect_work);
    ...
}

However, if cpcap_phy_irq_thread() is preempted right after checking the
active flag:

cpcap_phy_irq_thread() {
    ...
    if (!atomic_read(&ddata->active))
        return IRQ_NONE;

    /* preempted here */
    schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));
    ...
}

Could this sequence occur during unbind/removal:

1. IRQ thread reads active == 1, gets preempted.
2. cpcap_usb_phy_remove() sets active = 0, calls cancel_delayed_work_sync()
   (which does nothing since the work isn't queued yet).
3. Devres frees the IRQ, waiting for the IRQ thread to finish.
4. IRQ thread resumes and queues the delayed work.
5. Devres frees the ddata structure.
6. The delayed work timer expires and accesses the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705101105.1798069-1-ivo.g.dimitrov.75@gmail.com?part=4