Re: [PATCH v4 3/6] soc: samsung: exynos-pmu: generalise gs101-specific cpu{idle,hotplug} for Exynos SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardware register misprogramming due to incorrect CPU index from smp_processor_id().
- [High] Use-After-Free of device node intr_gen_node.
- [High] Missing teardown of global notifiers on probe failure causes Use-After-Free.
- [Medium] Missing of_node_put() on error path causes a device node reference leak.
--

commit 00ecbc34b3961a015d2064ffdbd5f20fb602ef73
Author: Alexey Klimov <alexey.klimov@linaro.org>

soc: samsung: exynos-pmu: generalise gs101-specific cpu{idle,hotplug} for Exynos SoCs

This patch generalises cpuhotplug and cpuidle support for GS101-based SoCs
to be reused for other Exynos-based SoCs. It introduces cpu_pmu_offline and
cpu_pmu_online callbacks in exynos_pmu_data and updates gs101 platform data
to use them.

> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index d58376c38179b..660416c0db432 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
[ ... ]
> @@ -425,6 +346,11 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
>  		return 0;
>  	}
>  
> +	if (!pmu_context->pmu_data->cpu_pmu_offline || !pmu_context->pmu_data->cpu_pmu_online) {
> +		dev_err(dev, "PMU write/read sequence is not present for cpuhotplug and cpuidle\n");
> +		return -ENODEV;
> +	}
> +

Does this return path leak the intr_gen_node device node reference?
of_parse_phandle() acquires a reference to it earlier in this function,
which needs to be released via of_node_put() before returning.

[ ... ]
> @@ -458,17 +384,17 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
>  
>  	/* set PMU to power on */
>  	for_each_online_cpu(cpu)
> -		gs101_cpuhp_pmu_online(cpu);
> +		exynos_cpuhp_pmu_online(cpu);
>  
>  	/* register CPU hotplug callbacks */
>  	cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,	"soc/exynos-pmu:prepare",
> -			  gs101_cpuhp_pmu_online, NULL);
> +			  exynos_cpuhp_pmu_online, NULL);
>  
>  	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
> -			  NULL, gs101_cpuhp_pmu_offline);
> +			  NULL, exynos_cpuhp_pmu_offline);
>  
>  	/* register CPU PM notifiers for cpuidle */
> -	cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
> +	cpu_pm_register_notifier(&exynos_cpu_pm_notifier);
>  	register_reboot_notifier(&exynos_cpupm_reboot_nb);
>  	return 0;
>  }

This problem wasn't introduced by this patch, but is there a use-after-free
of intr_gen_node in setup_cpuhp_and_cpuidle()?

of_node_put(intr_gen_node) is called immediately after of_address_to_resource(),
but later the node pointer is passed to of_syscon_register_regmap():

	ret = of_address_to_resource(intr_gen_node, 0, &intrgen_res);
	of_node_put(intr_gen_node);
	...
	ret = of_syscon_register_regmap(intr_gen_node,
					pmu_context->pmuintrgen);

Also, this problem wasn't introduced by this patch, but does this function
need to provide a teardown mechanism for these global notifiers?

If exynos_pmu_probe() fails after this point, the devres-managed pmu_context
will be freed.

Since cpuhp_setup_state() and cpu_pm_register_notifier() don't use devm_
variants, subsequent CPU hotplug or idle events could invoke the dangling
callbacks and access the freed pmu_context, potentially causing a
use-after-free.

> diff --git a/drivers/soc/samsung/gs101-pmu.c b/drivers/soc/samsung/gs101-pmu.c
> index 17dadc1b9c6ec..5f2a599241447 100644
> --- a/drivers/soc/samsung/gs101-pmu.c
> +++ b/drivers/soc/samsung/gs101-pmu.c
[ ... ]
> @@ -322,11 +322,68 @@ static const struct regmap_access_table gs101_pmu_wr_table = {
>  	.n_no_ranges = ARRAY_SIZE(gs101_pmu_ro_registers),
>  };
>  
> +/*
> + * gs101_cpu_pmu_ prefix functions are common code shared by CPU PM notifiers
> + * (CPUIdle) and CPU hotplug callbacks. Functions should be called with IRQs
> + * disabled and cpupm_lock held.
> + */
> +static int gs101_cpu_pmu_online(struct exynos_pmu_context *pmu_context, unsigned int cpu)
> +	__must_hold(&pmu_context->cpupm_lock)
> +{
> +	unsigned int cpuhint = smp_processor_id();

This problem wasn't introduced by this patch, but will smp_processor_id()
return the correct CPU index during hotplug startup?

During CPU hotplug startup, exynos_cpuhp_pmu_online() is called via
CPUHP_BP_PREPARE_DYN, which executes on the Boot Processor.

As a result, smp_processor_id() returns the Boot Processor's ID instead of the
target CPU's ID, which could misconfigure the Boot Processor's register while
leaving the target CPU uninitialized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3