Re: [PATCH v4 13/16] ARM: dts: microchip: sama7g5: Add OTPC clocks

[sashiko-bot@kernel.org]

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

commit 449771270ea59bc4c7759cd22fba4a55741f936a
Author: Alexander Dahl <ada@thorsis.com>

ARM: dts: microchip: sama7g5: Add OTPC clocks

This patch adds the main RC oscillator and OTPC peripheral clocks to the
otpc node in the SAMA7G5 device tree.

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

> diff --git a/arch/arm/boot/dts/microchip/sama7g5.dtsi b/arch/arm/boot/dts/microchip/sama7g5.dtsi
> index b8296391fc696..30193f3bf9775 100644
> --- a/arch/arm/boot/dts/microchip/sama7g5.dtsi
> +++ b/arch/arm/boot/dts/microchip/sama7g5.dtsi
> @@ -1036,6 +1036,7 @@ otpc: efuse@e8c00000 {
>  			reg = <0xe8c00000 0x100>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> +			clocks = <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>, <&pmc PMC_TYPE_PERIPHERAL 67>;

[Severity: High]
Can mchp_otpc_probe() acquire SAMA7G5_PMC_MAIN_RC here, or does this cause
the OTPC probe to fail with -EINVAL due to an out-of-bounds core clock
index in sama7g5_pmc_setup()?

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(), sama7g5_pmc is allocated
with ncore set to SAMA7G5_PMC_MCK1 + 1 (14):

    sama7g5_pmc = pmc_data_allocate(SAMA7G5_PMC_MCK1 + 1,
                                    nck(sama7g5_systemck),
                                    nck(sama7g5_periphck),
                                    nck(sama7g5_gck), 8);

Because chws only has 14 entries (indices 0..13), writing to index
SAMA7G5_PMC_MAIN_RC (14) in sama7g5_pmc_setup() writes one element past the
end of sama7g5_pmc->chws[] into sama7g5_pmc->shws[0]:

    sama7g5_pmc->chws[SAMA7G5_PMC_MAIN_RC] = hw;

Then, when mchp_otpc_probe() calls devm_clk_bulk_get_all_enabled(),
drivers/clk/at91/pmc.c:of_clk_hw_pmc_get() checks:

    case PMC_TYPE_CORE:
        if (idx < pmc_data->ncore)
            return pmc_data->chws[idx];
        break;

Since idx is 14 and pmc_data->ncore is 14, of_clk_hw_pmc_get() returns
ERR_PTR(-EINVAL), failing the OTPC probe on SAMA7G5.

[Severity: High]
Also in drivers/clk/at91/sama7g5.c:sama7g5_pmc_setup() (and similarly in
drivers/clk/at91/sama7d65.c:sama7d65_pmc_setup()), when registering the
main RC oscillator:

    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;

Doesn't this store the uninitialized local pointer hw instead of main_rc_hw
into chws[SAMA7G5_PMC_MAIN_RC], returning a garbage clk_hw pointer when the
OTPC node requests <&pmc PMC_TYPE_CORE SAMA7G5_PMC_MAIN_RC>?

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