linux-kernel - Re: [PATCH v2 1/6] x86/irq: Add enumeration of NMI source reporting CPU feature

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]

Message-ID: <b3b10d29-857e-402b-95b9-1696baa88e81@intel.com>
Date: Fri, 21 Jun 2024 15:23:51 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: Jacob Pan <jacob.jun.pan@...ux.intel.com>, X86 Kernel <x86@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>,
	Dave Hansen <dave.hansen@...el.com>, "H. Peter Anvin" <hpa@...or.com>, "Ingo
 Molnar" <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	<linux-perf-users@...r.kernel.org>, Peter Zijlstra <peterz@...radead.org>
CC: Andi Kleen <andi.kleen@...el.com>, Xin Li <xin3.li@...el.com>
Subject: Re: [PATCH v2 1/6] x86/irq: Add enumeration of NMI source reporting
 CPU feature

Hi Jacob,

On 6/11/2024 9:54 AM, Jacob Pan wrote:

> 
> The functionality of NMI source reporting is tied to the FRED. Although it
> is enumerated by a unique CPUID feature bit, it cannot be turned off
> independently once FRED is activated.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> ---
> v2: Removed NMI source from static CPU ID dependency table (HPA)

I am not sure if this would work in all scenarios. See below.
Sorry, I couldn't chime-in during the v1 review when this was suggested.


> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4fa0b17e5043..465f04e4a79f 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -1427,8 +1427,10 @@ early_param("fred", fred_setup);
>  
>  void __init trap_init(void)
>  {
> -	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred)
> +	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred) {
>  		setup_clear_cpu_cap(X86_FEATURE_FRED);
> +		setup_clear_cpu_cap(X86_FEATURE_NMI_SOURCE);
> +	}
>  
>  	/* Init cpu_entry_area before IST entries are set up */
>  	setup_cpu_entry_areas();

I think this relies on the fact that whenever X86_FEATURE_NMI_SOURCE is
set, X86_FEATURE_FRED will also be set by the hardware. Though this
might be the expected behavior, hardware sometimes messes up and the
dependency entry in the static table would probably help catch that.

IIUC, when X86_FEATURE_NMI_SOURCE is set and X86_FEATURE_FRED is
cleared, cpu_feature_enabled(X86_FEATURE_FRED) will fail and the above
check would not end up clearing X86_FEATURE_NMI_SOURCE.

Isn't the following entry necessary to detect a misconfiguration or is
the purpose of the cpuid_deps table something else?

diff --git a/arch/x86/kernel/cpu/cpuid-deps.c
b/arch/x86/kernel/cpu/cpuid-deps.c
index b7d9f530ae16..39526041e91a 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -84,6 +84,7 @@ static const struct cpuid_dep cpuid_deps[] = {
        { X86_FEATURE_SHSTK,                    X86_FEATURE_XSAVES    },
        { X86_FEATURE_FRED,                     X86_FEATURE_LKGS      },
        { X86_FEATURE_FRED,                     X86_FEATURE_WRMSRNS   },
+       { X86_FEATURE_NMI_SOURCE,		X86_FEATURE_FRED      },
        {}
 };