Re: [PATCH 7/9] cpufreq: tegra194: derive frequency domain from device tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] cpufreq: tegra194: unvalidated cluster LUT for multi-cluster policy
- [High] cpufreq: tegra194: unvalidated cpuid and clusterid in tegra194_get_speed()
--

commit 92b7184ee71717b6f0c29371b05878084c1e552d
Author: Sumit Gupta <sumitg@nvidia.com>

cpufreq: tegra194: derive frequency domain from device tree

This commit updates tegra194_cpufreq_init() to group CPUs into policies using
the device tree performance-domains property when present, allowing a single
policy to span multiple clusters and storing the domain index in clkid.

> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
> index 047166463efef..1328aa0a1bd1a 100644
> --- a/drivers/cpufreq/tegra194-cpufreq.c
> +++ b/drivers/cpufreq/tegra194-cpufreq.c
[ ... ]
> @@ -528,21 +529,45 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
>  	int maxcpus_per_cluster = data->soc->maxcpus_per_cluster;
> -	u32 clusterid = data->cpu_data[policy->cpu].clusterid;
>  	struct cpufreq_frequency_table *freq_table;
>  	struct cpufreq_frequency_table *bpmp_lut;
> -	u32 start_cpu, cpu;
> +	struct of_phandle_args args;
> +	u32 start_cpu, cpu, clusterid;
>  	int ret;
>  
> +	/* Covers all possible CPUs, so offline siblings are grouped too. */
> +	ret = of_perf_domain_get_sharing_cpumask(policy->cpu, "performance-domains",
> +						 "#performance-domain-cells",
> +						 policy->cpus, &args);
> +	if (!ret) {
> +		if (args.args_count != 1) {
> +			pr_err("cpufreq: cpu%u: invalid #performance-domain-cells\n",
> +			       policy->cpu);
> +			of_node_put(args.np);
> +			return -EINVAL;
> +		}
> +
> +		for_each_cpu(cpu, policy->cpus)
> +			data->cpu_data[cpu].clkid = args.args[0];
> +
> +		of_node_put(args.np);
> +	} else if (ret == -ENOENT) {
> +		/* Legacy grouping for DTs without performance-domains. */
> +		start_cpu = rounddown(policy->cpu, maxcpus_per_cluster);
> +		for (cpu = start_cpu; cpu < (start_cpu + maxcpus_per_cluster); cpu++)
> +			if (cpu_possible(cpu))
> +				cpumask_set_cpu(cpu, policy->cpus);
> +	} else {
> +		pr_err("cpufreq: cpu%u: failed to get performance domain: %d\n",
> +		       policy->cpu, ret);
> +		return ret;
> +	}
> +
> +	/* The BPMP LUT is indexed by the MPIDR cluster id, not clkid. */
> +	clusterid = data->cpu_data[policy->cpu].clusterid;
>  	if (clusterid >= data->soc->num_clusters || !data->bpmp_luts[clusterid])
>  		return -EINVAL;

[Severity: High]
Since policy->cpus can now span multiple clusters, what happens when
tegra194_get_speed() is called for a CPU in another cluster of the same
policy (for example via cpufreq_get(), or after the initial policy->cpu goes
offline and cpufreq_offline() migrates policy->cpu to a sibling in a second
cluster without re-running tegra194_cpufreq_init())?

tegra194_cpufreq_init() only validates clusterid and
data->bpmp_luts[clusterid] for the initial policy->cpu, but
tegra194_get_speed() indexes data->bpmp_luts[] using the queried CPU's own
clusterid:

drivers/cpufreq/tegra194-cpufreq.c:tegra194_get_speed() {
	...
	u32 clusterid = data->cpu_data[cpu].clusterid;
	...
	cpufreq_for_each_valid_entry(pos, data->bpmp_luts[clusterid]) {
}

If tegra_cpufreq_bpmp_read_lut() returned -BPMP_EINVAL for that secondary
cluster (leaving data->bpmp_luts[clusterid] as NULL), or if that CPU's
clusterid is >= data->soc->num_clusters on an SoC where get_cpu_ndiv() does
not depend on freq_core_reg (such as Tegra194 or Tegra264), can
tegra194_get_speed() dereference a NULL pointer or read out of bounds in
data->bpmp_luts[]?

[Severity: High]
Can tegra194_get_speed() also trigger an out-of-bounds MMIO read in
tegra234_sample_counters() when cpuid or clusterid is out of bounds?

In tegra_cpufreq_cpu_online(), cpuid and clusterid from MPIDR are stored in
data->cpu_data[cpu] unconditionally, and if either is out of bounds, it logs
a warning, leaves freq_core_reg as NULL, and still returns 0:

drivers/cpufreq/tegra194-cpufreq.c:tegra_cpufreq_cpu_online() {
	...
	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);
}

tegra194_cpufreq_init() does not check cpuid >= data->soc->maxcpus_per_cluster
for policy->cpu, nor does it validate clusterid or cpuid for cross-cluster or
initially offline sibling CPUs in policy->cpus (which may later come online
via tegra194_cpufreq_online() without re-running tegra194_cpufreq_init()).

When tegra194_get_speed() runs on such a CPU, it calls
tegra194_calculate_speed() before get_cpu_ndiv() checks freq_core_reg:

drivers/cpufreq/tegra194-cpufreq.c:tegra194_get_speed() {
	...
	/* reconstruct actual cpu freq using counters */
	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);
}

On Tegra234 and Tegra238, tegra194_calculate_speed()->tegra_read_counters()->
tegra234_sample_counters() computes actmon_reg directly from the unvalidated
clusterid and cpuid without checking freq_core_reg:

drivers/cpufreq/tegra194-cpufreq.c:tegra234_sample_counters() {
	...
	actmon_reg = CORE_ACTMON_CNTR_REG(data, data->cpu_data[c->cpu].clusterid,
					  data->cpu_data[c->cpu].cpuid);

	val = readq(actmon_reg);
}

Could this read past the mapped MMIO region in data->regs?

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