Re: [PATCH v2] clk: qcom: ipq-cmn-pll: Assign .num before accessing .hws

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Wrong teardown order due to mixed devm and manual cleanup creates a use-after-free race condition.
--

commit 30f995bdacca1d3339ef8960a158a04565cea6d6
Author: Aamir Ahmed <elb12345@hotmail.co.uk>
clk: qcom: ipq-cmn-pll: Assign .num before accessing .hws

hw_data->hws[] is annotated with __counted_by(num), so hw_data->num
must hold the element count before the array is accessed. This commit
moves the assignment to right after the allocation to avoid out-of-bounds
accesses during registration and cleanup.

> diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
> index dafe8c1738df8..a9abad9ff4e57 100644
> --- a/drivers/clk/qcom/ipq-cmn-pll.c
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
[ ... ]
> @@ -406,7 +408,6 @@ static int ipq_cmn_pll_register_clks(struct platform_device *pdev)
>  	 * is configured to 12 GHZ by DT property assigned-clock-rates-u64.
>  	 */
>  	hw_data->hws[CMN_PLL_CLK] = cmn_pll_hw;
> -	hw_data->num = num_clks + 1;
>  
>  	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, hw_data);

[Severity: Critical]
This is a pre-existing issue, but does mixing devm and manual cleanup here
create a use-after-free race condition during unbind?

In ipq_cmn_pll_clk_remove(), the driver manually unregisters the child
clocks:

static void ipq_cmn_pll_clk_remove(struct platform_device *pdev)
{
	...
	for (i = 0; i < hw_data->num; i++) {
		if (i != CMN_PLL_CLK)
			clk_hw_unregister(hw_data->hws[i]);
	}
}

This manual unregistration frees the underlying clk_core structures before
the devres cleanup phase begins. Because the OF clock provider was registered
using devm_of_clk_add_hw_provider() in ipq_cmn_pll_register_clks(), it
remains active until after remove() returns.

If a consumer requests a clock via clk_get() during this race window, the
active provider's of_clk_hw_onecell_get() will return a clk_hw containing a
dangling pointer to the freed clk_core, leading to a use-after-free when
the clock framework accesses it.

>  	if (ret)
>  		goto unregister_fixed_clk;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/AS8P251MB000187BDFAD98A21F4C356E9C8BD2@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM?part=1