2024-06-03 11:16:29

by Anastasia Belova

Subject: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value

cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it
and return in case of error.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Anastasia Belova <[email protected]>
---
drivers/cpufreq/amd-pstate.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 1b7e82a0ad2e..672cb6c280a4 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
unsigned long max_perf, min_perf, des_perf,
cap_perf, lowest_nonlinear_perf, max_freq;
struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+ if (!policy)
+ return;
struct amd_cpudata *cpudata = policy->driver_data;
unsigned int target_freq;

@@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
static void amd_pstate_update_limits(unsigned int cpu)
{
struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+ if (!policy)
+ return;
struct amd_cpudata *cpudata = policy->driver_data;
u32 prev_high = 0, cur_high = 0;
int ret;
--
2.30.2

2024-06-04 16:08:41

by Mario Limonciello

Subject: Re: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value

On 6/3/2024 06:07, Anastasia Belova wrote:
> cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it
> and return in case of error.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Anastasia Belova <[email protected]>
Thank you!

Acked-by: Mario Limonciello <[email protected]>

> ---
> drivers/cpufreq/amd-pstate.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 1b7e82a0ad2e..672cb6c280a4 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> unsigned long max_perf, min_perf, des_perf,
> cap_perf, lowest_nonlinear_perf, max_freq;
> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> + if (!policy)
> + return;
> struct amd_cpudata *cpudata = policy->driver_data;
> unsigned int target_freq;
>
> @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
> static void amd_pstate_update_limits(unsigned int cpu)
> {
> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> + if (!policy)
> + return;
> struct amd_cpudata *cpudata = policy->driver_data;
> u32 prev_high = 0, cur_high = 0;
> int ret;

2024-06-06 09:56:33

by Gautham R.Shenoy

Subject: Re: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value

Hello,

On Mon, Jun 03, 2024 at 02:07:41PM +0300, Anastasia Belova wrote:
> cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it
> and return in case of error.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Anastasia Belova <[email protected]>

Thank you for the patch. Indeed we should be checking if the policy is
valid before dereferencing it.

> ---
> drivers/cpufreq/amd-pstate.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 1b7e82a0ad2e..672cb6c280a4 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> unsigned long max_perf, min_perf, des_perf,
> cap_perf, lowest_nonlinear_perf, max_freq;
> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> + if (!policy)
> + return;

This patch mixes code and declarations. While I personally don't
prefer that, since we have moved to using C99, the compiler does
not complain, nor does checkpatch complain.

So is this ok for cpufreq, Rafael?

Or would you prefer something like:

unsigned long cap_perf, lowest_nonlinear_perf;
unsigned long max_perf, min_perf, des_perf;
struct cpufreq_policy *policy;
struct amd_cpudata *cpudata;
unsigned int target_freq;
unsigned long max_freq;

policy = cpufreq_cpu_get(cpu);
if (!policy)
return;

cpudata = policy->driver_data;



> struct amd_cpudata *cpudata = policy->driver_data;
> unsigned int target_freq;
>
> @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
> static void amd_pstate_update_limits(unsigned int cpu)
> {
> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> + if (!policy)
> + return;

Ditto.

> struct amd_cpudata *cpudata = policy->driver_data;
> u32 prev_high = 0, cur_high = 0;
> int ret;
> --
> 2.30.2
>

--
Thanks and Regards
gautham.