Re: [PATCH v2 5/7] cpufreq: Add RISC-V RPMI cpufreq driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] cpufreq: data race on global driver_data during concurrent probe causes NULL dereference
- [Medium] cpufreq: uninitialized stack memory read of args.args[0] on malformed DT
--

commit d5239b6ffbfd0e1e9bde3c75990dec71b8fdd74f
Author: Joshua Yeong <joshua.yeong@starfivetech.com>

cpufreq: Add RISC-V RPMI cpufreq driver

This patch adds a cpufreq driver for RISC-V platforms utilizing the RPMI
performance service group. The driver serves as a front-end over the RPMI
core to manage frequency switching and registers an energy model based on
the power cost of each level.

> diff --git a/drivers/cpufreq/riscv-rpmi-cpufreq.c b/drivers/cpufreq/riscv-rpmi-cpufreq.c
> new file mode 100644
> index 0000000000000..1a5d0248f879e
> --- /dev/null
> +++ b/drivers/cpufreq/riscv-rpmi-cpufreq.c
> @@ -0,0 +1,294 @@
[ ... ]
> +static int rpmi_perf_init(struct cpufreq_policy *policy)
> +{
> +	struct cpufreq_frequency_table *freq_table;
> +	struct rpmi_perf_cpufreq_data *data;
> +	struct rpmi_perf_domain *domain;
> +	struct platform_device *pdev = cpufreq_get_driver_data();
> +	struct rpmi_perf **mpxy_perf = dev_get_platdata(&pdev->dev);
> +	struct of_phandle_args args;
> +	int ret, nr_opp;
> +	struct device *cpu_dev;
> +
> +	cpu_dev = get_cpu_device(policy->cpu);
> +	if (!cpu_dev) {
> +		pr_err("failed to get cpu%d device\n", policy->cpu);
> +		return -ENODEV;
> +	}
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = of_perf_domain_get_sharing_cpumask(policy->cpu,
> +						 "performance-domains",
> +						 "#performance-domain-cells",
> +						 policy->cpus, &args);
> +	if (ret) {
> +		dev_err(cpu_dev, "%s: failed to get performance domain info: %d\n",
> +			__func__, ret);
> +		goto out_free_priv;
> +	}
> +
> +	/* A domain ID only means something to the provider the CPU names. */
> +	if (args.np != dev_of_node(pdev->dev.parent)) {
> +		dev_err(cpu_dev, "performance domain of %pOF, not of %pOF\n",
> +			args.np, dev_of_node(pdev->dev.parent));
> +		of_node_put(args.np);
> +		ret = -ENODEV;
> +		goto out_free_priv;
> +	}
> +
> +	domain = rpmi_perf_domain_by_id(*mpxy_perf, args.args[0]);

[Severity: Medium]
Does this read uninitialized stack memory if the device tree is malformed or
specifies #performance-domain-cells = <0>?

If of_perf_domain_get_sharing_cpumask() resolves the phandle but
args.args_count is 0, the args.args array will be left uninitialized since
struct of_phandle_args args was declared on the stack.

Unconditionally consuming args.args[0] could leak uninitialized kernel stack
memory into the domain ID lookup, potentially logging garbage values on
failure and causing erratic domain resolution.

> +	of_node_put(args.np);
> +	if (!domain) {
> +		dev_err(cpu_dev, "performance domain %u is not usable\n",
> +			args.args[0]);
> +		ret = -EINVAL;
> +		goto out_free_priv;
> +	}
[ ... ]
> +static int rpmi_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct rpmi_perf **mpxy_perf = dev_get_platdata(&pdev->dev);
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	if (!mpxy_perf || !*mpxy_perf)
> +		return -EINVAL;
> +
> +	/*
> +	 * There is one cpufreq driver for the whole system, so only one
> +	 * provider can drive the CPUs. Refuse another one rather than point
> +	 * the registered driver at its domains.
> +	 */
> +	if (rpmi_perf_cpufreq_driver.driver_data)
> +		return dev_err_probe(dev, -EBUSY,
> +				     "CPUs are already driven by another provider\n");
> +
> +	rpmi_perf_cpufreq_driver.driver_data = pdev;
> +
> +	ret = cpufreq_register_driver(&rpmi_perf_cpufreq_driver);
> +	if (ret) {
> +		rpmi_perf_cpufreq_driver.driver_data = NULL;
> +		return dev_err_probe(dev, ret, "registering cpufreq failed\n");
> +	}

[Severity: High]
Is this check-and-set sequence protected against concurrent probing?

If multiple riscv-rpmi-cpufreq platform devices probe concurrently (such as
from asynchronous probing or manual sysfs binds), multiple threads might
bypass the initial if (rpmi_perf_cpufreq_driver.driver_data) check and
overwrite driver_data.

Both threads would then attempt to register the driver. The cpufreq core's
internal locks would cause the second thread to fail with -EEXIST.

The failing thread would execute this error path and set driver_data to NULL,
corrupting the state established by the successful thread.

Later, when the cpufreq core invokes rpmi_perf_init(), it calls
cpufreq_get_driver_data(), which would return NULL. The function then attempts
to access pdev->dev.parent, which could lead to a NULL pointer dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008091032.2832333-1-joshua.yeong@starfivetech.com?part=5