system locks up with CONFIG_SLS=Y; 5.17.0-rc

system locks up with CONFIG_SLS=Y; 5.17.0-rc

[Jamie Heilman]

I've been (somewhat unsuccessfully) trying to bisect a hard lock-up
of my workstation that occurs when I'm running 5.17 rc kernels a few
seconds after I start a kvm guest instance.  There is no output to
any log, everything locks up completely, sysrq doesn't even work
anymore.  As bisection progressed closer and closer to the branch
where straight-line-speculation mitigation was enabled, and as bisect
landing me between 9cdbeec40968 ("x86/entry_32: Fix segment exceptions")
and 3411506550b1 ("x86/csum: Rewrite/optimize csum_partial()") wasn't
resulting in clear results (my system definately starts Oopsing and
gets so hosed up that I'm forced to reboot, but it isn't quite as dire
as sysrq continues to function) I decided to just try a build with
CONFIG_SLS disabled, and it turns out that works just fine.  Sooo...

This system uses a Intel Core2 Duo E8400 processor.
working config (CONFIG_SLS=N) and dmesg at:
http://audible.transient.net/~jamie/k/sls.config-5.17.0-rc8
http://audible.transient.net/~jamie/k/sls.dmesg

(I don't think the dmesg of CONFIG_SLS=Y is really any different.)

As far as I know the guest kernel I hand to qemu doesn't really
matter, but the gist of my qemu command line is:

qemu-system-x86_64 -m 2048 -name "$NAME" -machine pc,accel=kvm \
    -nographic -no-user-config -nodefaults -boot strict=on \
    -rtc base=utc -smp 1,sockets=1,cores=1,threads=1 \
    -chardev pipe,id=char0,path="$DIR/monitor" \
    -chardev pty,id=char1 \
    -device isa-serial,chardev=char1 \
    -device virtio-blk-pci,drive=blk0,bootindex=1 \
    -device virtio-net-pci,netdev=net0,"mac=$IF_MAC" \
    -device virtio-rng-pci,rng=rng0,max-bytes=1024,period=3000 \
    -drive "id=blk0,file=/dev/S/$NAME,if=none,format=raw,cache=none" \
    -mon chardev=char0,id=monitor,mode=control \
    -netdev "tap,id=net0,ifname=$NAME,script=no,downscript=no" \
    -object rng-random,id=rng0,filename=/dev/random


No clue what additional debugging would help to enable here, if
anything.  As you can see from the dmesg, I'm using gcc 11.2.0 from
Debian unstable, 4:11.2.0-2 to be exact.  Let me know what other
information would be useful.

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

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

[Borislav Petkov]

On Wed, Mar 16, 2022 at 09:51:35AM +0000, Jamie Heilman wrote:
> I've been (somewhat unsuccessfully) trying to bisect a hard lock-up
> of my workstation that occurs when I'm running 5.17 rc kernels a few
> seconds after I start a kvm guest instance.  There is no output to
> any log, everything locks up completely, sysrq doesn't even work
> anymore.

Any chance you can connect that box with a serial cable, get serial
console working and see if you can catch dmesg with it this way?

https://www.kernel.org/doc/html/latest/admin-guide/serial-console.html

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Dave Hansen]

On 3/16/22 02:51, Jamie Heilman wrote:
> I've been (somewhat unsuccessfully) trying to bisect a hard lock-up
> of my workstation that occurs when I'm running 5.17 rc kernels a few
> seconds after I start a kvm guest instance.  There is no output to
> any log, everything locks up completely, sysrq doesn't even work
> anymore.  As bisection progressed closer and closer to the branch
> where straight-line-speculation mitigation was enabled, and as bisect
> landing me between 9cdbeec40968 ("x86/entry_32: Fix segment exceptions")
> and 3411506550b1 ("x86/csum: Rewrite/optimize csum_partial()") wasn't
> resulting in clear results (my system definately starts Oopsing and
> gets so hosed up that I'm forced to reboot, but it isn't quite as dire
> as sysrq continues to function) I decided to just try a build with
> CONFIG_SLS disabled, and it turns out that works just fine.  Sooo...
> 
> This system uses a Intel Core2 Duo E8400 processor.
> working config (CONFIG_SLS=N) and dmesg at:
> http://audible.transient.net/~jamie/k/sls.config-5.17.0-rc8
> http://audible.transient.net/~jamie/k/sls.dmesg
> 
> (I don't think the dmesg of CONFIG_SLS=Y is really any different.)

If you get really ambitious, you could try to see if any of the
individual things that change based on the CONFIG_SLS #ifdef trigger
this.  Basically, turn off the config option and then go manually
enabling each of the sites.

The odd thing is that it isn't touching anything really KVM-specific.
It probably influences some KVM-specific assembly, but it's hard to see
how that might break anything.

The worrying part is:

ifdef CONFIG_SLS
  KBUILD_CFLAGS += -mharden-sls=all
endif

That's presumably a shiny, new compiler option, also known as a
relatively lightly tested compiler option.

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

[Jamie Heilman]

Borislav Petkov wrote:
> On Wed, Mar 16, 2022 at 09:51:35AM +0000, Jamie Heilman wrote:
> > I've been (somewhat unsuccessfully) trying to bisect a hard lock-up
> > of my workstation that occurs when I'm running 5.17 rc kernels a few
> > seconds after I start a kvm guest instance.  There is no output to
> > any log, everything locks up completely, sysrq doesn't even work
> > anymore.
> 
> Any chance you can connect that box with a serial cable, get serial
> console working and see if you can catch dmesg with it this way?
> 
> https://www.kernel.org/doc/html/latest/admin-guide/serial-console.html

Yep that worked, here's output, you can see the network get set up and
then boom:

br0: port 2(motorhead) entered blocking state
br0: port 2(motorhead) entered disabled state
device motorhead entered promiscuous mode
br0: port 2(motorhead) entered blocking state
br0: port 2(motorhead) entered forwarding state
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]
 ? x86_emulate_instruction+0x345/0x600 [kvm]
 ? vmx_handle_exit+0x2f5/0x760 [kvm_intel]
 ? kvm_arch_vcpu_ioctl_run+0x60b/0x1b40 [kvm]
 ? kvm_vcpu_ioctl+0x2ce/0x690 [kvm]
 ? __x64_sys_ioctl+0x483/0xa50
 ? do_syscall_64+0x40/0xa0
 ? entry_SYSCALL_64_after_hwframe+0x44/0xae
 </TASK>
Modules linked in: nfsv4 cpufreq_userspace cpufreq_powersave cpufreq_ondemand cpufreq_conservative autofs4 fan nfsd auth_rpcgss nfs lockd grace fscache netfs sunrpc bridge stp llc nhpoly1305_sse2 nhpoly1305 aes_generic libaes chacha_generic chacha_x86_64 libchacha adiantum libpoly1305 vhost_net tun vhost vhost_iotlb tap dm_crypt snd_hda_codec_analog snd_hda_codec_generic snd_usb_audio snd_usbmidi_lib snd_rawmidi usb_storage snd_hda_intel snd_seq_device snd_intel_dspcfg snd_hda_codec snd_hwdep snd_hda_core dcdbas tg3 snd_pcm floppy kvm_intel snd_timer snd evdev soundcore kvm sr_mod cdrom irqbypass sg xfs dm_mod raid1 md_mod psmouse
---[ end trace 0000000000000000 ]---
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
Kernel panic - not syncing: Fatal exception in interrupt
Kernel Offset: disabled
---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---


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

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

[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

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

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.