Re: [PATCH 2/2] phy: rockchip: Make sure clk_init_data is fully initialized

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

Pre-existing issues:
- [High] Incorrect validation of the `refclk` pointer results in registering a clock with 1 parent but a NULL parent name if the optional 'phyclk' is missing.
- [High] Missing `of_clk_del_provider` on the error path of `inno_hdmi_phy_probe`, leading to a use-after-free in the OF clock provider list.
--

--- Patch [2]: [PATCH 2/2] phy: rockchip: Make sure clk_init_data is fully initialized ---
commit 830443ad1b9c48211b68c5bb691a7101b0e76d09
Author: Geert Uytterhoeven <[email protected]>

phy: rockchip: Make sure clk_init_data is fully initialized

This commit zero-initializes the clk_init_data structures across several
rockchip phy drivers to ensure all mutually-exclusive members are cleared.
This prevents subtle bugs when specific initialization configurations are
enabled.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> index 1483907413faa..80aeed965c3f3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> @@ -999,7 +999,7 @@ static int inno_hdmi_phy_clk_register(struct inno_hdmi_phy *inno)
>  {
>  	struct device *dev = inno->dev;
>  	struct device_node *np = dev->of_node;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	const char *parent_name;
>  	int ret;

[Severity: High]
This isn't a bug introduced by this patch, but does the clock provider
registration here leak on the probe error path?

In inno_hdmi_phy_probe(), if devm_of_phy_provider_register() fails after
the clock is registered:

inno_hdmi_phy_probe() {
    ...
    ret = inno_hdmi_phy_clk_register(inno);
        // Calls of_clk_add_provider() which adds to global list
    ...
    phy_provider = devm_of_phy_provider_register(...);
    return PTR_ERR_OR_ZERO(phy_provider);
}

If the phy provider registration fails, the probe aborts without calling
of_clk_del_provider(). Devres will free inno->phyclk, leaving a dangling
pointer in the global of_clk_providers list which could trigger a
use-after-free during subsequent clock lookups.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 7d8a533f24aea..e47b89015586f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -400,7 +400,7 @@ static int
>  rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>  {
>  	struct device_node *node = rphy->dev->of_node;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct clk *refclk = NULL;
>  	const char *clk_name;
>  	int i;

[Severity: High]
This is a pre-existing issue, but does this function correctly handle
missing optional clocks later in the logic?

If the optional phyclk is not found, refclk remains NULL. However, the
validation check further down evaluates NULL as true:

rockchip_usb2phy_clk480m_register() {
    ...
    if (!IS_ERR(refclk)) {
        clk_name = __clk_get_name(refclk);
        init.parent_names = &clk_name;
        init.num_parents = 1;
    } else {
        ...
    }
}

Since !IS_ERR(NULL) is true, it attempts to register the clock with 1 parent
and a NULL parent name, causing clk_register() to fail with -EINVAL and
breaking USB PHY support on platforms missing this clock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy