| Message ID | 20250412160518.1824538-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: apple-soc: Fix possible null pointer dereference | expand |
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.
On Sun, Apr 13, 2025 at 5:02 AM Marc Zyngier <maz@kernel.org> wrote: > > 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. 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@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. > > -- Thanks so much for your reply and comments! -Chenyuan
On Sun, 13 Apr 2025 22:31:26 +0100, Chenyuan Yang <chenyuan0y@gmail.com> wrote: > > On Sun, Apr 13, 2025 at 5:02 AM Marc Zyngier <maz@kernel.org> wrote: > > > > 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. > > 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 That's not static analysis, that's just an evolved form of pseudo-AI driven copy/paste patching. In other words, the worst sort of tool. > > > 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. I don't think adding more crap without proper justification is the way to go. If this value can be NULL, you should be able to demonstrate an execution that leads to this behaviour. That's what an analysis tool would perform. M.
On 13-04-25, 11:02, Marc Zyngier wrote: > 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. > > So while this is not wrong, I don't think this serves any real > purpose. I have applied such patches in the past, considering the same as good practice. But I do understand your inputs. And so I tried to see if there is actually a way to trigger this. - Platform with two cpufreq policies (freq domains) with one CPU in each of them. - Boot the kernel, policies will initialize for both the domains. - Hotplug out CPU1, that will remove the policy as well. - Call cpufreq_quick_get(1), this will call the ->get() callback for CPU1, for which there is no policy available. But this is the case only for drivers with `setpolicy` callback, this shouldn't happen on apple-soc. I am not sure now if we should just apply this patch to be safe, or leave it as is. The cpufreq core may change in the future and call the `get` callback for all drivers. Rafael, what do you suggest ?
On Wed, 16 Apr 2025 08:34:20 +0100, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 13-04-25, 11:02, Marc Zyngier wrote: > > 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. > > > > So while this is not wrong, I don't think this serves any real > > purpose. > > I have applied such patches in the past, considering the same as good > practice. But I do understand your inputs. > > And so I tried to see if there is actually a way to trigger this. > > - Platform with two cpufreq policies (freq domains) with one CPU in > each of them. > - Boot the kernel, policies will initialize for both the domains. > - Hotplug out CPU1, that will remove the policy as well. > - Call cpufreq_quick_get(1), this will call the ->get() callback for > CPU1, for which there is no policy available. > > But this is the case only for drivers with `setpolicy` callback, this > shouldn't happen on apple-soc. > > I am not sure now if we should just apply this patch to be safe, or > leave it as is. The cpufreq core may change in the future and call the > `get` callback for all drivers. If that's the case, I'd suggest adding a __must_check annotation to cpufreq_cpu_get_raw() and co. At least we'll get a warning on all missing uses, fix them in one go, and avoid the constant churn of more or less correct patches. Thanks, M.
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);
Check if policy is NULL before dereferencing it. This is similar to the commit cf7de25878a1 ("cppc_cpufreq: Fix possible null pointer dereference"). This is found by our static analysis tool KNighter. 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(-)