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]

Date: Fri, 21 Jun 2024 16:46:15 -0700
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: Sohil Mehta <sohil.mehta@...el.com>
Cc: 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>, Andi Kleen <andi.kleen@...el.com>, Xin Li
 <xin3.li@...el.com>, jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v2 1/6] x86/irq: Add enumeration of NMI source reporting
 CPU feature


On Fri, 21 Jun 2024 15:23:51 -0700, Sohil Mehta <sohil.mehta@...el.com>
wrote:

> > 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?
My understanding is that cpuid_deps is to ensure CPU features are
cleared according to its dependency chain. Not for HW bugs/quirks.

> 
> 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      },
>         {}
>  };
If FRED is never reported by CPUID, then there would not be any calls to
setup_clear_cpu_cap(X86_FEATURE_FRED), so this table does not help clear
the dependent NMI_SOURCE, right?

In the next version, I will add runtime disable if HW malfunctions. i.e. no
valid bitmask.

Maybe we can also add a big WARN_ON like this:
if (WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_FRED) &&
		cpu_feature_enabled(X86_FEATURE_NMI_SOURCE)) 
	setup_clear_cpu_cap(X86_FEATURE_NMI_SOURCE);

Thanks,

Jacob