Re: [PATCH v4 06/16] clk: at91: Add peripheral clock id for OTPC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: at91: uninitialized hw assigned to chws in sama7d65/sama7g5_pmc_setup()
- [High] clk: at91: out-of-bounds chws write and lookup failure in sama7g5_pmc_setup()
--

commit 056fbff3f28231ae65a50d25dfabd2b4de1811af
Author: Alexander Dahl <ada@thorsis.com>

clk: at91: Add peripheral clock id for OTPC

This series splits up the AT91 PMC clock binding indices per SoC, exports the
main RC oscillator clock to Device Tree, and adds the OTPC peripheral clock
IDs for sam9x60, sam9x7, sama7d65, and sama7g5.

> diff --git a/drivers/clk/at91/sama7d65.c b/drivers/clk/at91/sama7d65.c
> index ba8ff413fa2c6..1f3c724739133 100644
> --- a/drivers/clk/at91/sama7d65.c
> +++ b/drivers/clk/at91/sama7d65.c

[ ... ]

> @@ -1138,6 +1139,7 @@ static void __init sama7d65_pmc_setup(struct device_node *np)
>  						   50000000);
>  	if (IS_ERR(main_rc_hw))
>  		goto err_free;
> +	sama7d65_pmc->chws[SAMA7D65_PMC_MAIN_RC] = hw;

[Severity: High]
Does this store an uninitialized pointer into
sama7d65_pmc->chws[SAMA7D65_PMC_MAIN_RC]?

In sama7d65_pmc_setup(), hw is declared without initialization and
at91_clk_register_main_rc_osc() stores its return value in main_rc_hw:

	struct clk_hw *hw, *main_rc_hw, *main_osc_hw, *main_xtal_hw;
	...
	main_rc_hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
						   50000000);
	if (IS_ERR(main_rc_hw))
		goto err_free;
	sama7d65_pmc->chws[SAMA7D65_PMC_MAIN_RC] = hw;

Should this assign main_rc_hw instead of hw?

> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
> index 1340c2b006192..3e0cfd2613851 100644
> --- a/drivers/clk/at91/sama7g5.c
> +++ b/drivers/clk/at91/sama7g5.c

[ ... ]

> @@ -997,7 +998,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>  	if (IS_ERR(regmap))
>  		return;
>  
> -	sama7g5_pmc = pmc_data_allocate(PMC_MCK1 + 1,
> +	sama7g5_pmc = pmc_data_allocate(SAMA7G5_PMC_MCK1 + 1,
>  					nck(sama7g5_systemck),
>  					nck(sama7g5_periphck),
>  					nck(sama7g5_gck), 8);
> @@ -1014,6 +1015,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>  						   50000000);
>  	if (IS_ERR(main_rc_hw))
>  		goto err_free;
> +	sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] = hw;

[Severity: High]
In sama7g5_pmc_setup(), hw is also uninitialized at this point because
at91_clk_register_main_rc_osc() assigns its result to main_rc_hw. Should this
assign main_rc_hw here as well?

[Severity: High]
Additionally, can writing to sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] overflow
sama7g5_pmc->chws[] and cause lookups of SAMA7G5_PMC_MAIN_RC to fail?

In include/dt-bindings/clock/microchip,sama7g5-pmc.h:

	#define SAMA7G5_PMC_MCK1	13
	#define SAMA7G5_PMC_MAIN_RC	14

sama7g5_pmc_setup() calls pmc_data_allocate(SAMA7G5_PMC_MCK1 + 1, ...), which
sets sama7g5_pmc->ncore to 14 and places sama7g5_pmc->shws at
sama7g5_pmc->chws + 14. Indexing sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] writes
to index 14, past the 14-element chws slice into shws[0]. Furthermore, when
of_clk_hw_pmc_get() resolves a PMC_TYPE_CORE clock with idx ==
SAMA7G5_PMC_MAIN_RC, the check idx < pmc_data->ncore (14 < 14) will be false
and return ERR_PTR(-EINVAL).

Should pmc_data_allocate() be called with SAMA7G5_PMC_MAIN_RC + 1 as ncore?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105110715.2486432-1-ada@thorsis.com?part=6