arch/x86/include/asm/nospec-branch.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
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.info/
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_64On 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?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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.
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
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
© 2016 - 2024 Red Hat, Inc.