The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 785bf1ab58aa1f89a5dfcb17b682b7089d69c34f Gitweb: https://git.kernel.org/tip/785bf1ab58aa1f89a5dfcb17b682b7089d69c34f Author: Pawan Gupta pawan.kumar.gupta@linux.intel.com AuthorDate: Thu, 26 Sep 2024 09:10:31 -07:00 Committer: Dave Hansen dave.hansen@linux.intel.com CommitterDate: Tue, 08 Oct 2024 15:19:21 -07:00
x86/bugs: Use code segment selector for VERW operand
Robert Gill reported below #GP in 32-bit mode when dosemu software was executing vm86() system call:
general protection fault: 0000 [#1] PREEMPT SMP CPU: 4 PID: 4610 Comm: dosemu.bin Not tainted 6.6.21-gentoo-x86 #1 Hardware name: Dell Inc. PowerEdge 1950/0H723K, BIOS 2.7.0 10/30/2010 EIP: restore_all_switch_stack+0xbe/0xcf EAX: 00000000 EBX: 00000000 ECX: 00000000 EDX: 00000000 ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: ff8affdc DS: 0000 ES: 0000 FS: 0000 GS: 0033 SS: 0068 EFLAGS: 00010046 CR0: 80050033 CR2: 00c2101c CR3: 04b6d000 CR4: 000406d0 Call Trace: show_regs+0x70/0x78 die_addr+0x29/0x70 exc_general_protection+0x13c/0x348 exc_bounds+0x98/0x98 handle_exception+0x14d/0x14d exc_bounds+0x98/0x98 restore_all_switch_stack+0xbe/0xcf exc_bounds+0x98/0x98 restore_all_switch_stack+0xbe/0xcf
This only happens in 32-bit mode when VERW based mitigations like MDS/RFDS are enabled. This is because segment registers with an arbitrary user value can result in #GP when executing VERW. Intel SDM vol. 2C documents the following behavior for VERW instruction:
#GP(0) - If a memory operand effective address is outside the CS, DS, ES, FS, or GS segment limit.
CLEAR_CPU_BUFFERS macro executes VERW instruction before returning to user space. Use %cs selector to reference VERW operand. This ensures VERW will not #GP for an arbitrary user %ds.
Fixes: a0e2dab44d22 ("x86/entry_32: Add VERW just before userspace transition") Reported-by: Robert Gill rtgill82@gmail.com Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com Cc: stable@vger.kernel.org # 5.10+ Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218707 Closes: https://lore.kernel.org/all/8c77ccfd-d561-45a1-8ed5-6b75212c7a58@leemhuis.in... Suggested-by: Dave Hansen dave.hansen@linux.intel.com Suggested-by: Brian Gerst brgerst@gmail.com Signed-off-by: Pawan Gupta pawan.kumar.gupta@linux.intel.com --- arch/x86/include/asm/nospec-branch.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h index ff5f1ec..96b410b 100644 --- a/arch/x86/include/asm/nospec-branch.h +++ b/arch/x86/include/asm/nospec-branch.h @@ -323,7 +323,16 @@ * Note: Only the memory operand variant of VERW clears the CPU buffers. */ .macro CLEAR_CPU_BUFFERS - ALTERNATIVE "", __stringify(verw _ASM_RIP(mds_verw_sel)), X86_FEATURE_CLEAR_CPU_BUF +#ifdef CONFIG_X86_64 + ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_CLEAR_CPU_BUF +#else + /* + * In 32bit mode, the memory operand must be a %cs reference. The data + * segments may not be usable (vm86 mode), and the stack segment may not + * be flat (ESPFIX32). + */ + ALTERNATIVE "", "verw %cs:mds_verw_sel", X86_FEATURE_CLEAR_CPU_BUF +#endif .endm
#ifdef CONFIG_X86_64
On Tue, Oct 08, 2024 at 10:45:36PM -0000, tip-bot2 for Pawan Gupta wrote:
.macro CLEAR_CPU_BUFFERS
- ALTERNATIVE "", __stringify(verw _ASM_RIP(mds_verw_sel)), X86_FEATURE_CLEAR_CPU_BUF
+#ifdef CONFIG_X86_64
- ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_CLEAR_CPU_BUF
+#else
- /*
* In 32bit mode, the memory operand must be a %cs reference. The data* segments may not be usable (vm86 mode), and the stack segment may not* be flat (ESPFIX32).*/- ALTERNATIVE "", "verw %cs:mds_verw_sel", X86_FEATURE_CLEAR_CPU_BUF
+#endif
So why didn't we ifdef the "verw mds_verw_sel(%rip)" and "verw %cs:mds_verw_sel" macro argument instead of adding more bigger ugly ifdeffery?
On Wed, Oct 09, 2024 at 08:11:02AM +0200, Borislav Petkov wrote:
On Tue, Oct 08, 2024 at 10:45:36PM -0000, tip-bot2 for Pawan Gupta wrote:
.macro CLEAR_CPU_BUFFERS
- ALTERNATIVE "", __stringify(verw _ASM_RIP(mds_verw_sel)), X86_FEATURE_CLEAR_CPU_BUF
+#ifdef CONFIG_X86_64
- ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_CLEAR_CPU_BUF
+#else
- /*
* In 32bit mode, the memory operand must be a %cs reference. The data* segments may not be usable (vm86 mode), and the stack segment may not* be flat (ESPFIX32).*/- ALTERNATIVE "", "verw %cs:mds_verw_sel", X86_FEATURE_CLEAR_CPU_BUF
+#endif
So why didn't we ifdef the "verw mds_verw_sel(%rip)" and "verw %cs:mds_verw_sel" macro argument instead of adding more bigger ugly ifdeffery?
You need ifdeffery either way around, either directly like this or for that macro. This is simple and straight forward.
On Wed, Oct 09, 2024 at 09:34:37AM +0200, Peter Zijlstra wrote:
You need ifdeffery either way around, either directly like this or for that macro. This is simple and straight forward.
Nothing in this file full of macros is simple. In any case, I would've done this as the ifdeffery is shorter and the macro is simpler. We have this coding pattern in a lot of headers, abstracting 32-bit vs 64-bit machine details, and it is a very common and familiar one:
/* * In 32bit mode, the memory operand must be a %cs reference. The data * segments may not be usable (vm86 mode), and the stack segment may not be * flat (ESPFIX32). */ #ifdef CONFIG_X86_64 #define VERW_ARG "verw mds_verw_sel(%rip)" #else /* CONFIG_X86_32 */ #define VERW_ARG "verw %cs:mds_verw_sel" #endif
/* * Macro 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 CLEAR_CPU_BUFFERS ALTERNATIVE "", VERW_ARG, X86_FEATURE_CLEAR_CPU_BUF .endm
On 09/10/2024 10:32 am, Borislav Petkov wrote:
On Wed, Oct 09, 2024 at 09:34:37AM +0200, Peter Zijlstra wrote:
You need ifdeffery either way around, either directly like this or for that macro. This is simple and straight forward.
Nothing in this file full of macros is simple. In any case, I would've done this as the ifdeffery is shorter and the macro is simpler. We have this coding pattern in a lot of headers, abstracting 32-bit vs 64-bit machine details, and it is a very common and familiar one:
/*
- In 32bit mode, the memory operand must be a %cs reference. The data
- segments may not be usable (vm86 mode), and the stack segment may not be
- flat (ESPFIX32).
*/ #ifdef CONFIG_X86_64 #define VERW_ARG "verw mds_verw_sel(%rip)" #else /* CONFIG_X86_32 */ #define VERW_ARG "verw %cs:mds_verw_sel" #endif
/*
- Macro 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 CLEAR_CPU_BUFFERS ALTERNATIVE "", VERW_ARG, X86_FEATURE_CLEAR_CPU_BUF .endm
I'll bite. Why do you think this form is is better?
You've now got VERW_ARG leaking across the whole kernel, and a layer of obfuscatio^W indirection in CLEAR_CPU_BUFFERS.
Admittedly, when I wrote this fragment as a suggestion[1], the 32bit comment was in the main comment because there really is no need for it to be separate.
But abstracting away VERW_ARG like this hampers legibility/clarity, rather than improving it IMO.
~Andrew
[1] https://lore.kernel.org/lkml/5703f2d8-7ca0-4f01-a954-c0eb1de930b4@citrix.com...
On Wed, Oct 09, 2024 at 11:24:19AM +0100, Andrew Cooper wrote:
I'll bite. Why do you think this form is is better?
Smaller, shorter ifdeffery block. An example speaks more than 1000 words:
arch/x86/include/asm/asm.h
You've now got VERW_ARG leaking across the whole kernel, and a layer of obfuscatio^W indirection in CLEAR_CPU_BUFFERS.
We have that all around the kernel anyway.
Admittedly, when I wrote this fragment as a suggestion[1], the 32bit comment was in the main comment because there really is no need for it to be separate.
But abstracting away VERW_ARG like this hampers legibility/clarity, rather than improving it IMO.
I guess we see it differently.
I don't care all that much to continue this - I'll just say that having the CLEAR_CPU_BUFFERS macro text simpler and putting the argument complexity abstracted away in macros reads better to me.
Oh well.
On 10/8/24 23:11, Borislav Petkov wrote:
.macro CLEAR_CPU_BUFFERS
- ALTERNATIVE "", __stringify(verw _ASM_RIP(mds_verw_sel)), X86_FEATURE_CLEAR_CPU_BUF
+#ifdef CONFIG_X86_64
- ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_CLEAR_CPU_BUF
+#else
- /*
* In 32bit mode, the memory operand must be a %cs reference. The data* segments may not be usable (vm86 mode), and the stack segment may not* be flat (ESPFIX32).*/- ALTERNATIVE "", "verw %cs:mds_verw_sel", X86_FEATURE_CLEAR_CPU_BUF
+#endif
So why didn't we ifdef the "verw mds_verw_sel(%rip)" and "verw %cs:mds_verw_sel" macro argument instead of adding more bigger ugly ifdeffery?
I'm not jumping for joy about how it looks, but I applied it because it's good enough and the regression was about to get its driver's license. ;)
I did spend some time noodling to come up with _some_ common 32/64-bit implementation, but 32-bit is just too special of a snowflake.
linux-stable-mirror@lists.linaro.org
-
Andrew Cooper -
Borislav Petkov -
Dave Hansen -
Peter Zijlstra -
tip-bot2 for Pawan Gupta