From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 26 Feb 2019 15:31:47 -0000 Received: from mx1.redhat.com ([209.132.183.28]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1gyeht-0003Fx-J9 for speck@linutronix.de; Tue, 26 Feb 2019 16:31:46 +0100 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1EF56308FEC3 for ; Tue, 26 Feb 2019 15:31:39 +0000 (UTC) Received: from treble (ovpn-120-232.rdu2.redhat.com [10.10.120.232]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C758D5D6B3 for ; Tue, 26 Feb 2019 15:31:38 +0000 (UTC) Date: Tue, 26 Feb 2019 09:31:36 -0600 From: Josh Poimboeuf Subject: [MODERATED] Re: [patch V4 06/11] x86/speculation/mds: Conditionally clear CPU buffers on idle entry Message-ID: <20190226153136.icbx2ch3tqfjnik6@treble> References: <20190222222418.405369026@linutronix.de> <20190222224149.616531116@linutronix.de> MIME-Version: 1.0 In-Reply-To: <20190222224149.616531116@linutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Fri, Feb 22, 2019 at 11:24:24PM +0100, speck for Thomas Gleixner wrote: > From: Thomas Gleixner > > 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 > Reviewed-by: Borislav Petkov > --- > 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 > + > /* Provide __cpuidle; we can't safely include */ > #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 > > #include > +#include > > #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 > > @@ -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