Re: system locks up with CONFIG_SLS=Y; 5.17.0-rc

Re: system locks up with CONFIG_SLS=Y; 5.17.0-rc

[Borislav Petkov]

+ kvm folks.

On Wed, Mar 16, 2022 at 08:15:43PM +0000, Jamie Heilman wrote:
> It does indeed!

Thanks, here's a proper patch. I've added your Reported-by and Tested-by
tags - I hope that's ok.

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 16 Mar 2022 22:05:52 +0100
Subject: [PATCH] kvm/emulate: Fix SETcc emulation function offsets with SLS

The commit in Fixes started adding INT3 after RETs as a mitigation
against straight-line speculation.

The fastop SETcc implementation in kvm's insn emulator uses macro magic
to generate all possible SETcc functions and to jump to them when
emulating the respective instruction.

However, it hardcodes the size and alignment of those functions to 4: a
three-byte SETcc insn and a single-byte RET. BUT, with SLS, there's an
INT3 that gets slapped after the RET, which brings the whole scheme out
of alignment:

  15:   0f 90 c0                seto   %al
  18:   c3                      ret
  19:   cc                      int3
  1a:   0f 1f 00                nopl   (%rax)
  1d:   0f 91 c0                setno  %al
  20:   c3                      ret
  21:   cc                      int3
  22:   0f 1f 00                nopl   (%rax)
  25:   0f 92 c0                setb   %al
  28:   c3                      ret
  29:   cc                      int3

and this explodes like this:

  int3: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 2435 Comm: qemu-system-x86 Not tainted 5.17.0-rc8-sls #1
  Hardware name: Dell Inc. Precision WorkStation T3400  /0TP412, BIOS A14 04/30/2012
  RIP: 0010:setc+0x5/0x8 [kvm]
  Code: 00 00 0f 1f 00 0f b6 05 43 24 06 00 c3 cc 0f 1f 80 00 00 00 00 0f 90 c0 c3 cc 0f 1f 00 0f 91 c0 c3 cc 0f 1f 00 0f 92 c0 c3 cc <0f> 1f 00 0f 93 c0 c3 cc 0f 1f 00 0f 94 c0 c3 cc 0f 1f 00 0f 95 c0
  Call Trace:
   <TASK>
   ? x86_emulate_insn [kvm]
   ? x86_emulate_instruction [kvm]
   ? vmx_handle_exit [kvm_intel]
   ? kvm_arch_vcpu_ioctl_run [kvm]
   ? kvm_vcpu_ioctl [kvm]
   ? __x64_sys_ioctl
   ? do_syscall_64+0x40/0xa0
   ? entry_SYSCALL_64_after_hwframe+0x44/0xae
   </TASK>

Align everything to 8 and use a macro for that instead of hard-coding
naked numbers.

Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
Reported-by: Jamie Heilman <jamie@audible.transient.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
Tested-by: Jamie Heilman <jamie@audible.transient.net>
Link: https://lore.kernel.org/r/YjGzJwjrvxg5YZ0Z@audible.transient.net
---
 arch/x86/kvm/emulate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f667bd8df533..e88ce4171c4a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -430,8 +430,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	FOP_END
 
 /* Special case for SETcc - 1 instruction per cc */
+
+#define SETCC_ALIGN 8
+
 #define FOP_SETCC(op) \
-	".align 4 \n\t" \
+	".align " __stringify(SETCC_ALIGN) " \n\t" \
 	".type " #op ", @function \n\t" \
 	#op ": \n\t" \
 	ASM_ENDBR \
@@ -1049,7 +1052,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
 static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
 {
 	u8 rc;
-	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
+	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
 	asm("push %[flags]; popf; " CALL_NOSPEC
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

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

Re: system locks up with CONFIG_SLS=Y; 5.17.0-rc

[Jamie Heilman]

Borislav Petkov wrote:
> + kvm folks.
> 
> On Wed, Mar 16, 2022 at 08:15:43PM +0000, Jamie Heilman wrote:
> > It does indeed!
> 
> Thanks, here's a proper patch. I've added your Reported-by and Tested-by
> tags - I hope that's ok.

Yeah, absolutely.

-- 
Jamie Heilman                     http://audible.transient.net/~jamie/

Re: system locks up with CONFIG_SLS=Y; 5.17.0-rc

[Peter Zijlstra]

On Wed, Mar 16, 2022 at 10:23:37PM +0100, Borislav Petkov wrote:
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f667bd8df533..e88ce4171c4a 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -430,8 +430,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>  	FOP_END
>  
>  /* Special case for SETcc - 1 instruction per cc */
> +
> +#define SETCC_ALIGN 8

I'd suggest writing that like:

	#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))

That way people can enjoy smaller text when they don't do the whole SLS
thing.... Also, it appears to me I added an ENDBR to this in
tip/x86/core, well, that needs fixing too. Tomorrow tho.

> +
>  #define FOP_SETCC(op) \
> -	".align 4 \n\t" \
> +	".align " __stringify(SETCC_ALIGN) " \n\t" \
>  	".type " #op ", @function \n\t" \
>  	#op ": \n\t" \
>  	ASM_ENDBR \
> @@ -1049,7 +1052,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
>  static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
>  {
>  	u8 rc;
> -	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
> +	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
>  
>  	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
>  	asm("push %[flags]; popf; " CALL_NOSPEC
> -- 
> 2.29.2
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

[PATCH -v1.1] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Borislav Petkov]

On Wed, Mar 16, 2022 at 11:02:01PM +0100, Peter Zijlstra wrote:
> I'd suggest writing that like:
> 
> 	#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
> 
> That way people can enjoy smaller text when they don't do the whole SLS
> thing.... Also, it appears to me I added an ENDBR to this in
> tip/x86/core, well, that needs fixing too. Tomorrow tho.

Done.

Jamie, I'd appreciate testing this one too, pls, just in case.

Thx.

---
From: Borislav Petkov <bp@suse.de>

The commit in Fixes started adding INT3 after RETs as a mitigation
against straight-line speculation.

The fastop SETcc implementation in kvm's insn emulator uses macro magic
to generate all possible SETcc functions and to jump to them when
emulating the respective instruction.

However, it hardcodes the size and alignment of those functions to 4: a
three-byte SETcc insn and a single-byte RET. BUT, with SLS, there's an
INT3 that gets slapped after the RET, which brings the whole scheme out
of alignment:

  15:   0f 90 c0                seto   %al
  18:   c3                      ret
  19:   cc                      int3
  1a:   0f 1f 00                nopl   (%rax)
  1d:   0f 91 c0                setno  %al
  20:   c3                      ret
  21:   cc                      int3
  22:   0f 1f 00                nopl   (%rax)
  25:   0f 92 c0                setb   %al
  28:   c3                      ret
  29:   cc                      int3

and this explodes like this:

  int3: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 2435 Comm: qemu-system-x86 Not tainted 5.17.0-rc8-sls #1
  Hardware name: Dell Inc. Precision WorkStation T3400  /0TP412, BIOS A14 04/30/2012
  RIP: 0010:setc+0x5/0x8 [kvm]
  Code: 00 00 0f 1f 00 0f b6 05 43 24 06 00 c3 cc 0f 1f 80 00 00 00 00 0f 90 c0 c3 cc 0f 1f 00 0f 91 c0 c3 cc 0f 1f 00 0f 92 c0 c3 cc <0f> 1f 00 0f 93 c0 c3 cc 0f 1f 00 0f 94 c0 c3 cc 0f 1f 00 0f 95 c0
  Call Trace:
   <TASK>
   ? x86_emulate_insn [kvm]
   ? x86_emulate_instruction [kvm]
   ? vmx_handle_exit [kvm_intel]
   ? kvm_arch_vcpu_ioctl_run [kvm]
   ? kvm_vcpu_ioctl [kvm]
   ? __x64_sys_ioctl
   ? do_syscall_64+0x40/0xa0
   ? entry_SYSCALL_64_after_hwframe+0x44/0xae
   </TASK>

Raise the alignment value when SLS is enabled and use a macro for that
instead of hard-coding naked numbers.

Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
Reported-by: Jamie Heilman <jamie@audible.transient.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/YjGzJwjrvxg5YZ0Z@audible.transient.net
---
 arch/x86/kvm/emulate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f667bd8df533..01c0a02f4004 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -430,8 +430,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	FOP_END
 
 /* Special case for SETcc - 1 instruction per cc */
+
+#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
+
 #define FOP_SETCC(op) \
-	".align 4 \n\t" \
+	".align " __stringify(SETCC_ALIGN) " \n\t" \
 	".type " #op ", @function \n\t" \
 	#op ": \n\t" \
 	ASM_ENDBR \
@@ -1049,7 +1052,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
 static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
 {
 	u8 rc;
-	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
+	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
 	asm("push %[flags]; popf; " CALL_NOSPEC
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

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

[PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Borislav Petkov]

On Thu, Mar 17, 2022 at 10:37:56AM +0100, Borislav Petkov wrote:
> Jamie, I'd appreciate testing this one too, pls, just in case.

Here's a version against -rc8 - the previous one was against tip/master
and had other contextual changes in it.

---
From: Borislav Petkov <bp@suse.de>

The commit in Fixes started adding INT3 after RETs as a mitigation
against straight-line speculation.

The fastop SETcc implementation in kvm's insn emulator uses macro magic
to generate all possible SETcc functions and to jump to them when
emulating the respective instruction.

However, it hardcodes the size and alignment of those functions to 4: a
three-byte SETcc insn and a single-byte RET. BUT, with SLS, there's an
INT3 that gets slapped after the RET, which brings the whole scheme out
of alignment:

  15:   0f 90 c0                seto   %al
  18:   c3                      ret
  19:   cc                      int3
  1a:   0f 1f 00                nopl   (%rax)
  1d:   0f 91 c0                setno  %al
  20:   c3                      ret
  21:   cc                      int3
  22:   0f 1f 00                nopl   (%rax)
  25:   0f 92 c0                setb   %al
  28:   c3                      ret
  29:   cc                      int3

and this explodes like this:

  int3: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 2435 Comm: qemu-system-x86 Not tainted 5.17.0-rc8-sls #1
  Hardware name: Dell Inc. Precision WorkStation T3400  /0TP412, BIOS A14 04/30/2012
  RIP: 0010:setc+0x5/0x8 [kvm]
  Code: 00 00 0f 1f 00 0f b6 05 43 24 06 00 c3 cc 0f 1f 80 00 00 00 00 0f 90 c0 c3 cc 0f 1f 00 0f 91 c0 c3 cc 0f 1f 00 0f 92 c0 c3 cc <0f> 1f 00 0f 93 c0 c3 cc 0f 1f 00 0f 94 c0 c3 cc 0f 1f 00 0f 95 c0
  Call Trace:
   <TASK>
   ? x86_emulate_insn [kvm]
   ? x86_emulate_instruction [kvm]
   ? vmx_handle_exit [kvm_intel]
   ? kvm_arch_vcpu_ioctl_run [kvm]
   ? kvm_vcpu_ioctl [kvm]
   ? __x64_sys_ioctl
   ? do_syscall_64+0x40/0xa0
   ? entry_SYSCALL_64_after_hwframe+0x44/0xae
   </TASK>

Raise the alignment value when SLS is enabled and use a macro for that
instead of hard-coding naked numbers.

Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
Reported-by: Jamie Heilman <jamie@audible.transient.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/YjGzJwjrvxg5YZ0Z@audible.transient.net
---
 arch/x86/kvm/emulate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5719d8cfdbd9..f321abb9a4a8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -429,8 +429,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	FOP_END
 
 /* Special case for SETcc - 1 instruction per cc */
+
+#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
+
 #define FOP_SETCC(op) \
-	".align 4 \n\t" \
+	".align " __stringify(SETCC_ALIGN) " \n\t" \
 	".type " #op ", @function \n\t" \
 	#op ": \n\t" \
 	#op " %al \n\t" \
@@ -1047,7 +1050,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
 static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
 {
 	u8 rc;
-	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
+	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
 	asm("push %[flags]; popf; " CALL_NOSPEC
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Peter Zijlstra]

On Thu, Mar 17, 2022 at 11:52:33AM +0100, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> The commit in Fixes started adding INT3 after RETs as a mitigation
> against straight-line speculation.
> 
> The fastop SETcc implementation in kvm's insn emulator uses macro magic
> to generate all possible SETcc functions and to jump to them when
> emulating the respective instruction.
> 
> However, it hardcodes the size and alignment of those functions to 4: a
> three-byte SETcc insn and a single-byte RET. BUT, with SLS, there's an
> INT3 that gets slapped after the RET, which brings the whole scheme out
> of alignment:
> 
>   15:   0f 90 c0                seto   %al
>   18:   c3                      ret
>   19:   cc                      int3
>   1a:   0f 1f 00                nopl   (%rax)
>   1d:   0f 91 c0                setno  %al
>   20:   c3                      ret
>   21:   cc                      int3
>   22:   0f 1f 00                nopl   (%rax)
>   25:   0f 92 c0                setb   %al
>   28:   c3                      ret
>   29:   cc                      int3
> 
> and this explodes like this:
> 
>   int3: 0000 [#1] PREEMPT SMP PTI
>   CPU: 0 PID: 2435 Comm: qemu-system-x86 Not tainted 5.17.0-rc8-sls #1
>   Hardware name: Dell Inc. Precision WorkStation T3400  /0TP412, BIOS A14 04/30/2012
>   RIP: 0010:setc+0x5/0x8 [kvm]
>   Code: 00 00 0f 1f 00 0f b6 05 43 24 06 00 c3 cc 0f 1f 80 00 00 00 00 0f 90 c0 c3 cc 0f 1f 00 0f 91 c0 c3 cc 0f 1f 00 0f 92 c0 c3 cc <0f> 1f 00 0f 93 c0 c3 cc 0f 1f 00 0f 94 c0 c3 cc 0f 1f 00 0f 95 c0
>   Call Trace:
>    <TASK>
>    ? x86_emulate_insn [kvm]
>    ? x86_emulate_instruction [kvm]
>    ? vmx_handle_exit [kvm_intel]
>    ? kvm_arch_vcpu_ioctl_run [kvm]
>    ? kvm_vcpu_ioctl [kvm]
>    ? __x64_sys_ioctl
>    ? do_syscall_64+0x40/0xa0
>    ? entry_SYSCALL_64_after_hwframe+0x44/0xae
>    </TASK>
> 
> Raise the alignment value when SLS is enabled and use a macro for that
> instead of hard-coding naked numbers.
> 
> Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
> Reported-by: Jamie Heilman <jamie@audible.transient.net>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: https://lore.kernel.org/r/YjGzJwjrvxg5YZ0Z@audible.transient.net

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Depending on what Paolo wants, it might make sense to merge this into
tip/x86/urgent such that we can then resolve the merge conflict vs
tip/x86/core with something like the below:

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 113fd5c1b874..06dfbe9adcdb 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -24,6 +24,7 @@
 #include <linux/stringify.h>
 #include <asm/debugreg.h>
 #include <asm/nospec-branch.h>
+#include <asm/ibt.h>
 
 #include "x86.h"
 #include "tss.h"
@@ -431,7 +432,19 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 
 /* Special case for SETcc - 1 instruction per cc */
 
-#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
+/*
+ * Depending on .config the SETcc functions look like:
+ *
+ * setcc:
+ * +0	ENDBR		[CONFIG_X86_KERNEL_IBT]
+ * +4	SETcc	%al
+ * +7	RET
+ * +8	INT3		[CONFIG_SLS]
+ *
+ * Which gives possible sizes: 4, 5, 8, 9 which when rounded up to the
+ * next power-of-two alignment become: 4, 8, 16.
+ */
+#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)) * (1 + HAS_KERNEL_IBT))
 
 #define FOP_SETCC(op) \
 	".align " __stringify(SETCC_ALIGN) " \n\t" \

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Paolo Bonzini]

On 3/17/22 12:04, Peter Zijlstra wrote:
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Depending on what Paolo wants, it might make sense to merge this into
> tip/x86/urgent such that we can then resolve the merge conflict vs
> tip/x86/core with something like the below:

Sorry for responding late, I was sick the past few days.  Go ahead and 
apply it to tip/x86/core with the rest of the SLS and IBT patches.  If 
you place it in front of the actual insertion of the INT3 it will even 
be bisectable, but I'm not sure if your commit hashes are already frozen.

Just one thing:

> -#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
> +/*
> + * Depending on .config the SETcc functions look like:
> + *
> + * setcc:
> + * +0	ENDBR		[CONFIG_X86_KERNEL_IBT]
> + * +4	SETcc	%al
> + * +7	RET
> + * +8	INT3		[CONFIG_SLS]
> + *
> + * Which gives possible sizes: 4, 5, 8, 9 which when rounded up to the
> + * next power-of-two alignment become: 4, 8, 16.
> + */
> +#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)) * (1 + HAS_KERNEL_IBT))

This might be slightly nicer as (4 << IS_ENABLED(CONFIG_SLS) << 
HAS_KERNEL_IBT.  Or maybe not, depends on your taste.

It might also be worth doing:

#define SETCC_LENGTH (4 + IS_ENABLED(CONFIG_SLS) + 4 * HAS_KERNEL_IBT)
#define SETCC_ALIGN  (4 << IS_ENABLED(CONFIG_SLS) << HAS_KERNEL_IBT)
BUILD_BUG_ON(SETCC_LENGTH <= SETCC_ALIGN);

Thanks,

Paolo

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Borislav Petkov]

On Sat, Mar 19, 2022 at 02:24:06PM +0100, Paolo Bonzini wrote:
> Sorry for responding late, I was sick the past few days.  Go ahead and apply
> it to tip/x86/core with the rest of the SLS and IBT patches.  If you place
> it in front of the actual insertion of the INT3 it will even be bisectable,
> but I'm not sure if your commit hashes are already frozen.

I think they are and we need this fix in 5.17 where the SLS stuff went
in. I'll send it to Linus tomorrow.

> Just one thing:

Yeah, peterz can then do this ontop, before sending the IBT pile.

Thx for letting us know!

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Paolo Bonzini]

On 3/19/22 14:36, Borislav Petkov wrote:
> On Sat, Mar 19, 2022 at 02:24:06PM +0100, Paolo Bonzini wrote:
>> Sorry for responding late, I was sick the past few days.  Go ahead and apply
>> it to tip/x86/core with the rest of the SLS and IBT patches.  If you place
>> it in front of the actual insertion of the INT3 it will even be bisectable,
>> but I'm not sure if your commit hashes are already frozen.
> 
> I think they are and we need this fix in 5.17 where the SLS stuff went
> in. I'll send it to Linus tomorrow.

Nah, don't worry.  I'll take care of it, I'm still not 100% on top of 
things but I can handle one patch. :)

Paolo

>> Just one thing:
> 
> Yeah, peterz can then do this ontop, before sending the IBT pile.
> 
> Thx for letting us know!
> 

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Borislav Petkov]

On Sat, Mar 19, 2022 at 02:41:20PM +0100, Paolo Bonzini wrote:
> Nah, don't worry.  I'll take care of it, I'm still not 100% on top of things
> but I can handle one patch. :)

Well, if you take it, then you'll have to give us an immutable branch,
please, to merge it into x86/core so that peterz can do his IBT fix
ontop before he sends the stuff during the merge window.

In any case, here's the final version (did some commit message fixups +
added tags).

Thx.

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 16 Mar 2022 22:05:52 +0100
Subject: [PATCH] kvm/emulate: Fix SETcc emulation function offsets with SLS

The commit in Fixes started adding INT3 after RETs as a mitigation
against straight-line speculation.

The fastop SETcc implementation in kvm's insn emulator uses macro magic
to generate all possible SETcc functions and to jump to them when
emulating the respective instruction.

However, it hardcodes the size and alignment of those functions to 4: a
three-byte SETcc insn and a single-byte RET. BUT, with SLS, there's an
INT3 that gets slapped after the RET, which brings the whole scheme out
of alignment:

  15:   0f 90 c0                seto   %al
  18:   c3                      ret
  19:   cc                      int3
  1a:   0f 1f 00                nopl   (%rax)
  1d:   0f 91 c0                setno  %al
  20:   c3                      ret
  21:   cc                      int3
  22:   0f 1f 00                nopl   (%rax)
  25:   0f 92 c0                setb   %al
  28:   c3                      ret
  29:   cc                      int3

and this explodes like this:

  int3: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 2435 Comm: qemu-system-x86 Not tainted 5.17.0-rc8-sls #1
  Hardware name: Dell Inc. Precision WorkStation T3400  /0TP412, BIOS A14 04/30/2012
  RIP: 0010:setc+0x5/0x8 [kvm]
  Code: 00 00 0f 1f 00 0f b6 05 43 24 06 00 c3 cc 0f 1f 80 00 00 00 00 0f 90 c0 c3 cc 0f \
	  1f 00 0f 91 c0 c3 cc 0f 1f 00 0f 92 c0 c3 cc <0f> 1f 00 0f 93 c0 c3 cc 0f 1f 00 \
	  0f 94 c0 c3 cc 0f 1f 00 0f 95 c0
  Call Trace:
   <TASK>
   ? x86_emulate_insn [kvm]
   ? x86_emulate_instruction [kvm]
   ? vmx_handle_exit [kvm_intel]
   ? kvm_arch_vcpu_ioctl_run [kvm]
   ? kvm_vcpu_ioctl [kvm]
   ? __x64_sys_ioctl
   ? do_syscall_64
   ? entry_SYSCALL_64_after_hwframe
   </TASK>

Raise the alignment value when SLS is enabled and use a macro for that
instead of hard-coding naked numbers.

Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
Reported-by: Jamie Heilman <jamie@audible.transient.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Jamie Heilman <jamie@audible.transient.net>
Link: https://lore.kernel.org/r/YjGzJwjrvxg5YZ0Z@audible.transient.net
---
 arch/x86/kvm/emulate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5719d8cfdbd9..f321abb9a4a8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -429,8 +429,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	FOP_END
 
 /* Special case for SETcc - 1 instruction per cc */
+
+#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
+
 #define FOP_SETCC(op) \
-	".align 4 \n\t" \
+	".align " __stringify(SETCC_ALIGN) " \n\t" \
 	".type " #op ", @function \n\t" \
 	#op ": \n\t" \
 	#op " %al \n\t" \
@@ -1047,7 +1050,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
 static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
 {
 	u8 rc;
-	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
+	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
 	asm("push %[flags]; popf; " CALL_NOSPEC
-- 
2.29.2

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Paolo Bonzini]

On 3/19/22 14:50, Borislav Petkov wrote:
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 5719d8cfdbd9..f321abb9a4a8 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -429,8 +429,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>   	FOP_END
>   
>   /* Special case for SETcc - 1 instruction per cc */
> +
> +#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
> +
>   #define FOP_SETCC(op) \
> -	".align 4 \n\t" \
> +	".align " __stringify(SETCC_ALIGN) " \n\t" \
>   	".type " #op ", @function \n\t" \
>   	#op ": \n\t" \
>   	#op " %al \n\t" \
> @@ -1047,7 +1050,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
>   static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
>   {
>   	u8 rc;
> -	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
> +	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
>   
>   	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
>   	asm("push %[flags]; popf; " CALL_NOSPEC

So this is what I squashed in:

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f321abb9a4a8..e86d610dc6b7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -430,7 +430,19 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
  
  /* Special case for SETcc - 1 instruction per cc */
  
-#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
+/*
+ * Depending on .config the SETcc functions look like:
+ *
+ * SETcc %al   [3 bytes]
+ * RET         [1 byte]
+ * INT3        [1 byte; CONFIG_SLS]
+ *
+ * Which gives possible sizes 4 or 5.  When rounded up to the
+ * next power-of-two alignment they become 4 or 8.
+ */
+#define SETCC_LENGTH	(4 + IS_ENABLED(CONFIG_SLS))
+#define SETCC_ALIGN	(4 << IS_ENABLED(CONFIG_SLS))
+static_assert(SETCC_LENGTH <= SETCC_ALIGN);
  
  #define FOP_SETCC(op) \
  	".align " __stringify(SETCC_ALIGN) " \n\t" \

Paolo

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Boris Petkov]

On March 20, 2022 2:04:02 PM UTC, Paolo Bonzini <pbonzini@redhat.com> wrote:
>So this is what I squashed in:
>
>diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>index f321abb9a4a8..e86d610dc6b7 100644
>--- a/arch/x86/kvm/emulate.c
>+++ b/arch/x86/kvm/emulate.c
>@@ -430,7 +430,19 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>  
>  /* Special case for SETcc - 1 instruction per cc */
>  
>-#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
>+/*
>+ * Depending on .config the SETcc functions look like:
>+ *
>+ * SETcc %al   [3 bytes]
>+ * RET         [1 byte]
>+ * INT3        [1 byte; CONFIG_SLS]
>+ *
>+ * Which gives possible sizes 4 or 5.  When rounded up to the
>+ * next power-of-two alignment they become 4 or 8.
>+ */
>+#define SETCC_LENGTH	(4 + IS_ENABLED(CONFIG_SLS))
>+#define SETCC_ALIGN	(4 << IS_ENABLED(CONFIG_SLS))
>+static_assert(SETCC_LENGTH <= SETCC_ALIGN);
>  
>  #define FOP_SETCC(op) \
>  	".align " __stringify(SETCC_ALIGN) " \n\t" \
>
>Paolo


Ack.

Thanks.

-- 
Sent from a small device: formatting sux and brevity is inevitable.

Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS

[Jamie Heilman]

Borislav Petkov wrote:
> On Thu, Mar 17, 2022 at 10:37:56AM +0100, Borislav Petkov wrote:
> > Jamie, I'd appreciate testing this one too, pls, just in case.
> 
> Here's a version against -rc8 - the previous one was against tip/master
> and had other contextual changes in it.

You can add my Tested-by: Jamie Heilman <jamie@audible.transient.net>

This still works great with CONFIG_SLS=Y or CONFIG_SLS=N.

> ---
> From: Borislav Petkov <bp@suse.de>
> 
> The commit in Fixes started adding INT3 after RETs as a mitigation
> against straight-line speculation.
> 
> The fastop SETcc implementation in kvm's insn emulator uses macro magic
> to generate all possible SETcc functions and to jump to them when
> emulating the respective instruction.
> 
> However, it hardcodes the size and alignment of those functions to 4: a
> three-byte SETcc insn and a single-byte RET. BUT, with SLS, there's an
> INT3 that gets slapped after the RET, which brings the whole scheme out
> of alignment:
> 
>   15:   0f 90 c0                seto   %al
>   18:   c3                      ret
>   19:   cc                      int3
>   1a:   0f 1f 00                nopl   (%rax)
>   1d:   0f 91 c0                setno  %al
>   20:   c3                      ret
>   21:   cc                      int3
>   22:   0f 1f 00                nopl   (%rax)
>   25:   0f 92 c0                setb   %al
>   28:   c3                      ret
>   29:   cc                      int3
> 
> and this explodes like this:
> 
>   int3: 0000 [#1] PREEMPT SMP PTI
>   CPU: 0 PID: 2435 Comm: qemu-system-x86 Not tainted 5.17.0-rc8-sls #1
>   Hardware name: Dell Inc. Precision WorkStation T3400  /0TP412, BIOS A14 04/30/2012
>   RIP: 0010:setc+0x5/0x8 [kvm]
>   Code: 00 00 0f 1f 00 0f b6 05 43 24 06 00 c3 cc 0f 1f 80 00 00 00 00 0f 90 c0 c3 cc 0f 1f 00 0f 91 c0 c3 cc 0f 1f 00 0f 92 c0 c3 cc <0f> 1f 00 0f 93 c0 c3 cc 0f 1f 00 0f 94 c0 c3 cc 0f 1f 00 0f 95 c0
>   Call Trace:
>    <TASK>
>    ? x86_emulate_insn [kvm]
>    ? x86_emulate_instruction [kvm]
>    ? vmx_handle_exit [kvm_intel]
>    ? kvm_arch_vcpu_ioctl_run [kvm]
>    ? kvm_vcpu_ioctl [kvm]
>    ? __x64_sys_ioctl
>    ? do_syscall_64+0x40/0xa0
>    ? entry_SYSCALL_64_after_hwframe+0x44/0xae
>    </TASK>
> 
> Raise the alignment value when SLS is enabled and use a macro for that
> instead of hard-coding naked numbers.
> 
> Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
> Reported-by: Jamie Heilman <jamie@audible.transient.net>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: https://lore.kernel.org/r/YjGzJwjrvxg5YZ0Z@audible.transient.net
> ---
>  arch/x86/kvm/emulate.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 5719d8cfdbd9..f321abb9a4a8 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -429,8 +429,11 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>  	FOP_END
>  
>  /* Special case for SETcc - 1 instruction per cc */
> +
> +#define SETCC_ALIGN	(4 * (1 + IS_ENABLED(CONFIG_SLS)))
> +
>  #define FOP_SETCC(op) \
> -	".align 4 \n\t" \
> +	".align " __stringify(SETCC_ALIGN) " \n\t" \
>  	".type " #op ", @function \n\t" \
>  	#op ": \n\t" \
>  	#op " %al \n\t" \
> @@ -1047,7 +1050,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
>  static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
>  {
>  	u8 rc;
> -	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
> +	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
>  
>  	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
>  	asm("push %[flags]; popf; " CALL_NOSPEC
> -- 
> 2.29.2
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

-- 
Jamie Heilman                     http://audible.transient.net/~jamie/