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

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

[Aamir Ahmed]

hw_data->hws[] is annotated with __counted_by(num), so hw_data->num
must hold the element count before the array is accessed.
ipq_cmn_pll_register_clks() assigns it only after storing the fixed
rate output clocks and the CMN PLL, and the unwind loop under
unregister_fixed_clk reads .hws[] with .num still zero. Both are
out-of-bounds accesses under CONFIG_UBSAN_BOUNDS.

Move the assignment to right after the allocation.

Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
Reviewed-by: Luo Jie <jie.luo@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
v2:
  - cut the commit message down (Bjorn)
  - collect Reviewed-by tags
v1: https://lore.kernel.org/linux-clk/AS8P251MB0001B905088748016A0965F1C8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/

Compile-tested only; I have no IPQ hardware.

 drivers/clk/qcom/ipq-cmn-pll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/ipq-cmn-pll.c b/drivers/clk/qcom/ipq-cmn-pll.c
index dafe8c1738df..a9abad9ff4e5 100644
--- a/drivers/clk/qcom/ipq-cmn-pll.c
+++ b/drivers/clk/qcom/ipq-cmn-pll.c
@@ -380,6 +380,8 @@ static int ipq_cmn_pll_register_clks(struct platform_device *pdev)
 	if (!hw_data)
 		return -ENOMEM;
 
+	hw_data->num = num_clks + 1;
+
 	/*
 	 * Register the CMN PLL clock, which is the parent clock of
 	 * the fixed rate output clocks.
@@ -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);
 	if (ret)

base-commit: df2908090cda368b01ff43709f51890076c56157
-- 
2.55.0

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

[sashiko-bot]

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