Re: [PATCH v2 1/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free (UAF) of `dphy` when the PHY provider is unbound but a consumer is still active.
- [Medium] The runtime PM fix is incomplete as it misses the identical leak in the companion RX driver.
- [Medium] Resource leak of `txesc_clk` on the error path of `stf_dphy_init()`.
- [Medium] Incomplete hardware teardown and clock leak in `stf_dphy_exit()` upon reset failure.
--

--- Patch [1]: [PATCH v2 1/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe ---
commit a6c202e493e39186dc1b4cf599b709a5e76356a2
Author: Can Peng <pengcan@kylinos.cn>
Subject: phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

This commit changes pm_runtime_enable() to devm_pm_runtime_enable() in the
JH7110 DPHY TX driver's probe function. This ensures that runtime PM is
properly disabled on probe failures or driver unbind.

> diff --git a/drivers/phy/starfive/phy-jh7110-dphy-tx.c b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> index c64d1c91b1307..181491a938079 100644
> --- a/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> @@ -392,6 +392,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
>  	struct stf_dphy *dphy;
> +	int ret;
>  
>  	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but is it safe to tie the lifetime of dphy to
the provider device using devm_kzalloc? 

If the provider is unbound via sysfs, devres will automatically free dphy.
However, a consumer might still be bound due to a stateless device link, and
if it calls phy_power_off(), it triggers stf_dphy_power_off() which could
dereference the freed dphy->dev:

stf_dphy_power_off() {
    ...
    struct stf_dphy *dphy = phy_get_drvdata(phy);

    return pm_runtime_put_sync(dphy->dev);
}

>  	if (!dphy)
>  		return -ENOMEM;

[ ... ]

> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);

[Severity: Medium]
This isn't a bug introduced by this patch, but does the companion RX driver
in drivers/phy/starfive/phy-jh7110-dphy-rx.c also need this same fix? 

It appears to have the exact same pattern where pm_runtime_enable() is not
managed by devres and is not reverted on a subsequent provider registration
error or driver unbind:

stf_dphy_probe() {
    ...
    pm_runtime_enable(&pdev->dev);
    
    phy_set_drvdata(dphy->phy, dphy);
    phy_provider = devm_of_phy_provider_register(&pdev->dev,
                                                 of_phy_simple_xlate);

    return PTR_ERR_OR_ZERO(phy_provider);
}

> +	if (ret)
> +		return ret;
>  
>  	dphy->txesc_clk = devm_clk_get(&pdev->dev, "txesc");

[Severity: Medium]
This is a pre-existing issue, but does the error path in stf_dphy_init() leak
the txesc_clk clock prepare and enable counts? 

If reset_control_deassert() fails, it returns without unwinding the previous
clk_prepare_enable():

stf_dphy_init() {
    ...
    ret = reset_control_deassert(dphy->sys_rst);
    if (ret) {
        dev_err(dphy->dev, "Failed to deassert sys_rst\n");
        return ret;
    }
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does stf_dphy_exit() abandon the remaining
hardware teardown sequence if reset_control_assert() fails? 

The early return skips calling clk_disable_unprepare() and
stf_dphy_hw_reset():

stf_dphy_exit() {
    ...
    ret = reset_control_assert(dphy->sys_rst);
    if (ret) {
        dev_err(dphy->dev, "Failed to assert sys_rst\n");
        return ret;
    }

    clk_disable_unprepare(dphy->txesc_clk);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718090054.444513-1-pengcan@kylinos.cn?part=1

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