[MODERATED] Re: [patch V4 06/11] x86/speculation/mds: Conditionally clear CPU buffers on idle entry

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Feb 22, 2019 at 11:24:24PM +0100, speck for Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Add a static key which controls the invocation of the CPU buffer clear
> mechanism on idle entry. This is independent of other MDS mitigations
> because the idle entry invocation to mitigate the potential leakage due to
> store buffer repartitioning is only necessary on SMT systems.
> 
> Add the actual invocations to the different halt/mwait variants which
> covers all usage sites. mwaitx is not patched as it's not available on
> Intel CPUs.
> 
> The buffer clear is only invoked before entering the C-State to prevent
> that stale data from the idling CPU is spilled to the Hyper-Thread sibling
> after the Store buffer got repartitioned and all entries are available to
> the non idle sibling.
> 
> When coming out of idle the store buffer is partitioned again so each
> sibling has half of it available. Now CPU which returned from idle could be
> speculatively exposed to contents of the sibling, but the buffers are
> flushed either on exit to user space or on VMENTER.
> 
> When later on conditional buffer clearing is implemented on top of this,
> then there is no action required either because before returning to user
> space the context switch will set the condition flag which causes a flush
> on the return to user path.
> 
> This intentionaly does not handle the case in the acpi/processor_idle

intentionally

> driver which uses the legacy IO port interface for C-State transitions for
> two reasons:
> 
>  - The acpi/processor_idle driver was replaced by the intel_idle driver
>    almost a decade ago. Anything Nehalem upwards supports it and defaults
>    to that new driver.
> 
>  - The legacy IO port interface is likely to be used on older and therefore
>    unaffected CPUs or on systems which do not receive microcode updates
>    anymore, so there is no point in adding that.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> ---
> V4: Export mds_idle_clear
> V3: Adjust document wording
> ---
>  Documentation/x86/mds.rst            |   35 +++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/irqflags.h      |    4 ++++
>  arch/x86/include/asm/mwait.h         |    7 +++++++
>  arch/x86/include/asm/nospec-branch.h |   12 ++++++++++++
>  arch/x86/kernel/cpu/bugs.c           |    3 +++
>  5 files changed, 61 insertions(+)
> 
> --- a/Documentation/x86/mds.rst
> +++ b/Documentation/x86/mds.rst
> @@ -135,3 +135,38 @@ Mitigation points
>     handler repopulates the buffers to some extent. Machine checks are not
>     reliably controllable and the window is extremly small so mitigation
>     would just tick a checkbox that this theoretical corner case is covered.
> +
> +
> +2. C-State transition
> +^^^^^^^^^^^^^^^^^^^^^
> +
> +   When a CPU goes idle and enters a C-State the CPU buffers need to be
> +   cleared on affected CPUs when SMT is active. This addresses the
> +   repartitioning of the store buffer when one of the Hyper-Threads enters
> +   a C-State.
> +
> +   When SMT is inactive, i.e. either the CPU does not support it or all
> +   sibling threads are offline CPU buffer clearing is not required.
> +
> +   The invocation is controlled by the static key mds_idle_clear which is
> +   switched depending on the chosen mitigation mode and the SMT state of
> +   the system.
> +
> +   The buffer clear is only invoked before entering the C-State to prevent
> +   that stale data from the idling CPU can be spilled to the Hyper-Thread

s/can be spilled/from spilling/

> +   sibling after the store buffer got repartitioned and all entries are
> +   available to the non idle sibling.
> +
> +   When coming out of idle the store buffer is partitioned again so each
> +   sibling has half of it available. The back from idle CPU could be then
> +   speculatively exposed to contents of the sibling. The buffers are
> +   flushed either on exit to user space or on VMENTER so malicious code
> +   in user space or the guest cannot speculatively access them.
> +
> +   The mitigation is hooked into all variants of halt()/mwait(), but does
> +   not cover the legacy ACPI IO-Port mechanism because the ACPI idle driver
> +   has been superseded by the intel_idle driver around 2010 and is
> +   preferred on all affected CPUs which are expected to gain the MD_CLEAR
> +   functionality in microcode. Aside of that the IO-Port mechanism is a
> +   legacy interface which is only used on older systems which are either
> +   not affected or do not receive microcode updates anymore.
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -6,6 +6,8 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#include <asm/nospec-branch.h>
> +
>  /* Provide __cpuidle; we can't safely include <linux/cpu.h> */
>  #define __cpuidle __attribute__((__section__(".cpuidle.text")))
>  
> @@ -54,11 +56,13 @@ static inline void native_irq_enable(voi
>  
>  static inline __cpuidle void native_safe_halt(void)
>  {
> +	mds_idle_clear_cpu_buffers();
>  	asm volatile("sti; hlt": : :"memory");
>  }
>  
>  static inline __cpuidle void native_halt(void)
>  {
> +	mds_idle_clear_cpu_buffers();
>  	asm volatile("hlt": : :"memory");
>  }
>  
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -6,6 +6,7 @@
>  #include <linux/sched/idle.h>
>  
>  #include <asm/cpufeature.h>
> +#include <asm/nospec-branch.h>
>  
>  #define MWAIT_SUBSTATE_MASK		0xf
>  #define MWAIT_CSTATE_MASK		0xf
> @@ -40,6 +41,8 @@ static inline void __monitorx(const void
>  
>  static inline void __mwait(unsigned long eax, unsigned long ecx)
>  {
> +	mds_idle_clear_cpu_buffers();
> +
>  	/* "mwait %eax, %ecx;" */
>  	asm volatile(".byte 0x0f, 0x01, 0xc9;"
>  		     :: "a" (eax), "c" (ecx));
> @@ -74,6 +77,8 @@ static inline void __mwait(unsigned long
>  static inline void __mwaitx(unsigned long eax, unsigned long ebx,
>  			    unsigned long ecx)
>  {
> +	/* No MDS buffer clear as this is AMD/HYGON only */
> +
>  	/* "mwaitx %eax, %ebx, %ecx;" */
>  	asm volatile(".byte 0x0f, 0x01, 0xfb;"
>  		     :: "a" (eax), "b" (ebx), "c" (ecx));
> @@ -81,6 +86,8 @@ static inline void __mwaitx(unsigned lon
>  
>  static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
>  {
> +	mds_idle_clear_cpu_buffers();
> +
>  	trace_hardirqs_on();
>  	/* "mwait %eax, %ecx;" */
>  	asm volatile("sti; .byte 0x0f, 0x01, 0xc9;"
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -319,6 +319,7 @@ DECLARE_STATIC_KEY_FALSE(switch_mm_cond_
>  DECLARE_STATIC_KEY_FALSE(switch_mm_always_ibpb);
>  
>  DECLARE_STATIC_KEY_FALSE(mds_user_clear);
> +DECLARE_STATIC_KEY_FALSE(mds_idle_clear);
>  
>  #include <asm/segment.h>
>  
> @@ -345,6 +346,17 @@ static inline void mds_clear_cpu_buffers
>  	asm volatile("verw %[ds]" : : [ds] "m" (ds) : "cc");
>  }
>  
> +/**
> + * mds_idle_clear_cpu_buffers - Mitigation for MDS vulnerability
> + *
> + * Clear CPU buffers if the corresponding static key is enabled
> + */
> +static inline void mds_idle_clear_cpu_buffers(void)
> +{
> +	if (static_branch_likely(&mds_idle_clear))
> +		mds_clear_cpu_buffers();
> +}

This two-line construct is more readable than the
mds_user_clear_cpu_buffers() three-line version from the previous patch,
I'd suggest doing the same thing there.

-- 
Josh