Re: [PATCH v2 3/3] clk: socfpga: stratix10: register the SP timer clocks early

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: socfpga: use-after-free of unmapped IO memory on probe failure
- [Medium] clk: socfpga: memory leak of clk_data on probe error
- [Medium] clk: socfpga: incomplete early init rollback causes fatal registration conflicts
--

commit 68e80ef968cd718473fa02d4989427b8a0ddd324
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

clk: socfpga: stratix10: register the SP timer clocks early

The patch registers the SP timer clocks early using CLK_OF_DECLARE_DRIVER()
to prevent probe deferral, while deferring the remaining clocks to the
platform driver probe.

> diff --git a/drivers/clk/socfpga/clk-s10.c b/drivers/clk/socfpga/clk-s10.c
> index b4bf4e2d38e16..7eae2dfa408d5 100644
> --- a/drivers/clk/socfpga/clk-s10.c
> +++ b/drivers/clk/socfpga/clk-s10.c

[ ... ]

> +static struct stratix10_clock_data *s10_clk_data_alloc(void __iomem *base)
> +{
> +	struct stratix10_clock_data *clk_data;
> +	int i;
> +
> +	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, STRATIX10_NUM_CLKS),
> +			   GFP_KERNEL);
> +	if (!clk_data)
> +		return NULL;

[ ... ]

> +static void __init s10_clkmgr_early_init(struct device_node *np)
>  {

[ ... ]

> +	s10_clk_register_early(clk_data);
>  
> -	for (i = 0; i < num_clks; i++)
> -		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {
> +		pr_err("%s: failed to add clock provider\n", __func__);
> +		return;
> +	}

[Severity: Medium]
If of_clk_add_hw_provider() fails here, does it leave the globally registered
early clocks active but without a provider?

Since s10_clk_data is left as NULL, won't the fallback probe s10_clkmgr_init()
attempt to re-register the early clocks via s10_clk_register_early()? Could
this cause a fatal name collision (-EEXIST) in the clock framework and
permanently deprive downstream consumers of the early clocks?

> -	s10_clk_register_pll(s10_pll_clks, ARRAY_SIZE(s10_pll_clks), clk_data);
> +	s10_clk_data = clk_data;
> +}
> +
> +CLK_OF_DECLARE_DRIVER(stratix10_clkmgr, "intel,stratix10-clkmgr",
> +		      s10_clkmgr_early_init);
> +
> +static int s10_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data = s10_clk_data;
> +	void __iomem *base;
> +	int i;
> +
> +	if (!clk_data) {
> +		base = devm_platform_ioremap_resource(pdev, 0);
> +		if (IS_ERR(base)) {
> +			pr_err("%s: failed to map clock registers\n", __func__);
> +			return PTR_ERR(base);
> +		}
> +
> +		clk_data = s10_clk_data_alloc(base);
> +		if (!clk_data)
> +			return -ENOMEM;
> +
> +		s10_clk_register_early(clk_data);
> +	}

[ ... ]

> -	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
> -	return 0;
> +	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +
> +	if (clk_data == s10_clk_data)
> +		return 0;
> +
> +	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> +				      &clk_data->clk_data);
>  }

[Severity: Medium]
If of_clk_add_hw_provider() fails here and propagates the error directly, does
it leak the clk_data allocation?

Since clk_data is now allocated with kzalloc() in s10_clk_data_alloc()
instead of devm_kzalloc(), bypassing explicit cleanup on this error path
appears to permanently leak the memory.

[Severity: High]
Does propagating the error from of_clk_add_hw_provider() also cause a
use-after-free on unmapped IO memory?

If the fallback probe mapped base using devm_platform_ioremap_resource(), a
probe failure will trigger devres cleanup and automatically unmap base.
However, since the clocks were registered without being tied to devres in
s10_clk_register_early(), they remain globally active and hold pointers to the
now unmapped IO memory.

Could any subsequent access by the clock framework to these clocks dereference
the unmapped memory and lead to a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790935401.git.adrian.ho.yin.ng@altera.com?part=3