[MODERATED] [PATCH 0/4] RFC-MDSv1 1

[MODERATED] [PATCH 0/4] RFC-MDSv1 1

[Andi Kleen]

Here's a preview of patches to add VERW support to the Linux for the
recently released Coffelake microcode. This allows the kernel
to flush CPU buffers on kernel boundaries and when going into idle.

This mainly covers single thread, not SMT (except for the idle case)

I lumped all the issues together under the Microarchitectural Data
Sampling (MDS) name because they need the same mitigations,a
and it doesn't seem worth duplicating the sysfs files and bug entries.

Patches passed some stress testing.

Some notes:
- So far against 4.19
- 64bit only so far. 32bit will be in future versions.
- Some other CPUs will need software sequences. These will
be in a future version of the patchkit, and also will require
moving some of the hooks.
- One of the MSR bits uses the older name "Microarchitectural Buffer
Sampling" (MBS). So far I kept that until that documentation is updated.

Right now this is not to apply, but to discuss and that people can
experiment if they want.

Comments welcome.

Andi Kleen (4):
  x86/speculation/mds: Add basic bug infrastructre for MDS
  x86/speculation/mds: Clear CPU buffers on kernel exit for 64bit
  x86/speculation/mds: Clear CPU buffers on entering idle
  x86/speculation/mds: Add sysfs reporting

 .../admin-guide/kernel-parameters.txt         |  3 ++
 arch/x86/entry/calling.h                      |  8 ++++
 arch/x86/entry/entry_64.S                     |  6 +++
 arch/x86/entry/entry_64_compat.S              |  1 +
 arch/x86/include/asm/cpufeatures.h            |  2 +
 arch/x86/include/asm/irqflags.h               |  3 ++
 arch/x86/include/asm/msr-index.h              |  1 +
 arch/x86/include/asm/mwait.h                  |  4 ++
 arch/x86/include/asm/nospec-branch.h          |  5 +++
 arch/x86/kernel/cpu/bugs.c                    | 44 +++++++++++++++++++
 arch/x86/kernel/cpu/common.c                  | 14 ++++++
 arch/x86/kernel/kvm.c                         |  1 +
 arch/x86/kernel/process.c                     |  4 ++
 arch/x86/kernel/smpboot.c                     |  1 +
 drivers/base/cpu.c                            |  8 ++++
 15 files changed, 105 insertions(+)

-- 
2.17.2

[MODERATED] [PATCH 1/4] RFC-MDSv1 4

[Andi Kleen]

MDS is micro architectural data sampling, which is a side channel
attack on several internal buffers in Intel CPUs. They all have
the same mitigation, so we lump them all together as a
single MDS issue.

This patch adds the basic infrastructure to detect if the current
CPU is affected by MDS, and if yes set the right BUG bits.

We also provide a command line option "mds_disable" to disable
any workarounds.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 arch/x86/include/asm/cpufeatures.h              |  2 ++
 arch/x86/include/asm/msr-index.h                |  1 +
 arch/x86/kernel/cpu/bugs.c                      | 10 ++++++++++
 arch/x86/kernel/cpu/common.c                    | 14 ++++++++++++++
 5 files changed, 30 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 92eb1f42240d..b8dfb3ac2af6 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2312,6 +2312,9 @@
 			Format: <first>,<last>
 			Specifies range of consoles to be captured by the MDA.
 
+	mds_disable	[X86]
+			Disable workarounds for Micro-architectural Data Sampling.
+
 	mem=nn[KMG]	[KNL,BOOT] Force usage of a specific amount of memory
 			Amount of memory to be used when the kernel is not able
 			to see the whole system memory or for test.
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 89a048c2faec..60751c82982c 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -340,6 +340,7 @@
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (EDX), word 18 */
 #define X86_FEATURE_AVX512_4VNNIW	(18*32+ 2) /* AVX-512 Neural Network Instructions */
 #define X86_FEATURE_AVX512_4FMAPS	(18*32+ 3) /* AVX-512 Multiply Accumulation Single precision */
+#define X86_FEATURE_MB_CLEAR		(18*32+10) /* Flush state on VERW */
 #define X86_FEATURE_PCONFIG		(18*32+18) /* Intel PCONFIG */
 #define X86_FEATURE_SPEC_CTRL		(18*32+26) /* "" Speculation Control (IBRS + IBPB) */
 #define X86_FEATURE_INTEL_STIBP		(18*32+27) /* "" Single Thread Indirect Branch Predictors */
@@ -377,5 +378,6 @@
 #define X86_BUG_SPECTRE_V2		X86_BUG(16) /* CPU is affected by Spectre variant 2 attack with indirect branches */
 #define X86_BUG_SPEC_STORE_BYPASS	X86_BUG(17) /* CPU is affected by speculative store bypass attack */
 #define X86_BUG_L1TF			X86_BUG(18) /* CPU is affected by L1 Terminal Fault */
+#define X86_BUG_MDS			X86_BUG(19) /* CPU is affected by Microarchitectural data sampling */
 
 #endif /* _ASM_X86_CPUFEATURES_H */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 4731f0cf97c5..e54b4035b462 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -76,6 +76,7 @@
 						    * attack, so no Speculative Store Bypass
 						    * control required.
 						    */
+#define ARCH_CAP_MBS_NO			(1 << 5)   /* No Microarchitectural buffer sampling */
 
 #define MSR_IA32_FLUSH_CMD		0x0000010b
 #define L1D_FLUSH			(1 << 0)   /*
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 40bdaea97fe7..a4630d558ebd 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -34,6 +34,7 @@
 static void __init spectre_v2_select_mitigation(void);
 static void __init ssb_select_mitigation(void);
 static void __init l1tf_select_mitigation(void);
+static void __init mds_select_mitigation(void);
 
 /*
  * Our boot-time value of the SPEC_CTRL MSR. We read it once so that any
@@ -93,6 +94,8 @@ void __init check_bugs(void)
 
 	l1tf_select_mitigation();
 
+	mds_select_mitigation();
+
 #ifdef CONFIG_X86_32
 	/*
 	 * Check whether we are able to run this kernel safely on SMP.
@@ -775,6 +778,13 @@ early_param("l1tf", l1tf_cmdline);
 
 #undef pr_fmt
 
+static void mds_select_mitigation(void)
+{
+	if (cmdline_find_option_bool(boot_command_line, "mds_disable") ||
+	    !boot_cpu_has_bug(X86_BUG_MDS))
+		setup_clear_cpu_cap(X86_FEATURE_MB_CLEAR);
+}
+
 #ifdef CONFIG_SYSFS
 
 #define L1TF_DEFAULT_MSG "Mitigation: PTE Inversion"
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 44c4ef3d989b..e71b498e111d 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -997,6 +997,14 @@ static const __initconst struct x86_cpu_id cpu_no_l1tf[] = {
 	{}
 };
 
+static const __initconst struct x86_cpu_id cpu_no_mds[] = {
+	/* in addition to cpu_no_speculation */
+	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_GOLDMONT	},
+	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_DENVERTON	},
+	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_GEMINI_LAKE	},
+	{}
+};
+
 static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 {
 	u64 ia32_cap = 0;
@@ -1018,6 +1026,12 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (ia32_cap & ARCH_CAP_IBRS_ALL)
 		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
 
+	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+	    !x86_match_cpu(cpu_no_mds)) &&
+	    !(ia32_cap & ARCH_CAP_MBS_NO) &&
+	    !(ia32_cap & ARCH_CAP_RDCL_NO))
+		setup_force_cpu_bug(X86_BUG_MDS);
+
 	if (x86_match_cpu(cpu_no_meltdown))
 		return;
 
-- 
2.17.2

[MODERATED] [PATCH 2/4] RFC-MDSv1 0

[Andi Kleen]

For MDS the CPU might leak previously touched data
in CPU internal structures. Make sure to clear these structures
every time we exit the kernel. This prevents any leakage
between user processes or between kernel and user.

The flushing is provided by new microcode as a new side
effect of the otherwise unused VERW instruction. We
add VERW to all the kernel exit paths.

We don't need to do this for guests because the L1TF cache
flush will implicitely do the same flushing, and is
automatically selected on MDS affected systems.

This mitigation doesn't address Hyper Threading.

So far this is for 64bit only, 32bit is not covered yet.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/entry/calling.h         | 8 ++++++++
 arch/x86/entry/entry_64.S        | 6 ++++++
 arch/x86/entry/entry_64_compat.S | 1 +
 3 files changed, 15 insertions(+)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 352e70cd33e8..e16f0f9f73cf 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -1,4 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/stringify.h>
+#include <asm/segment.h>
 #include <linux/jump_label.h>
 #include <asm/unwind_hints.h>
 #include <asm/cpufeatures.h>
@@ -329,6 +331,12 @@ For 32-bit we have the following conventions - kernel is built with
 
 #endif
 
+.macro EXIT_MDS
+	/* Clear CPU buffers that could leak. Instruction must be in memory form. */
+	ALTERNATIVE "", __stringify(pushq $__USER_DS ; verw (%rsp) ; addq $8, %rsp),\
+		 X86_FEATURE_MB_CLEAR
+.endm
+
 #endif /* CONFIG_X86_64 */
 
 /*
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f95dcb209fdf..b6c768820c6c 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -325,6 +325,8 @@ syscall_return_via_sysret:
 	pushq	RSP-RDI(%rdi)	/* RSP */
 	pushq	(%rdi)		/* RDI */
 
+	EXIT_MDS
+
 	/*
 	 * We are on the trampoline stack.  All regs except RDI are live.
 	 * We can do future final exit work right here.
@@ -684,6 +686,8 @@ GLOBAL(swapgs_restore_regs_and_return_to_usermode)
 	/* Push user RDI on the trampoline stack. */
 	pushq	(%rdi)
 
+	EXIT_MDS
+
 	/*
 	 * We are on the trampoline stack.  All regs except RDI are live.
 	 * We can do future final exit work right here.
@@ -806,6 +810,7 @@ native_irq_return_ldt:
 	orq	PER_CPU_VAR(espfix_stack), %rax
 
 	SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi
+	EXIT_MDS
 	SWAPGS					/* to user GS */
 	popq	%rdi				/* Restore user RDI */
 
@@ -1645,6 +1650,7 @@ end_repeat_nmi:
 	jnz	nmi_restore
 nmi_swapgs:
 	SWAPGS_UNSAFE_STACK
+	EXIT_MDS
 nmi_restore:
 	POP_REGS
 
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 7d0df78db727..d1abe8b7c605 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -271,6 +271,7 @@ sysret32_from_system_call:
 	popq	%rdx			/* Skip pt_regs->cx */
 	popq	%rdx			/* pt_regs->dx */
 	popq	%rsi			/* pt_regs->si */
+	EXIT_MDS
 	popq	%rdi			/* pt_regs->di */
 
         /*
-- 
2.17.2

[MODERATED] [PATCH 3/4] RFC-MDSv1 3

[Andi Kleen]

When entering idle the internal state of the current CPU might
become visible to the thread sibling because the CPU "frees" some
internal resources.

To ensure there is no MDS leakage always clear the CPU state
before doing any idling. We only do this if SMT is enabled,
as otherwise there is no leakage possible.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/irqflags.h      |  3 +++
 arch/x86/include/asm/mwait.h         |  4 ++++
 arch/x86/include/asm/nospec-branch.h |  5 +++++
 arch/x86/kernel/cpu/bugs.c           | 18 ++++++++++++++++++
 arch/x86/kernel/kvm.c                |  1 +
 arch/x86/kernel/process.c            |  4 ++++
 arch/x86/kernel/smpboot.c            |  1 +
 7 files changed, 36 insertions(+)

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 15450a675031..754beb7e4886 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -3,6 +3,7 @@
 #define _X86_IRQFLAGS_H_
 
 #include <asm/processor-flags.h>
+#include <asm/nospec-branch.h>
 
 #ifndef __ASSEMBLY__
 
@@ -96,6 +97,7 @@ static inline notrace void arch_local_irq_enable(void)
  */
 static inline __cpuidle void arch_safe_halt(void)
 {
+	clear_cpu_buffers_idle();
 	native_safe_halt();
 }
 
@@ -105,6 +107,7 @@ static inline __cpuidle void arch_safe_halt(void)
  */
 static inline __cpuidle void halt(void)
 {
+	clear_cpu_buffers_idle();
 	native_halt();
 }
 
diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
index 39a2fb29378a..36edad498d19 100644
--- a/arch/x86/include/asm/mwait.h
+++ b/arch/x86/include/asm/mwait.h
@@ -40,6 +40,8 @@ static inline void __monitorx(const void *eax, unsigned long ecx,
 
 static inline void __mwait(unsigned long eax, unsigned long ecx)
 {
+	clear_cpu_buffers_idle();
+
 	/* "mwait %eax, %ecx;" */
 	asm volatile(".byte 0x0f, 0x01, 0xc9;"
 		     :: "a" (eax), "c" (ecx));
@@ -82,6 +84,8 @@ static inline void __mwaitx(unsigned long eax, unsigned long ebx,
 static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 {
 	trace_hardirqs_on();
+	clear_cpu_buffers_idle();
+
 	/* "mwait %eax, %ecx;" */
 	asm volatile("sti; .byte 0x0f, 0x01, 0xc9;"
 		     :: "a" (eax), "c" (ecx));
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index fd2a8c1b88bc..68daf585208e 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -7,6 +7,7 @@
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
 #include <asm/msr-index.h>
+#include <asm/segment.h>
 
 /*
  * Fill the CPU return stack buffer.
@@ -355,4 +356,8 @@ do {								\
 # endif
 #endif
 
+#ifndef __ASSEMBLY__
+void clear_cpu_buffers_idle(void);
+#endif
+
 #endif /* _ASM_X86_NOSPEC_BRANCH_H_ */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index a4630d558ebd..e220453a1350 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -785,6 +785,24 @@ static void mds_select_mitigation(void)
 		setup_clear_cpu_cap(X86_FEATURE_MB_CLEAR);
 }
 
+/*
+ * Clear CPU buffers before going idle, so that no state is leaked to SMT
+ * siblings taking over thread resources.
+ * Out of line to avoid include hell.
+ */
+void clear_cpu_buffers_idle(void)
+{
+	if (cpu_smt_control != CPU_SMT_ENABLED)
+		return;
+	/* Has to be memory form, don't modify to use an register */
+	alternative_input("",
+		"pushq %[kernelds]; verw (%%rsp) ; addq $8,%%rsp \n",
+		X86_FEATURE_MB_CLEAR,
+		[kernelds] "i" (__KERNEL_DS));
+}
+
+EXPORT_SYMBOL(clear_cpu_buffers_idle);
+
 #ifdef CONFIG_SYSFS
 
 #define L1TF_DEFAULT_MSG "Mitigation: PTE Inversion"
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index d9b71924c23c..5cfd61b40f92 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -159,6 +159,7 @@ void kvm_async_pf_task_wait(u32 token, int interrupt_kernel)
 			/*
 			 * We cannot reschedule. So halt.
 			 */
+			clear_cpu_buffers_idle();
 			native_safe_halt();
 			local_irq_disable();
 		}
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c93fcfdf1673..0ab1c580d36f 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -523,6 +523,8 @@ void stop_this_cpu(void *dummy)
 	disable_local_APIC();
 	mcheck_cpu_clear(this_cpu_ptr(&cpu_info));
 
+	clear_cpu_buffers_idle();
+
 	/*
 	 * Use wbinvd on processors that support SME. This provides support
 	 * for performing a successful kexec when going from SME inactive
@@ -609,6 +611,8 @@ static __cpuidle void mwait_idle(void)
 			mb(); /* quirk */
 		}
 
+		clear_cpu_buffers_idle();
+
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		if (!need_resched())
 			__sti_mwait(0, 0);
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index f02ecaf97904..a0b08b59be3f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1660,6 +1660,7 @@ void hlt_play_dead(void)
 		wbinvd();
 
 	while (1) {
+		clear_cpu_buffers_idle();
 		native_halt();
 		/*
 		 * If NMI wants to wake up CPU0, start CPU0.
-- 
2.17.2

[MODERATED] [PATCH 4/4] RFC-MDSv1 2

[Andi Kleen]

Report mds mitigation state in sysfs vulnerabilities.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 16 ++++++++++++++++
 drivers/base/cpu.c         |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index e220453a1350..404489db0180 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -871,6 +871,16 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
 		if (boot_cpu_has(X86_FEATURE_L1TF_PTEINV))
 			return l1tf_show_state(buf);
 		break;
+
+	case X86_BUG_MDS:
+		/* Assumes Hypervisor exposed HT state to us if in guest */
+		if (boot_cpu_has(X86_FEATURE_MB_CLEAR)) {
+			if (cpu_smt_control != CPU_SMT_ENABLED)
+				return sprintf(buf, "Mitigation: MB_CLEAR\n");
+			return sprintf(buf, "Mitigation: MB_CLEAR, HT vulnerable\n");
+		}
+		return sprintf(buf, "Vulnerable\n");
+
 	default:
 		break;
 	}
@@ -902,4 +912,10 @@ ssize_t cpu_show_l1tf(struct device *dev, struct device_attribute *attr, char *b
 {
 	return cpu_show_common(dev, attr, buf, X86_BUG_L1TF);
 }
+
+ssize_t cpu_show_mds(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return cpu_show_common(dev, attr, buf, X86_BUG_MDS);
+}
+
 #endif
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index eb9443d5bae1..2fd6ca1021c2 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -546,11 +546,18 @@ ssize_t __weak cpu_show_l1tf(struct device *dev,
 	return sprintf(buf, "Not affected\n");
 }
 
+ssize_t __weak cpu_show_mds(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Not affected\n");
+}
+
 static DEVICE_ATTR(meltdown, 0444, cpu_show_meltdown, NULL);
 static DEVICE_ATTR(spectre_v1, 0444, cpu_show_spectre_v1, NULL);
 static DEVICE_ATTR(spectre_v2, 0444, cpu_show_spectre_v2, NULL);
 static DEVICE_ATTR(spec_store_bypass, 0444, cpu_show_spec_store_bypass, NULL);
 static DEVICE_ATTR(l1tf, 0444, cpu_show_l1tf, NULL);
+static DEVICE_ATTR(mds, 0444, cpu_show_mds, NULL);
 
 static struct attribute *cpu_root_vulnerabilities_attrs[] = {
 	&dev_attr_meltdown.attr,
@@ -558,6 +565,7 @@ static struct attribute *cpu_root_vulnerabilities_attrs[] = {
 	&dev_attr_spectre_v2.attr,
 	&dev_attr_spec_store_bypass.attr,
 	&dev_attr_l1tf.attr,
+	&dev_attr_mds.attr,
 	NULL
 };
 
-- 
2.17.2

[MODERATED] Re: [PATCH 4/4] RFC-MDSv1 2

[Greg KH]

On Sun, Nov 11, 2018 at 07:41:04PM -0800, speck for Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> Subject:  x86/speculation/mds: Add sysfs reporting
> 
> Report mds mitigation state in sysfs vulnerabilities.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 16 ++++++++++++++++
>  drivers/base/cpu.c         |  8 ++++++++
>  2 files changed, 24 insertions(+)

No Documentation/ABI/ update as well?

And your subject lines are odd, what order are these to be applied in,
the [PATCH N/4] order of the "v1 N" order at the end of the line?

thanks,

greg k-h

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On 12/11/2018 03:41, speck for Andi Kleen wrote:
> @@ -329,6 +331,12 @@ For 32-bit we have the following conventions - kernel is built with
>  
>  #endif
>  
> +.macro EXIT_MDS
> +	/* Clear CPU buffers that could leak. Instruction must be in memory form. */
> +	ALTERNATIVE "", __stringify(pushq $__USER_DS ; verw (%rsp) ; addq $8, %rsp),\
> +		 X86_FEATURE_MB_CLEAR

The code sample from the slides is push $0.  Here, you are forcing an
implicit access into the GDT by using a non-NUL selector, which is a
compound performance hit.

What I am considering for Xen is to have a dedicated uint16_t
nul_selector and referencing that via a RIP-relative access, which also
disassembles to something obvious.

~Andrew

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Andi Kleen]

On Tue, Nov 13, 2018 at 12:07:47AM +0000, speck for Andrew Cooper wrote:
> On 12/11/2018 03:41, speck for Andi Kleen wrote:
> > @@ -329,6 +331,12 @@ For 32-bit we have the following conventions - kernel is built with
> >  
> >  #endif
> >  
> > +.macro EXIT_MDS
> > +	/* Clear CPU buffers that could leak. Instruction must be in memory form. */
> > +	ALTERNATIVE "", __stringify(pushq $__USER_DS ; verw (%rsp) ; addq $8, %rsp),\
> > +		 X86_FEATURE_MB_CLEAR
> 
> The code sample from the slides is push $0.  Here, you are forcing an
> implicit access into the GDT by using a non-NUL selector, which is a
> compound performance hit.

Actually it should be the same as 0. I think the slides just use $0 because
they don't know which OS the sequence would be used on.

-Andi

[MODERATED] Re: [PATCH 4/4] RFC-MDSv1 2

[Andi Kleen]

On Mon, Nov 12, 2018 at 02:36:05PM -0800, speck for Greg KH wrote:
> On Sun, Nov 11, 2018 at 07:41:04PM -0800, speck for Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > Subject:  x86/speculation/mds: Add sysfs reporting
> > 
> > Report mds mitigation state in sysfs vulnerabilities.
> > 
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/bugs.c | 16 ++++++++++++++++
> >  drivers/base/cpu.c         |  8 ++++++++
> >  2 files changed, 24 insertions(+)
> 
> No Documentation/ABI/ update as well?

Ok.

> 
> And your subject lines are odd, what order are these to be applied in,
> the [PATCH N/4] order of the "v1 N" order at the end of the line?

I'm not sure where the v1 N came from, must be from Thomas' speck script?

The intended order in [PATCH N/x].

-Andi

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Jiri Kosina]

On Sun, 11 Nov 2018, speck for Andi Kleen wrote:

> We don't need to do this for guests because the L1TF cache flush will 
> implicitely do the same flushing, and is automatically selected on MDS 
> affected systems.

Just to make sure, as there has been some confusion around this last time 
this has been discussed -- for the above to be true, ucode update is 
necessary anyway, right? The L1D flush, as implemented in the current 
publicly available ucode, doesn't flush the microarchitectural buffers, 
that is only coming with ucode that will provide enhanced verw semantics, 
right?

Thanks,

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Luck, Tony]

On Tue, Nov 13, 2018 at 09:47:57PM +0100, speck for Jiri Kosina wrote:
> Just to make sure, as there has been some confusion around this last time 
> this has been discussed -- for the above to be true, ucode update is 
> necessary anyway, right? The L1D flush, as implemented in the current 
> publicly available ucode, doesn't flush the microarchitectural buffers, 
> that is only coming with ucode that will provide enhanced verw semantics, 
> right?

Correct. Yuo need a microcode that enumerates MB_CLEAR to tell
you that your L1D flush will also flush the micro-architecture
structures for MDS.  The "old" microcode that was released for
L1TF doesn't do this extra flush.

-Tony

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On 13/11/2018 01:35, speck for Andi Kleen wrote:
> On Tue, Nov 13, 2018 at 12:07:47AM +0000, speck for Andrew Cooper wrote:
>> On 12/11/2018 03:41, speck for Andi Kleen wrote:
>>> @@ -329,6 +331,12 @@ For 32-bit we have the following conventions - kernel is built with
>>>  
>>>  #endif
>>>  
>>> +.macro EXIT_MDS
>>> +	/* Clear CPU buffers that could leak. Instruction must be in memory form. */
>>> +	ALTERNATIVE "", __stringify(pushq $__USER_DS ; verw (%rsp) ; addq $8, %rsp),\
>>> +		 X86_FEATURE_MB_CLEAR
>> The code sample from the slides is push $0.  Here, you are forcing an
>> implicit access into the GDT by using a non-NUL selector, which is a
>> compound performance hit.
> Actually it should be the same as 0. I think the slides just use $0 because
> they don't know which OS the sequence would be used on.

__USER_DS is GDT slot 15 in 32bit, and slot 5 in 64bit.

Either way, above and beyond the microarchitectural flush, you're
causing a implicit memory read into the GDT for VERW to fulfil its
architectural behaviour.

I was under the impression that the recommendation to use a NUL selector
was specifically to avoid this second read.

~Andrew

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Andi Kleen]

On Tue, Nov 13, 2018 at 09:47:57PM +0100, speck for Jiri Kosina wrote:
> On Sun, 11 Nov 2018, speck for Andi Kleen wrote:
> 
> > We don't need to do this for guests because the L1TF cache flush will 
> > implicitely do the same flushing, and is automatically selected on MDS 
> > affected systems.
> 
> Just to make sure, as there has been some confusion around this last time 
> this has been discussed -- for the above to be true, ucode update is 
> necessary anyway, right? The L1D flush, as implemented in the current 
> publicly available ucode, doesn't flush the microarchitectural buffers, 
> that is only coming with ucode that will provide enhanced verw semantics, 
> right?

Yes that is correct.

-Andi

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Andi Kleen]

> I was under the impression that the recommendation to use a NUL selector
> was specifically to avoid this second read.

No the rationale for the recommendation is to avoid mispredictions 
in the microcode. But the two are equivalent.

-Andi

[MODERATED] Re: [PATCH 2/4] RFC-MDSv1 0

[Andrew Cooper]

On 14/11/2018 04:04, speck for Andi Kleen wrote:
>> I was under the impression that the recommendation to use a NUL selector
>> was specifically to avoid this second read.
> No the rationale for the recommendation is to avoid mispredictions 
> in the microcode. But the two are equivalent.

After a discussion with Jason where (again) he suggested the use of %ds
rather than NUL, I went back and extended my micro-benchmark.

All other things being equal and cache-hot, a VERW of %ds really is
faster NUL.  (Net execution time is about 2/3 as long as recorded by two
rdtscp's, not accounting for any systematic timing errors).  This is
just against some of my development boxes - I'm still waiting on the
correct CPU to use the prototype microcode with additional VERW goodies.

Either way - I'm surprised by these results, but %ds does looks to be
the quickest option.  Apologies for the noise on this thread.

~Andrew