linux-kernel - Re: [PATCH] cpufreq: amd-pstate: avoid uninitialized variable use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Feb 2023 09:58:18 +0000
From:   Wyes Karny <wyes.karny@....com>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Huang Rui <ray.huang@....com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Perry Yuan <Perry.Yuan@....com>,
        Mario Limonciello <Mario.Limonciello@....com>,
        Arnd Bergmann <arnd@...db.de>, Jinzhou Su <Jinzhou.Su@....com>,
        Meng Li <li.meng@....com>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: amd-pstate: avoid uninitialized variable use

Hi Arnd, Ray, Perry,

On 07 Feb 17:12, Arnd Bergmann wrote:
> @@ -1076,9 +1075,9 @@ static void amd_pstate_epp_init(unsigned int cpu)
>  		value |= (u64)epp << 24;
>  	}
>  
> +	amd_pstate_set_epp(cpudata, epp);
>  skip_epp:
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
> -	amd_pstate_set_epp(cpudata, epp);
>  	cpufreq_cpu_put(policy);
>  }

I see an issue here with MSR-based systems.
Here EPP is being set before updating `cppc_req_cached` and for MSR
based systems while updating EPP value, MIN_PERF and MAX_PERF also
updated. So, here whatever value MSR_AMD_CPPC_REQ had for min and max
perf that will be rewritten instead of highest and lowest perf from
MSR_AMD_CPPC_CAP1.

Can we do something like this:

@@ -1053,32 +1052,34 @@ static void amd_pstate_epp_init(unsigned int
cpu)
        value &= ~AMD_CPPC_DES_PERF(~0L);
        value |= AMD_CPPC_DES_PERF(0);

+       /* No need to set epp again if previous and current policy is same */
        if (cpudata->epp_policy == cpudata->policy)
                goto skip_epp;

        cpudata->epp_policy = cpudata->policy;

-       if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
-               epp = amd_pstate_get_epp(cpudata, value);
-               if (epp < 0)
-                       goto skip_epp;
-               /* force the epp value to be zero for performance policy */
-               epp = 0;
-       } else {
-               /* Get BIOS pre-defined epp value */
-               epp = amd_pstate_get_epp(cpudata, value);
-               if (epp)
-                       goto skip_epp;
+       /* Get BIOS pre-defined epp value */
+       epp = amd_pstate_get_epp(cpudata, value);
+       if (epp < 0) {
+               /**
+                * This return value can only be negative for shared_memory
+                * systems where EPP register read/write not supported.
+                */
+               goto skip_epp;
        }
+
+       if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
+               epp = 0;
+
        /* Set initial EPP value */
        if (boot_cpu_has(X86_FEATURE_CPPC)) {
                value &= ~GENMASK_ULL(31, 24);
                value |= (u64)epp << 24;
        }

-skip_epp:
        WRITE_ONCE(cpudata->cppc_req_cached, value);
        amd_pstate_set_epp(cpudata, epp);
+skip_epp:
        cpufreq_cpu_put(policy);
 }


>  
> -- 
> 2.39.1
>