Re: [PATCH v2 1/3] clk: socfpga: agilex: 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 I/O memory on probe failure
- [Medium] clk: socfpga: memory and I/O mapping leaks on error paths
- [Medium] clk: socfpga: double registration of early clocks on provider failure
--

commit 1aa58afee8b5118fcfe81945cc09867307ba4bd8
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Subject: clk: socfpga: agilex: register the SP timer clocks early

This commit splits the clock registration for the Agilex platform into an
early initialization phase to support timers that require clocks before
core_initcall, and defers the remaining clocks to the platform driver probe.

> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> index 2bdea1997b5ef..91a80b7b1886a 100644
> --- a/drivers/clk/socfpga/clk-agilex.c
> +++ b/drivers/clk/socfpga/clk-agilex.c

[ ... ]

> +static void __init
> +agilex_clkmgr_of_init(struct device_node *np,
> +		      void (*register_early)(struct stratix10_clock_data *))
> +{
> +	struct stratix10_clock_data *clk_data;
> +	void __iomem *base;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_err("%s: failed to map clock registers\n", __func__);
> +		return;
> +	}
> +
> +	clk_data = agilex_clk_data_alloc(base);
> +	if (!clk_data) {
> +		iounmap(base);
> +		return;
> +	}
> +
> +	register_early(clk_data);
> +
> +	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {

[Severity: Medium]
If of_clk_add_hw_provider() fails, register_early() will have already
registered the clocks via the CCF APIs. Because agilex_clk_data is not set,
they become permanently inaccessible to consumers. 

When the platform driver later probes, it will attempt to register these early
clocks again because agilex_clk_data is still NULL. Can this result in
duplicate CCF name rejections, leaving the new array with -ENOENT?

> +		pr_err("%s: failed to add clock provider\n", __func__);
> +		return;

[Severity: Medium]
Does this error path leak the I/O mapping and memory? It appears of_iomap()
base and agilex_clk_data_alloc() clk_data are not cleaned up with
iounmap() and kfree() before returning.

> +	}
> +
> +	agilex_clk_data = clk_data;
> +}

[ ... ]

> +static int agilex_clkmgr_add_provider(struct platform_device *pdev,
> +				      struct stratix10_clock_data *clk_data)
> +{
> +	int i;
> +
> +	for (i = 0; i < AGILEX_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 == agilex_clk_data)
> +		return 0;
> +
> +	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> +				      &clk_data->clk_data);

[Severity: High]
If early init was skipped or failed, agilex_clkmgr_get_clk_data() maps I/O
memory using devm_platform_ioremap_resource() and registers clocks.

Unlike the original code which ignored the return value, this propagates the
error from of_clk_add_hw_provider(). Returning the error causes the driver
core's devres to unmap the memory, but the clocks remain globally registered in
CCF with raw pointers to the unmapped base.

Can this lead to a use-after-free of devm-managed I/O memory if CCF attempts
to read from the unmapped base during operations like debugfs reads?

> +}
> +
> +static int agilex_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data;
> +
> +	clk_data = agilex_clkmgr_get_clk_data(pdev, agilex_clk_register_early);
> +	if (IS_ERR(clk_data))
> +		return PTR_ERR(clk_data);
>  
>  	agilex_clk_register_c_perip(agilex_main_perip_c_clks,
>  				 ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);
> @@ -490,36 +625,17 @@ static int agilex_clkmgr_init(struct platform_device *pdev)
>  
>  	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
>  			      clk_data);
> +
> +	return agilex_clkmgr_add_provider(pdev, clk_data);

[Severity: Medium]
If agilex_clkmgr_add_provider() fails, probe returns an error. Does this leak
the clk_data allocated by kzalloc() in agilex_clk_data_alloc()?

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