Re: [PATCH] cpufreq: apple-soc: Fix possible null pointer dereference

[Marc Zyngier <maz@kernel.org>]

On Sat, 12 Apr 2025 17:05:18 +0100,
Chenyuan Yang <chenyuan0y@gmail.com> wrote:
> 
> Check if policy is NULL before dereferencing it.
> 
> This is similar to the commit cf7de25878a1
> ("cppc_cpufreq: Fix possible null pointer dereference").
>

No, it's not similar. The patch you refer to actually introduces bugs
by returning -ENODEV in functions that have an unsigned return type.

> This is found by our static analysis tool KNighter.

I'm surprised that your tool hasn't found the above, because it should
be a pretty easy thing to do.

Irrespective of this, it would be good to describe under which
circumstances this can occur, because I can't see *how* this can
trigger. The policy is directly provided by the core code and provide
its association with a cpu, and is never NULL at the point of init.

And if it can trigger, why only fix this one particular case?
Dereferences of policy are all over the map, and would be just as
wrong.

> 
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> Fixes: 6286bbb40576 ("cpufreq: apple-soc: Add new driver to control Apple SoC CPU P-states")
> ---
>  drivers/cpufreq/apple-soc-cpufreq.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/apple-soc-cpufreq.c b/drivers/cpufreq/apple-soc-cpufreq.c
> index 4994c86feb57..3de9bb2b0f22 100644
> --- a/drivers/cpufreq/apple-soc-cpufreq.c
> +++ b/drivers/cpufreq/apple-soc-cpufreq.c
> @@ -135,10 +135,14 @@ static const struct of_device_id apple_soc_cpufreq_of_match[] __maybe_unused = {
>  static unsigned int apple_soc_cpufreq_get_rate(unsigned int cpu)
>  {
>  	struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
> -	struct apple_cpu_priv *priv = policy->driver_data;
> +	struct apple_cpu_priv *priv;
>  	struct cpufreq_frequency_table *p;
>  	unsigned int pstate;
>  
> +	if (!policy)
> +		return 0;
> +	priv = policy->driver_data;
> +
>  	if (priv->info->cur_pstate_mask) {
>  		u32 reg = readl_relaxed(priv->reg_base + APPLE_DVFS_STATUS);
>  

So while this is not wrong, I don't think this serves any real
purpose.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.