Re: [RESEND RFC PATCH] x86/bugs: Add "unknown" reporting for MMIO Stale Data

[Pawan Gupta]

On Thu, Jul 28, 2022 at 02:00:13PM +0200, Borislav Petkov wrote:
> On Thu, Jul 14, 2022 at 06:30:18PM -0700, 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.
> > 
> > Add support for "Unknown" reporting for such CPUs. Mitigation is not
> > deployed when the status is "Unknown".
> > 
> > "CPU is beyond its Servicing period" means these CPUs are beyond their
> > Servicing [1] period and have reached End of Servicing Updates (ESU) [2].
> > 
> >   [1] Servicing: The process of providing functional and security
> >   updates to Intel processors or platforms, utilizing the Intel Platform
> >   Update (IPU) process or other similar mechanisms.
> > 
> >   [2] End of Servicing Updates (ESU): ESU is the date at which Intel
> >   will no longer provide Servicing, such as through IPU or other similar
> >   update processes. ESU dates will typically be aligned to end of
> >   quarter.
> 
> The explanations of those things need to be...
> 
> > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Suggested-by: Tony Luck <tony.luck@xxxxxxxxx>
> > Fixes: 8d50cdf8b834 ("x86/speculation/mmio: Add sysfs reporting for Processor MMIO Stale Data")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@xxxxxxxxxxxxxxx>
> > ---
> > CPU vulnerability is unknown if, hardware doesn't set the immunity bits
> > and CPU is not in the known-affected-list.
> > 
> > In order to report the unknown status, this patch sets the MMIO bug
> > for all Intel CPUs that don't have the hardware immunity bits set.
> > Based on the known-affected-list of CPUs, mitigation selection then
> > deploys the mitigation or sets the "Unknown" status; which is ugly.
> > 
> > I will appreciate suggestions to improve this.
> > 
> > Thanks,
> > Pawan
> > 
> >  .../hw-vuln/processor_mmio_stale_data.rst     |  3 +++
> >  arch/x86/kernel/cpu/bugs.c                    | 11 +++++++-
> >  arch/x86/kernel/cpu/common.c                  | 26 +++++++++++++------
> >  arch/x86/kernel/cpu/cpu.h                     |  1 +
> >  4 files changed, 32 insertions(+), 9 deletions(-)
> > 
> > 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.
> 
> ... here.

Sure, I will move it here.

> > 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)
> 
> This function need to go to ...cpu/intel.c

It will be big churn to move this function to ...cpu/intel.c.

cpu/intel.c is not compiled for !CONFIG_CPU_SUP_INTEL, and bugs.c
depends on this function. For bugs.c to compile there needs to be
another version of this function for !CONFIG_CPU_SUP_INTEL.

Secondly, this function relies on cpu_vuln_blacklist and
arch_cap_mmio_immune(), both are in cpu/common.c and defined static.
They would also need to made available in cpu/intel.c.

To keep things simple, can this stay in cpu/common.c? And if there is a
compelling reason, I am willing to make the required changes.

> > +{
> > +	u64 ia32_cap = x86_read_arch_cap_msr();
> > +
> > +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> > +		return false;
> 
> <---- newline here.
> 
> > +	/*
> > +	 * CPU vulnerability is unknown when, hardware doesn't set the
> 
> no comma after the "when"
> 
> > +	 * 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;
> 
> <---- newline here.

Will do this, and above changes.

> > +	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 &&
> 
> Why is that vendor check here? We have the cpu_vuln_blacklist for a
> reason.

cpu_vuln_blacklist only has in-support MMIO vulnerable CPUs.

There is no easy way to report unknown status for older CPUs. This
patch sets the MMIO bug for all Intel CPUs that don't have the hardware
immunity bits set.

Later, mmio_select_mitigation() deploys the mitigation or sets the
"Unknown" status based on the known-affected-list of CPUs i.e.
cpu_vuln_blacklist.

The vendor check is required, otherwise MMIO bug will be set for
non-Intel CPUs also.

Thanks,
Pawan