[tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[tip-bot2 for Pawan Gupta]

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_64

Re: [tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[Peter Zijlstra]

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.

[tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[tip-bot2 for Pawan Gupta]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     e4d2102018542e3ae5e297bc6e229303abff8a0f
Gitweb:        https://git.kernel.org/tip/e4d2102018542e3ae5e297bc6e229303abff8a0f
Author:        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
AuthorDate:    Thu, 26 Sep 2024 09:10:31 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 09 Oct 2024 09:42:30 +02: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.

[ mingo: Fixed the SOB chain. ]

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>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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

Re: [tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[Borislav Petkov]

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

Re: [tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[Andrew Cooper]

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/

Re: [tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[Borislav Petkov]

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

Re: [tip: x86/urgent] x86/bugs: Use code segment selector for VERW operand

[Dave Hansen]

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.