Re: [PATCH 1/9] cpufreq: tegra194: move MPIDR read to CPU hotplug callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cpufreq: tegra194: out-of-bounds MMIO and LUT read on bad MPIDR
--

commit 13d8e373c1e546a0ef275842259e4854d9497025
Author: Sumit Gupta <sumitg@nvidia.com>

cpufreq: tegra194: move MPIDR read to CPU hotplug callback

This commit moves the per-CPU MPIDR read from probe-time SMP calls to a CPU
hotplug online callback so that CPUs offline at boot do not use uninitialized
core and cluster IDs. It also validates the CPU count against the SoC data at
probe and skips populating the MMIO scratch register address when a CPU's
MPIDR exceeds the SoC bounds.

> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
> index 1eb96e9f3310d..be81ffd23edd5 100644
> --- a/drivers/cpufreq/tegra194-cpufreq.c
> +++ b/drivers/cpufreq/tegra194-cpufreq.c
[ ... ]
> @@ -121,11 +124,44 @@ static void tegra234_get_cpu_cluster_id(u32 cpu, u32 *cpuid, u32 *clusterid)
>  		*clusterid = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>  }
>  
> +static int tegra_cpufreq_cpu_online(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct tegra194_cpufreq_data *data;
> +	u32 cpuid, clusterid;
> +
> +	data = hlist_entry(node, struct tegra194_cpufreq_data, cpuhp_node);
> +
> +	data->soc->ops->get_cpu_cluster_id(cpu, &cpuid, &clusterid);
> +
> +	data->cpu_data[cpu].cpuid = cpuid;
> +	data->cpu_data[cpu].clusterid = clusterid;
> +
> +	/* Derive per-core register address only when ccplex MMIO is mapped. */
> +	if (data->regs) {
> +		if (clusterid >= data->soc->num_clusters ||
> +		    cpuid >= data->soc->maxcpus_per_cluster) {
> +			pr_warn_once("cpufreq: cpu%u: bad MPIDR (cluster %u, core %u)\n",
> +				     cpu, clusterid, cpuid);
> +		} else {
> +			u32 index = clusterid * data->soc->maxcpus_per_cluster + cpuid;
> +
> +			data->cpu_data[cpu].freq_core_reg =
> +				SCRATCH_FREQ_CORE_REG(data, index);
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
When a CPU has an out-of-bounds MPIDR (clusterid >= data->soc->num_clusters or
cpuid >= data->soc->maxcpus_per_cluster), tegra_cpufreq_cpu_online() leaves
data->cpu_data[cpu].freq_core_reg as NULL so NDIV accesses are skipped, but
still stores the out-of-bounds cpuid and clusterid into data->cpu_data[cpu]
and returns 0.

In tegra194_cpufreq_init(), only clusterid >= data->soc->num_clusters is
checked on policy->cpu:

    if (clusterid >= data->soc->num_clusters || !data->bpmp_luts[clusterid])
            return -EINVAL;

Neither cpuid >= data->soc->maxcpus_per_cluster on policy->cpu nor the MPIDR
values of sibling CPUs added to policy->cpus are checked there.

When cpufreq later queries the frequency for that CPU via
tegra194_get_speed(), it calls tegra194_calculate_speed() before checking
get_cpu_ndiv():

    rate = tegra194_calculate_speed(cpu);

    /* get last written ndiv value */
    ret = data->soc->ops->get_cpu_ndiv(cpu, data->cpu_data[cpu].cpuid, clusterid, &ndiv);
    if (WARN_ON_ONCE(ret))
            return rate;

    cpufreq_for_each_valid_entry(pos, data->bpmp_luts[clusterid]) {

On Tegra234, tegra194_calculate_speed() queues tegra_read_counters() ->
tegra234_read_counters(), which computes the counter address directly from the
stored clusterid and cpuid without checking freq_core_reg or bounds:

    actmon_reg = CORE_ACTMON_CNTR_REG(data, data->cpu_data[c->cpu].clusterid,
                                      data->cpu_data[c->cpu].cpuid);

    val = readq(actmon_reg);

And after tegra194_calculate_speed() returns, tegra234_get_cpu_ndiv() returns
-EINVAL for the NULL freq_core_reg, triggering WARN_ON_ONCE(ret) in
tegra194_get_speed().

In addition, on Tegra194 (where data->regs is NULL),
tegra_cpufreq_cpu_online() skips the bounds check altogether,
tegra194_get_cpu_ndiv() succeeds via system register read, and
tegra194_get_speed() indexes data->bpmp_luts[clusterid] without bounds
validation if a sibling CPU has clusterid >= data->soc->num_clusters.

Can this cause an out-of-bounds MMIO read in tegra234_read_counters(), a
WARN_ON_ONCE() splat, or an out-of-bounds read of data->bpmp_luts[clusterid]
in tegra194_get_speed() when a CPU reports a bad MPIDR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104181521.3003578-1-sumitg@nvidia.com?part=1