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

On Sun, 13 Apr 2025 22:31:26 +0100,
Chenyuan Yang [off-list ref] wrote:

On Sun, Apr 13, 2025 at 5:02 AM Marc Zyngier [off-list ref] wrote:

quoted

On Sat, 12 Apr 2025 17:05:18 +0100,
Chenyuan Yang [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

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