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

From: Peter Zijlstra <peterz@infradead.org>
To: Borislav Petkov <bp@alien8.de>
Cc: Jamie Heilman <jamie@audible.transient.net>,
	linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH -v1.2] kvm/emulate: Fix SETcc emulation function offsets with SLS
Date: Thu, 17 Mar 2022 12:04:05 +0100	[thread overview]
Message-ID: <YjMVpfe/9ldmWX8W@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <YjMS8eTOhXBOPFOe@zn.tnic>

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