Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Jul 2022 10:59:59 -0700
From:   Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Jonathan Corbet <corbet@lwn.net>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        tony.luck@intel.com, antonio.gomez.iglesias@linux.intel.com,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        andrew.cooper3@citrix.com, Josh Poimboeuf <jpoimboe@kernel.org>
Subject: Re: [RESEND RFC PATCH] x86/bugs: Add "unknown" reporting for MMIO
 Stale Data
Message-ID: <20220729175959.w7gd5z7dsbxrnydn@desk>
References: <a932c154772f2121794a5f2eded1a11013114711.1657846269.git.pawan.kumar.gupta@linux.intel.com>
 <f173a7c0-b4f8-17f3-a65d-e581fed32368@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f173a7c0-b4f8-17f3-a65d-e581fed32368@intel.com>
Precedence: bulk

On Thu, Jul 28, 2022 at 12:08:39PM -0700, Dave Hansen wrote:
> On 7/14/22 18:30, Pawan Gupta wrote:
> > Older CPUs beyond its Servicing period are not listed in the affected
> > processor list for MMIO Stale Data vulnerabilities. These CPUs currently
> > report "Not affected" in sysfs, which may not be correct.
> 
> I'd kinda like to remove the talk about the "servicing period" in this
> patch.  First, it's a moving target.  CPUs can move in and out of their
> servicing period as Intel changes its mind, or simply as time passes.
> 
> Intel could also totally choose to report a CPU as vulnerable *AND* have
> it be outside its service period.  Or, some good Samaritan community
> member might be able to test a crusty old CPU and determine if it's
> vulnerable.
> 
> > diff --git a/Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst b/Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst
> > index 9393c50b5afc..55524e0798da 100644
> > --- a/Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst
> > +++ b/Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst
> > @@ -230,6 +230,9 @@ The possible values in this file are:
> >       * - 'Mitigation: Clear CPU buffers'
> >         - The processor is vulnerable and the CPU buffer clearing mitigation is
> >           enabled.
> > +     * - 'Unknown: CPU is beyond its Servicing period'
> > +       - The processor vulnerability status is unknown because it is
> > +	 out of Servicing period. Mitigation is not attempted.
> 
> Unknown: Processor vendor did not provide vulnerability status.
> 
> >  If the processor is vulnerable then the following information is appended to
> >  the above information:
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index 0dd04713434b..dd6e78d370bc 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -416,6 +416,7 @@ enum mmio_mitigations {
> >  	MMIO_MITIGATION_OFF,
> >  	MMIO_MITIGATION_UCODE_NEEDED,
> >  	MMIO_MITIGATION_VERW,
> > +	MMIO_MITIGATION_UNKNOWN,
> >  };
> >  
> >  /* Default mitigation for Processor MMIO Stale Data vulnerabilities */
> > @@ -426,12 +427,18 @@ static const char * const mmio_strings[] = {
> >  	[MMIO_MITIGATION_OFF]		= "Vulnerable",
> >  	[MMIO_MITIGATION_UCODE_NEEDED]	= "Vulnerable: Clear CPU buffers attempted, no microcode",
> >  	[MMIO_MITIGATION_VERW]		= "Mitigation: Clear CPU buffers",
> > +	[MMIO_MITIGATION_UNKNOWN]	= "Unknown: CPU is beyond its servicing period",
> >  };
> 
> Let's just say:
> 
> 	Unknown: no mitigations
> 
> or even just: "Unknown"
> 
> >  static void __init mmio_select_mitigation(void)
> >  {
> >  	u64 ia32_cap;
> >  
> > +	if (mmio_stale_data_unknown()) {
> > +		mmio_mitigation = MMIO_MITIGATION_UNKNOWN;
> > +		return;
> > +	}
> > +
> >  	if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> >  	    cpu_mitigations_off()) {
> >  		mmio_mitigation = MMIO_MITIGATION_OFF;
> > @@ -1638,6 +1645,7 @@ void cpu_bugs_smt_update(void)
> >  			pr_warn_once(MMIO_MSG_SMT);
> >  		break;
> >  	case MMIO_MITIGATION_OFF:
> > +	case MMIO_MITIGATION_UNKNOWN:
> >  		break;
> >  	}
> >  
> > @@ -2235,7 +2243,8 @@ static ssize_t tsx_async_abort_show_state(char *buf)
> >  
> >  static ssize_t mmio_stale_data_show_state(char *buf)
> >  {
> > -	if (mmio_mitigation == MMIO_MITIGATION_OFF)
> > +	if (mmio_mitigation == MMIO_MITIGATION_OFF ||
> > +	    mmio_mitigation == MMIO_MITIGATION_UNKNOWN)
> >  		return sysfs_emit(buf, "%s\n", mmio_strings[mmio_mitigation]);
> >  
> >  	if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 736262a76a12..82088410870e 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -1286,6 +1286,22 @@ static bool arch_cap_mmio_immune(u64 ia32_cap)
> >  		ia32_cap & ARCH_CAP_SBDR_SSDP_NO);
> >  }
> >  
> > +bool __init mmio_stale_data_unknown(void)
> > +{
> > +	u64 ia32_cap = x86_read_arch_cap_msr();
> > +
> > +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> > +		return false;
> 
> Let's say why Intel is the special snowflake.  Maybe:
> 
> 	/*
> 	 * Intel does not document vulnerability information for old
> 	 * CPUs.  This means that only Intel CPUs can have unknown
> 	 * vulnerability state.
> 	 */
> 
> > +	/*
> > +	 * CPU vulnerability is unknown when, hardware doesn't set the
> > +	 * immunity bits and CPU is not in the known affected list.
> > +	 */
> > +	if (!cpu_matches(cpu_vuln_blacklist, MMIO) &&
> > +	    !arch_cap_mmio_immune(ia32_cap))
> > +		return true;
> > +	return false;
> > +}
> > +
> >  static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
> >  {
> >  	u64 ia32_cap = x86_read_arch_cap_msr();
> > @@ -1349,14 +1365,8 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
> >  	    cpu_matches(cpu_vuln_blacklist, SRBDS | MMIO_SBDS))
> >  		    setup_force_cpu_bug(X86_BUG_SRBDS);
> >  
> > -	/*
> > -	 * Processor MMIO Stale Data bug enumeration
> > -	 *
> > -	 * Affected CPU list is generally enough to enumerate the vulnerability,
> > -	 * but for virtualization case check for ARCH_CAP MSR bits also, VMM may
> > -	 * not want the guest to enumerate the bug.
> > -	 */
> > -	if (cpu_matches(cpu_vuln_blacklist, MMIO) &&
> > +	 /* Processor MMIO Stale Data bug enumeration */
> > +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> >  	    !arch_cap_mmio_immune(ia32_cap))
> >  		setup_force_cpu_bug(X86_BUG_MMIO_STALE_DATA);
> 
> Yeah, this is all looking a little clunky.
> 
> Maybe we just need a third state of cpu_has_bug() for all this and we
> shouldn't try cramming it in the MMIO-specific code and diluting the
> specificity of boot_cpu_has_bug().
> 
> Then the selection logic becomes simple:
> 
> 	if (!arch_cap_mmio_immune(ia32_cap))) {
> 		if (cpu_matches(cpu_vuln_blacklist, MMIO))
> 			setup_force_cpu_bug(X86_BUG_MMIO_STALE_DATA);
> 		else if (x86_vendor == X86_VENDOR_INTEL)
> 			setup_force_unknown_bug(X86_BUG_MMIO...);
> 	}
> 
> ... and then spit out the "Unknown" in the common code, just like the
> treatment "Not affected" gets.
> 
> static ssize_t cpu_show_common(...)
> {
>         if (!boot_cpu_has_bug(bug))
>                 return sprintf(buf, "Not affected\n");
> +
> +       if (!boot_cpu_unknown_bug(bug))
> +               return sprintf(buf, "Unknown\n");
> 
> Thoughts?

Sounds good. Similar to this Borislav suggested to add
X86_BUG_MMIO_UNKNOWN. I will see if I can combine both approaches.