MDS mitigation requires clearing the CPU buffers before returning to
user. This needs to be done late in the exit-to-user path. Current
location of VERW leaves a possibility of kernel data ending up in CPU
buffers for memory accesses done after VERW such as:
1. Kernel data accessed by an NMI between VERW and return-to-user can
remain in CPU buffers since NMI returning to kernel does not
execute VERW to clear CPU buffers.
2. Alyssa reported that after VERW is executed,
CONFIG_GCC_PLUGIN_STACKLEAK=y scrubs the stack used by a system
call. Memory accesses during stack scrubbing can move kernel stack
contents into CPU buffers.
3. When caller saved registers are restored after a return from
function executing VERW, the kernel stack accesses can remain in
CPU buffers(since they occur after VERW).
To fix this VERW needs to be moved very late in exit-to-user path.
In preparation for moving VERW to entry/exit asm code, create macros
that can be used in asm. Also make VERW patching depend on a new feature
flag X86_FEATURE_CLEAR_CPU_BUF.
Reported-by: Alyssa Milburn <[email protected]>
Suggested-by: Andrew Cooper <[email protected]>
Suggested-by: Peter Zijlstra <[email protected]>
Cc: [email protected]
Signed-off-by: Pawan Gupta <[email protected]>
---
arch/x86/entry/entry.S | 22 ++++++++++++++++++++++
arch/x86/include/asm/cpufeatures.h | 2 +-
arch/x86/include/asm/nospec-branch.h | 17 +++++++++++++++++
3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S
index 8c8d38f0cb1d..bd8e77c5a375 100644
--- a/arch/x86/entry/entry.S
+++ b/arch/x86/entry/entry.S
@@ -6,6 +6,9 @@
#include <linux/export.h>
#include <linux/linkage.h>
#include <asm/msr-index.h>
+#include <asm/unwind_hints.h>
+#include <asm/segment.h>
+#include <asm/cache.h>
.pushsection .noinstr.text, "ax"
@@ -20,3 +23,22 @@ SYM_FUNC_END(entry_ibpb)
EXPORT_SYMBOL_GPL(entry_ibpb);
.popsection
+
+/*
+ * Defines the VERW operand that is disguised as entry code so that
+ * it can be referenced with KPTI enabled. This ensures VERW can be
+ * used late in exit-to-user path after page tables are switched.
+ */
+.pushsection .entry.text, "ax"
+
+.align L1_CACHE_BYTES, 0xcc
+SYM_CODE_START_NOALIGN(mds_verw_sel)
+ UNWIND_HINT_UNDEFINED
+ ANNOTATE_NOENDBR
+ .word __KERNEL_DS
+.align L1_CACHE_BYTES, 0xcc
+SYM_CODE_END(mds_verw_sel);
+/* For KVM */
+EXPORT_SYMBOL_GPL(mds_verw_sel);
+
+.popsection
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index fdf723b6f6d0..2b62cdd8dd12 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -95,7 +95,7 @@
#define X86_FEATURE_SYSENTER32 ( 3*32+15) /* "" sysenter in IA32 userspace */
#define X86_FEATURE_REP_GOOD ( 3*32+16) /* REP microcode works well */
#define X86_FEATURE_AMD_LBR_V2 ( 3*32+17) /* AMD Last Branch Record Extension Version 2 */
-/* FREE, was #define X86_FEATURE_LFENCE_RDTSC ( 3*32+18) "" LFENCE synchronizes RDTSC */
+#define X86_FEATURE_CLEAR_CPU_BUF ( 3*32+18) /* "" Clear CPU buffers using VERW */
#define X86_FEATURE_ACC_POWER ( 3*32+19) /* AMD Accumulated Power Mechanism */
#define X86_FEATURE_NOPL ( 3*32+20) /* The NOPL (0F 1F) instructions */
#define X86_FEATURE_ALWAYS ( 3*32+21) /* "" Always-present feature */
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 262e65539f83..ec85dfe67123 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -315,6 +315,21 @@
#endif
.endm
+/*
+ * Macros to execute VERW instruction that mitigate transient data sampling
+ * attacks such as MDS. On affected systems a microcode update overloaded VERW
+ * instruction to also clear the CPU buffers. VERW clobbers CFLAGS.ZF.
+ *
+ * Note: Only the memory operand variant of VERW clears the CPU buffers.
+ */
+.macro EXEC_VERW
+ verw _ASM_RIP(mds_verw_sel)
+.endm
+
+.macro CLEAR_CPU_BUFFERS
+ ALTERNATIVE "", __stringify(EXEC_VERW), X86_FEATURE_CLEAR_CPU_BUF
+.endm
+
#else /* __ASSEMBLY__ */
#define ANNOTATE_RETPOLINE_SAFE \
@@ -536,6 +551,8 @@ DECLARE_STATIC_KEY_FALSE(switch_mm_cond_l1d_flush);
DECLARE_STATIC_KEY_FALSE(mmio_stale_data_clear);
+extern u16 mds_verw_sel;
+
#include <asm/segment.h>
/**
--
2.34.1
On Sun, Feb 04, 2024 at 11:18:59PM -0800, Pawan Gupta wrote:
> .popsection
> +
> +/*
> + * Defines the VERW operand that is disguised as entry code so that
"Define..."
> + * it can be referenced with KPTI enabled. This ensures VERW can be
"Ensure..."
But committer can fix those.
> + * used late in exit-to-user path after page tables are switched.
> + */
> +.pushsection .entry.text, "ax"
> +
> +.align L1_CACHE_BYTES, 0xcc
> +SYM_CODE_START_NOALIGN(mds_verw_sel)
> + UNWIND_HINT_UNDEFINED
> + ANNOTATE_NOENDBR
> + .word __KERNEL_DS
> +.align L1_CACHE_BYTES, 0xcc
> +SYM_CODE_END(mds_verw_sel);
> +/* For KVM */
> +EXPORT_SYMBOL_GPL(mds_verw_sel);
> +
> +.popsection
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index fdf723b6f6d0..2b62cdd8dd12 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -95,7 +95,7 @@
> #define X86_FEATURE_SYSENTER32 ( 3*32+15) /* "" sysenter in IA32 userspace */
> #define X86_FEATURE_REP_GOOD ( 3*32+16) /* REP microcode works well */
> #define X86_FEATURE_AMD_LBR_V2 ( 3*32+17) /* AMD Last Branch Record Extension Version 2 */
> -/* FREE, was #define X86_FEATURE_LFENCE_RDTSC ( 3*32+18) "" LFENCE synchronizes RDTSC */
> +#define X86_FEATURE_CLEAR_CPU_BUF ( 3*32+18) /* "" Clear CPU buffers using VERW */
> #define X86_FEATURE_ACC_POWER ( 3*32+19) /* AMD Accumulated Power Mechanism */
> #define X86_FEATURE_NOPL ( 3*32+20) /* The NOPL (0F 1F) instructions */
> #define X86_FEATURE_ALWAYS ( 3*32+21) /* "" Always-present feature */
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 262e65539f83..ec85dfe67123 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -315,6 +315,21 @@
> #endif
> .endm
>
> +/*
> + * Macros to execute VERW instruction that mitigate transient data sampling
> + * attacks such as MDS. On affected systems a microcode update overloaded VERW
> + * instruction to also clear the CPU buffers. VERW clobbers CFLAGS.ZF.
> + *
> + * Note: Only the memory operand variant of VERW clears the CPU buffers.
> + */
> +.macro EXEC_VERW
I think I asked this already:
Why isn't this called simply "VERW"?
There's no better name as this is basically the insn itself...
> + verw _ASM_RIP(mds_verw_sel)
> +.endm
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Feb 09, 2024 at 11:06:02AM -0800, Pawan Gupta wrote:
> (Though, there was a comment on avoiding the macro alltogether, to which
> I replied that it complicates 32-bit.)
Hmm, so this seems to build the respective entry_{32,64}.S TUs fine:
---
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index e81cabcb758f..7d1e5fe66495 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -313,12 +313,9 @@
*
* Note: Only the memory operand variant of VERW clears the CPU buffers.
*/
-.macro EXEC_VERW
- verw _ASM_RIP(mds_verw_sel)
-.endm
.macro CLEAR_CPU_BUFFERS
- ALTERNATIVE "", __stringify(EXEC_VERW), X86_FEATURE_CLEAR_CPU_BUF
+ ALTERNATIVE "", __stringify(verw _ASM_RIP(mds_verw_sel)), X86_FEATURE_CLEAR_CPU_BUF
.endm
#else /* __ASSEMBLY__ */
---
and looking at the asm:
64-bit:
# 317 "./arch/x86/include/asm/nospec-branch.h"
macro CLEAR_CPU_BUFFERS
ALTERNATIVE "", "verw mds_verw_sel (% rip)", ( 3*32+18)
endm
23: 0f 00 2d 00 00 00 00 verw 0x0(%rip) # 2a <.altinstr_replacement+0x2a>
32-bit:
macro CLEAR_CPU_BUFFERS
ALTERNATIVE "", "verw mds_verw_sel", ( 3*32+18)
endm
13d: 0f 00 2d 00 00 00 00 verw 0x0
it makes sense.
So what complications do you mean?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Feb 09, 2024 at 08:57:04PM +0100, Borislav Petkov wrote:
> On Fri, Feb 09, 2024 at 11:06:02AM -0800, Pawan Gupta wrote:
> > (Though, there was a comment on avoiding the macro alltogether, to which
> > I replied that it complicates 32-bit.)
>
> Hmm, so this seems to build the respective entry_{32,64}.S TUs fine:
>
> ---
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index e81cabcb758f..7d1e5fe66495 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -313,12 +313,9 @@
> *
> * Note: Only the memory operand variant of VERW clears the CPU buffers.
> */
> -.macro EXEC_VERW
> - verw _ASM_RIP(mds_verw_sel)
> -.endm
>
> .macro CLEAR_CPU_BUFFERS
> - ALTERNATIVE "", __stringify(EXEC_VERW), X86_FEATURE_CLEAR_CPU_BUF
> + ALTERNATIVE "", __stringify(verw _ASM_RIP(mds_verw_sel)), X86_FEATURE_CLEAR_CPU_BUF
> .endm
> +.macro CLEAR_CPU_BUFFERS
> + ALTERNATIVE "", __stringify(EXEC_VERW), X86_FEATURE_CLEAR_CPU_BUF
> +.endm
This works, thanks.
My mistake that I tried your earlier suggestion without playing with it:
> > +.macro CLEAR_CPU_BUFFERS
> > + ALTERNATIVE "", __stringify(EXEC_VERW), X86_FEATURE_CLEAR_CPU_BUF
> > +.endm
>
> Why can't this simply be:
>
> .macro CLEAR_CPU_BUFFERS
> ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_CLEAR_CPU_BUF
> .endm
>
> without that silly EXEC_VERW macro?
>
> --
> Regards/Gruss,
> Boris.
On Fri, Feb 09, 2024 at 06:28:43PM +0100, Borislav Petkov wrote:
> On Sun, Feb 04, 2024 at 11:18:59PM -0800, Pawan Gupta wrote:
> > .popsection
> > +
> > +/*
> > + * Defines the VERW operand that is disguised as entry code so that
>
> "Define..."
>
> > + * it can be referenced with KPTI enabled. This ensures VERW can be
>
> "Ensure..."
>
> But committer can fix those.
>
> > + * used late in exit-to-user path after page tables are switched.
> > + */
> > +.pushsection .entry.text, "ax"
> > +
> > +.align L1_CACHE_BYTES, 0xcc
> > +SYM_CODE_START_NOALIGN(mds_verw_sel)
> > + UNWIND_HINT_UNDEFINED
> > + ANNOTATE_NOENDBR
> > + .word __KERNEL_DS
> > +.align L1_CACHE_BYTES, 0xcc
> > +SYM_CODE_END(mds_verw_sel);
> > +/* For KVM */
> > +EXPORT_SYMBOL_GPL(mds_verw_sel);
> > +
> > +.popsection
> > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > index fdf723b6f6d0..2b62cdd8dd12 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -95,7 +95,7 @@
> > #define X86_FEATURE_SYSENTER32 ( 3*32+15) /* "" sysenter in IA32 userspace */
> > #define X86_FEATURE_REP_GOOD ( 3*32+16) /* REP microcode works well */
> > #define X86_FEATURE_AMD_LBR_V2 ( 3*32+17) /* AMD Last Branch Record Extension Version 2 */
> > -/* FREE, was #define X86_FEATURE_LFENCE_RDTSC ( 3*32+18) "" LFENCE synchronizes RDTSC */
> > +#define X86_FEATURE_CLEAR_CPU_BUF ( 3*32+18) /* "" Clear CPU buffers using VERW */
> > #define X86_FEATURE_ACC_POWER ( 3*32+19) /* AMD Accumulated Power Mechanism */
> > #define X86_FEATURE_NOPL ( 3*32+20) /* The NOPL (0F 1F) instructions */
> > #define X86_FEATURE_ALWAYS ( 3*32+21) /* "" Always-present feature */
> > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> > index 262e65539f83..ec85dfe67123 100644
> > --- a/arch/x86/include/asm/nospec-branch.h
> > +++ b/arch/x86/include/asm/nospec-branch.h
> > @@ -315,6 +315,21 @@
> > #endif
> > .endm
> >
> > +/*
> > + * Macros to execute VERW instruction that mitigate transient data sampling
> > + * attacks such as MDS. On affected systems a microcode update overloaded VERW
> > + * instruction to also clear the CPU buffers. VERW clobbers CFLAGS.ZF.
> > + *
> > + * Note: Only the memory operand variant of VERW clears the CPU buffers.
> > + */
> > +.macro EXEC_VERW
>
> I think I asked this already:
Sorry I can't seem to find that on lore.
(Though, there was a comment on avoiding the macro alltogether, to which
I replied that it complicates 32-bit.)
> Why isn't this called simply "VERW"?
>
> There's no better name as this is basically the insn itself...
Agree.
> > + verw _ASM_RIP(mds_verw_sel)
But, in this case the instruction needs a special operand, and the build
fails with the macro name VERW:
AS arch/x86/entry/entry.o
AS arch/x86/entry/entry_64.o
arch/x86/entry/entry_64.S: Assembler messages:
arch/x86/entry/entry_64.S:164: Error: too many positional arguments
arch/x86/entry/entry_64.S:577: Error: too many positional arguments
arch/x86/entry/entry_64.S:728: Error: too many positional arguments
arch/x86/entry/entry_64.S:1479: Error: too many positional arguments
make[4]: *** [scripts/Makefile.build:361: arch/x86/entry/entry_64.o] Error 1
make[3]: *** [scripts/Makefile.build:481: arch/x86/entry] Error 2
make[2]: *** [scripts/Makefile.build:481: arch/x86] Error 2
> > +.endm
Perhaps s/EXEC_VERW/_VERW/ ?
Thanks for the review.