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

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

[Dave Hansen]

On 3/16/22 11:45, Jamie Heilman wrote:

> 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
> RSP: 0018:ffffc90000a1fc68 EFLAGS: 00000283
> RAX: 0000000000000281 RBX: 0000000000000006 RCX: 0000000000000005
> RDX: ffffffffa01a4024 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: ffff88810ef76900 R08: 0000000000000000 R09: 0000000000000000
> R10: ffff88810ee54000 R11: 0000000000000000 R12: ffffffffa01d5720
> R13: 0000000000000000 R14: 0000000000000000 R15: ffff88810ef76900
> FS:  00007f23ecd79640(0000) GS:ffff888233c00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000108df8000 CR4: 00000000000426f0
> Call Trace:
>  <TASK>
>  ? x86_emulate_insn+0x76b/0xe00 [kvm]

Ooh, fun!

This hit one of the new int3's in "ASM_RET" in "setc" in
arch/x86/kvm/emulate.c:

	FOP_SETCC(setc)

Did the extra 'int3' screw up some presumed jump offset or something?

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

[Borislav Petkov]

On Wed, Mar 16, 2022 at 12:02:59PM -0700, Dave Hansen wrote:
> This hit one of the new int3's in "ASM_RET" in "setc" in
> arch/x86/kvm/emulate.c:
> 
> 	FOP_SETCC(setc)
> 
> Did the extra 'int3' screw up some presumed jump offset or something?

Yap, looks like it. I wonder how no one managed to hit this yet...

Jamie, does this fix it, per chance?

---
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


-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Wed, Mar 16, 2022 at 06:45:25PM +0000, Jamie Heilman wrote:
> Yep that worked, here's output, you can see the network get set up and
> then boom:

Thx, that was very useful. Does this below fix it, per chance:

---
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

-- 
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:
> On Wed, Mar 16, 2022 at 06:45:25PM +0000, Jamie Heilman wrote:
> > Yep that worked, here's output, you can see the network get set up and
> > then boom:
> 
> Thx, that was very useful. Does this below fix it, per chance:

It does indeed!

> ---
> 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
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

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

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.