[MODERATED] Re: [patch V2 05/10] MDS basics+ 5

[Borislav Petkov <bp@suse.de>]

On Wed, Feb 20, 2019 at 04:07:58PM +0100, speck for Thomas Gleixner wrote:
> Subject: [patch V2 05/10] x86/speculation/mds: Conditionaly clear CPU buffers on idle entry

WARNING: 'Conditionaly' may be misspelled - perhaps 'Conditionally'?

> 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 can be spilled to the Hyper-Thread

s/can //

> 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 the back from idle CPU could be

"Now the CPU which returned from idle... "

> 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
> 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>
> ---
>  Documentation/x86/mds.rst            |   33 +++++++++++++++++++++++++++++++++
>  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           |    2 ++
>  5 files changed, 58 insertions(+)
> 
> --- a/Documentation/x86/mds.rst
> +++ b/Documentation/x86/mds.rst
> @@ -143,3 +143,36 @@ Mitigation points
>  
>  	None of those are controllable by unpriviledged attackers to form a
>  	reliable exploit surface.
> +
> +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-Thread enters a

"... one of the Hyper-Threads... "

> +   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
> +   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 the back from idle CPU could be
> +   speculatively exposed to contents of the sibling, but the buffers are
> +   flushed either on exit to user space or on VMENTER.
> +
> +   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 superseeded by the intel_idle driver around 2010 and is

WARNING: 'superseeded' may be misspelled - perhaps 'superseded'?

> +   preferred on all affected CPUs which still receive microcode updates
> +   (Nehalem onwards). 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.

With that addressed:

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--