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

[Chenyuan Yang]

On Sun, Apr 13, 2025 at 5:02 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sat, 12 Apr 2025 17:05:18 +0100,
> Chenyuan Yang <chenyuan0y@xxxxxxxxx> 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.

Our tool currently identifies bug patterns by analyzing patches. For
example, in the similar function cppc_cpufreq_get_rate(),
a patch was applied to add a null check for the policy. Therefore, we
assume that a similar check should be implemented here

> 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.

It appears that similar checks are implemented in other areas—such as
in acpi-cpufreq.c, cppc_cpufreq.c, drivers/cpufreq/cpufreq_ondemand.c,
and drivers/cpufreq/cpufreq.c.
However, I'm not sure if apple_soc should adopt the same checking style.

> >
> > Signed-off-by: Chenyuan Yang <chenyuan0y@xxxxxxxxx>
> > 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.
>
> --

Thanks so much for your reply and comments!

-Chenyuan