Re: [PATCH v4 07/16] dt-bindings: nvmem: microchip-otpc: Add required clocks

[sashiko-bot@kernel.org]

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

commit 5100b946d046b686eef531d6cff261bfa6eb8aee
Author: Alexander Dahl <ada@thorsis.com>

dt-bindings: nvmem: microchip-otpc: Add required clocks

This commit updates the Microchip SAMA7G5 OTPC device tree binding to
require the main RC oscillator and the OTPC peripheral clock, and updates
the binding example to reference SAMA7G5_PMC_MAIN_RC and peripheral clock
67.

Link: https://lore.kernel.org/linux-clk/ec34efc2-2051-4b8a-b5d8-6e2fd5e08c28@microchip.com/T/#u

> diff --git a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
> index cc25f2927682e..bef0246a79f16 100644
> --- a/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
> @@ -27,14 +27,22 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    items:
> +      - description: main rc oscillator
> +      - description: otpc peripheral clock
> +
>  required:
>    - compatible
>    - reg
> +  - clocks
>  
>  unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/clock/microchip,sama7g5-pmc.h>
>      #include <dt-bindings/nvmem/microchip,sama7g5-otpc.h>
>  
>      otpc: efuse@e8c00000 {
> @@ -42,6 +50,7 @@ examples:
>          reg = <0xe8c00000 0xec>;
>          #address-cells = <1>;
>          #size-cells = <1>;
> +        clocks = <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 67>;

[Severity: High]
Does looking up SAMA7G5_PMC_MAIN_RC fail at runtime and overflow
sama7g5_pmc->chws[] during PMC initialization?

In include/dt-bindings/clock/microchip,sama7g5-pmc.h, SAMA7G5_PMC_MCK1 is 13
and SAMA7G5_PMC_MAIN_RC is 14. However, in drivers/clk/at91/sama7g5.c,
sama7g5_pmc_setup() allocates the PMC data with ncore set to
SAMA7G5_PMC_MCK1 + 1 (14) before writing to index SAMA7G5_PMC_MAIN_RC (14):

drivers/clk/at91/sama7g5.c:sama7g5_pmc_setup() {
	...
	sama7g5_pmc = pmc_data_allocate(SAMA7G5_PMC_MCK1 + 1,
					nck(sama7g5_systemck),
					nck(sama7g5_periphck),
					nck(sama7g5_gck), 8);
	...
	sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] = hw;
}

Because pmc_data_allocate() sets pmc_data->ncore to 14 and points
pmc_data->shws to pmc_data->chws + ncore, writing to
sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] writes one element past the end of
chws[] into shws[0].

Furthermore, when the OTPC consumer requests <&pmc PMC_TYPE_CORE
SAMA7G5_PMC_MAIN_RC>, of_clk_hw_pmc_get() in drivers/clk/at91/pmc.c checks
idx < pmc_data->ncore (14 < 14), which fails and returns ERR_PTR(-EINVAL).

Should sama7g5_pmc_setup() pass SAMA7G5_PMC_MAIN_RC + 1 (or PMC_INDEX_MAX) to
pmc_data_allocate() instead?

[Severity: High]
Also in the earlier patch in this series exporting main_rc_osc, could
sama7g5_pmc_setup() and sama7d65_pmc_setup() be storing an uninitialized hw
pointer into chws[] instead of main_rc_hw?

In both drivers/clk/at91/sama7g5.c:sama7g5_pmc_setup() and
drivers/clk/at91/sama7d65.c:sama7d65_pmc_setup(), the local variable hw is
uninitialized when at91_clk_register_main_rc_osc() is called:

	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;
	sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] = hw;

and similarly in sama7d65_pmc_setup():

	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;

Since hw is only assigned later when registering mainck, this stores an
uninitialized stack pointer into chws[SAMA7G5_PMC_MAIN_RC] and
chws[SAMA7D65_PMC_MAIN_RC]. Should both assignments use main_rc_hw instead
of hw?

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