Re: [PATCH v4 3/4] phy: qcom: qmp-usb-legacy: Fix possible NULL-deref on early runtime suspend

[sashiko-bot@kernel.org]

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

New issues:
- [High] Asynchronous `pm_runtime_put()` combined with inverted devres cleanup order introduces a Use-After-Free of `qmp->phy` on probe error.

Pre-existing issues:
- [Low] The `dev_pm_ops` structure is assigned unconditionally to the driver's `.pm` field without the `pm_ptr()` wrapper, producing dead code when PM is disabled.
--

--- Patch [3]: [PATCH v4 3/4] phy: qcom: qmp-usb-legacy: Fix possible NULL-deref on early runtime suspend ---
commit 3b25be245d58901212b78dd8afabc38a9e9e8abe
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

phy: qcom: qmp-usb-legacy: Fix possible NULL-deref on early runtime suspend

This commit addresses a possible NULL pointer dereference in the runtime
suspend path. It takes a runtime PM usage reference with
pm_runtime_get_noresume() before enabling PM and releases it only after
the PHY is created, preventing the suspend callback from executing while
the pointer is uninitialized.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c
> index 8bf951b0490cf..8b91eec0b5eee 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c
> @@ -1277,10 +1277,16 @@ static int qmp_usb_legacy_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_node_put;
>  
> +	/*
> +	 * Enable runtime PM before creating the PHY, phy_create() only enables
> +	 * it on the PHY device if already enabled on the parent. Hold a usage
> +	 * reference so callbacks cannot run before qmp->phy is assigned.
> +	 */
> +	pm_runtime_get_noresume(dev);
>  	pm_runtime_set_active(dev);
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret)
> -		goto err_node_put;
> +		goto err_pm_put;

[ ... ]

>  	qmp->phy = devm_phy_create(dev, np, &qmp_usb_legacy_phy_ops);
>  	if (IS_ERR(qmp->phy)) {
>  		ret = PTR_ERR(qmp->phy);
>  		dev_err(dev, "failed to create PHY: %d\n", ret);
> -		goto err_node_put;
> +		goto err_pm_put;
>  	}
>  
>  	phy_set_drvdata(qmp->phy, qmp);
>  
> +	pm_runtime_put(dev);
> +

[Severity: High]
Does this asynchronous pm_runtime_put() introduce a use-after-free on the
probe error path?

If a user enables runtime PM via sysfs (setting power/control to auto) before
or during probe, this call drops the usage counter to zero and queues an
asynchronous suspend.

If devm_of_phy_provider_register() subsequently fails, probe returns an error
and devres unwinds in reverse order:

1. devm_phy_create() unwinds, freeing qmp->phy.
2. devm_pm_runtime_enable() unwinds, calling pm_runtime_disable().

If the queued asynchronous suspend executes after qmp->phy is freed but before
pm_runtime_disable() completes, qmp_usb_legacy_runtime_suspend() will
dereference the freed pointer:

drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c:qmp_usb_legacy_runtime_suspend()
    if (!qmp->phy->init_count) {

Would using pm_runtime_put_sync() here ensure any pending suspend completes
before a potential devres cleanup?

>  	of_node_put(np);
>  
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  
>  	return PTR_ERR_OR_ZERO(phy_provider);

[ ... ]

[Severity: Low]
This is a pre-existing issue, but I noticed the dev_pm_ops structure is
assigned unconditionally to the driver:

drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c:qmp_usb_legacy_driver
    .pm = &qmp_usb_legacy_pm_ops,

If CONFIG_PM is disabled, does this leave qmp_usb_legacy_pm_ops and its
referenced callbacks as dead code?

Could pm_ptr() be used here to allow the compiler to optimize away the unused
structure when PM support is not built in?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-qcom-usb-phy-fix-null-v4-0-4d2a0f06d53a@oss.qualcomm.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy