Re: [PATCH] ARM: meson: keep reset control around

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unguarded access to the fixed-size `cpu_rstc` array using the `cpu` index.
- [High] Early returns in `meson_smp_prepare_cpus` leave `cpu_rstc` uninitialized, bypassing error checks during secondary boot.
--

commit 3a78a84452dfa8eb8509282b8a25dfa2693e6e1a
Author: Philipp Zabel <p.zabel@pengutronix.de>

ARM: meson: keep reset control around

This commit retains exclusive control over the CPU reset line rather than
releasing it. This is intended to keep the reset line deasserted after turning
on a CPU and avoids calling reset_control_put before workqueues are
operational.

> diff --git a/arch/arm/mach-meson/platsmp.c b/arch/arm/mach-meson/platsmp.c
> index 32ac60b89fdcc..5f18895104ed8 100644
> --- a/arch/arm/mach-meson/platsmp.c
> +++ b/arch/arm/mach-meson/platsmp.c
> @@ -34,6 +34,7 @@
>  static void __iomem *sram_base;
>  static void __iomem *scu_base;
>  static struct regmap *pmu;
> +static struct reset_control *cpu_rstc[4];
>  
>  static struct reset_control *meson_smp_get_core_reset(int cpu)
>  {
[ ... ]
> @@ -99,6 +101,9 @@ static void __init meson_smp_prepare_cpus(const char *scu_compatible,
>  	}
>  
>  	scu_enable(scu_base);
> +
> +	for (cpu = 0; cpu < ARRAY_SIZE(cpu_rstc); cpu++)
> +		cpu_rstc[cpu] = meson_smp_get_core_reset(cpu);
>  }

[Severity: High]
Does this leave the cpu_rstc array uninitialized if an earlier step fails?

If a setup step like mapping the SCU fails earlier in this function, it logs
an error and returns early without executing this loop. The cpu_rstc array
will remain filled with NULL pointers.

When smp_boot_secondary subsequently executes, it reads the uninitialized NULL
pointer from the array. Since IS_ERR(NULL) evaluates to false, the error
check is entirely bypassed. The subsequent reset control calls will act as
silent no-ops, which could result in attempting to power on an unreset CPU
instead of gracefully failing.

[ ... ]
> @@ -155,10 +160,9 @@ static int meson_smp_finalize_secondary_boot(unsigned int cpu)
>  static int meson8_smp_boot_secondary(unsigned int cpu,
>  				     struct task_struct *idle)
>  {
> -	struct reset_control *rstc;
> +	struct reset_control *rstc = cpu_rstc[cpu];

[Severity: High]
Does this introduce a potential out-of-bounds read on the cpu_rstc array?

The cpu_rstc array is statically allocated with a size of 4, but the cpu index
is used directly here without bounds checking. If the system's device tree
specifies more than 4 logical CPU nodes, nr_cpu_ids will exceed 4, and the
boot code will pass an index that reads past the array bounds.

The invalid pointer retrieved would then be dereferenced when asserting the
reset control.

>  	int ret;
>  
> -	rstc = meson_smp_get_core_reset(cpu);
>  	if (IS_ERR(rstc)) {
>  		pr_err("Couldn't get the reset controller for CPU%d\n", cpu);
>  		return PTR_ERR(rstc);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522083548.2360352-1-p.zabel@pengutronix.de?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic