Re: [PATCH v2 2/3] clk: socfpga: agilex5: 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 IOMEM on probe failure
- [High] clk: socfpga: loss of early clocks on early provider failure
- [Medium] clk: socfpga: memory and iomem leaks on init error paths
--

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

This commit registers the SP timer clocks early from of_clk_init() because
they are probed from time_init() and cannot defer. The remaining clocks are
registered when the platform driver probes, reusing the early provider.

> diff --git a/drivers/clk/socfpga/clk-agilex5.c b/drivers/clk/socfpga/clk-agilex5.c
> index f7f0ad884f645..e439950dd6da7 100644
> --- a/drivers/clk/socfpga/clk-agilex5.c
> +++ b/drivers/clk/socfpga/clk-agilex5.c

[ ... ]

> @@ -486,35 +502,92 @@ static int agilex5_clk_register_pll(const struct agilex5_pll_clock *clks,
>  	return 0;
>  }
>  
> -static int agilex5_clkmgr_init(struct platform_device *pdev)
> +static struct stratix10_clock_data *agilex5_clk_data;
> +
> +static struct stratix10_clock_data *agilex5_clk_data_alloc(void __iomem *base)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> -	struct device *dev = &pdev->dev;
>  	struct stratix10_clock_data *clk_data;
> -	void __iomem *base;
> -	int i, num_clks;
> -
> -	base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> -
> -	num_clks = AGILEX5_NUM_CLKS;
> +	int i;
>  
> -	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
> -				num_clks), GFP_KERNEL);
> +	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, AGILEX5_NUM_CLKS),
> +			   GFP_KERNEL);
>  	if (!clk_data)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	clk_data->base = base;
> -	clk_data->clk_data.num = num_clks;
> +	clk_data->clk_data.num = AGILEX5_NUM_CLKS;
> +
> +	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
> +		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
>  
> -	for (i = 0; i < num_clks; i++)
> -		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +	return clk_data;
> +}

[ ... ]

> +static void __init agilex5_clkmgr_early_init(struct device_node *np)
> +{
> +	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 = agilex5_clk_data_alloc(base);
> +	if (!clk_data) {
> +		iounmap(base);
> +		return;
> +	}
> +
> +	agilex5_clk_register_early(clk_data);
> +
> +	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]
Does this code leak the mapped IOMEM base and the allocated clk_data on failure?
Returning directly here without calling iounmap() and kfree() will permanently
leak them.

[Severity: High]
If of_clk_add_hw_provider() fails here, agilex5_clk_data remains NULL but the
early clocks are already registered in the Common Clock Framework (CCF).

During late init, agilex5_clkmgr_init() sees agilex5_clk_data is NULL and
attempts to re-register the early clocks. CCF rejects the duplicate names,
causing the early clock indices in the new clk_data to remain ERR_PTR.
The late provider registration succeeds but without the early clocks, breaking
consumers that need them.

Could this code deregister the early clocks on this error path, or handle
the registration failure differently to prevent this?

> +
> +	agilex5_clk_data = clk_data;
> +}

[ ... ]

> @@ -526,8 +599,15 @@ static int agilex5_clkmgr_init(struct platform_device *pdev)
>  	agilex5_clk_register_gate(agilex5_gate_clks,
>  				  ARRAY_SIZE(agilex5_gate_clks), clk_data);
>  
> -	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
> -	return 0;
> +	for (i = 0; i < AGILEX5_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 == agilex5_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]
Does this error path result in a use-after-free of the IOMEM base?

If clk_data was allocated dynamically in this function and
of_clk_add_hw_provider() fails, the probe function returns an error, which
triggers devres to unmap the IOMEM base. However, the clocks were registered
into the CCF without devres management and remain globally accessible.
If they are accessed later, they will dereference the unmapped IOMEM base.

[Severity: Medium]
Additionally, if of_clk_add_hw_provider() fails, does this code leak the
dynamically allocated clk_data structure? Since agilex5_clk_data_alloc() was
changed from using devm_kzalloc() to kzalloc(), it requires manual cleanup
on failure.

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