[kvm-unit-tests PATCH v2 0/5] Test for illegal LEA & related fixes

[kvm-unit-tests PATCH v2 0/5] Test for illegal LEA & related fixes

[Michal Luczaj]

Is this a correct way to put Sean's and Paolo's patches into this
series? I wasn't sure which patches are accepted or what is the current
base-commit, but I haven't seen the tip of kvm-unit-tests repo moving,
so here is it.

Michal Luczaj (3):
  x86: emulator.c cleanup: Save and restore exception handlers
  x86: emulator.c cleanup: Use ASM_TRY() for the UD_VECTOR cases
  x86: Test emulator's handling of LEA with /reg

Paolo Bonzini (1):
  x86: Introduce ASM_TRY_FEP() to handle exceptions thrown by
    FEP-triggered emulator

Sean Christopherson (1):
  x86: Dedup 32-bit vs. 64-bit ASM_TRY() by stealing kernel's
    __ASM_SEL()

 lib/x86/desc.h      |  22 +++----
 lib/x86/processor.h |  12 ++++
 x86/emulator.c      | 136 ++++++++++++++++++++++++--------------------
 3 files changed, 93 insertions(+), 77 deletions(-)

-- 
2.37.1

[kvm-unit-tests PATCH v2 1/5] x86: emulator.c cleanup: Save and restore exception handlers

[Michal Luczaj]

Users of handle_exception() should always save and restore the handlers.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
v1 -> v2: No changes

 x86/emulator.c | 78 ++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index cd78e3c..769a049 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -710,6 +710,7 @@ static __attribute__((target("sse2"))) void test_sse_exceptions(void *cross_mem)
 	void *page2 = (void *)(&bytes[4096]);
 	struct pte_search search;
 	pteval_t orig_pte;
+	handler old;
 
 	// setup memory for unaligned access
 	mem = (uint32_t *)(&bytes[8]);
@@ -725,10 +726,10 @@ static __attribute__((target("sse2"))) void test_sse_exceptions(void *cross_mem)
 	asm("movupd %1, %0" : "=m"(*mem) : "x"(vv) : "memory");
 	report(sseeq(v, mem), "movupd unaligned");
 	exceptions = 0;
-	handle_exception(GP_VECTOR, unaligned_movaps_handler);
+	old = handle_exception(GP_VECTOR, unaligned_movaps_handler);
 	asm("movaps %1, %0\n\t unaligned_movaps_cont:"
 			: "=m"(*mem) : "x"(vv));
-	handle_exception(GP_VECTOR, 0);
+	handle_exception(GP_VECTOR, old);
 	report(exceptions == 1, "unaligned movaps exception");
 
 	// setup memory for cross page access
@@ -746,10 +747,10 @@ static __attribute__((target("sse2"))) void test_sse_exceptions(void *cross_mem)
 	invlpg(page2);
 
 	exceptions = 0;
-	handle_exception(PF_VECTOR, cross_movups_handler);
+	old = handle_exception(PF_VECTOR, cross_movups_handler);
 	asm("movups %1, %0\n\t cross_movups_cont:" : "=m"(*mem) : "x"(vv) :
 			"memory");
-	handle_exception(PF_VECTOR, 0);
+	handle_exception(PF_VECTOR, old);
 	report(exceptions == 1, "movups crosspage exception");
 
 	// restore invalidated page
@@ -817,36 +818,38 @@ static void advance_rip_and_note_exception(struct ex_regs *regs)
 
 static void test_mmx_movq_mf(uint64_t *mem)
 {
-    /* movq %mm0, (%rax) */
-    extern char movq_start, movq_end;
-
-    uint16_t fcw = 0;  /* all exceptions unmasked */
-    write_cr0(read_cr0() & ~6);  /* TS, EM */
-    exceptions = 0;
-    handle_exception(MF_VECTOR, advance_rip_and_note_exception);
-    asm volatile("fninit; fldcw %0" : : "m"(fcw));
-    asm volatile("fldz; fldz; fdivp"); /* generate exception */
-
-    rip_advance = &movq_end - &movq_start;
-    asm(KVM_FEP "movq_start: movq %mm0, (%rax); movq_end:");
-    /* exit MMX mode */
-    asm volatile("fnclex; emms");
-    report(exceptions == 1, "movq mmx generates #MF");
-    handle_exception(MF_VECTOR, 0);
+	/* movq %mm0, (%rax) */
+	extern char movq_start, movq_end;
+	handler old;
+
+	uint16_t fcw = 0;  /* all exceptions unmasked */
+	write_cr0(read_cr0() & ~6);  /* TS, EM */
+	exceptions = 0;
+	old = handle_exception(MF_VECTOR, advance_rip_and_note_exception);
+	asm volatile("fninit; fldcw %0" : : "m"(fcw));
+	asm volatile("fldz; fldz; fdivp"); /* generate exception */
+
+	rip_advance = &movq_end - &movq_start;
+	asm(KVM_FEP "movq_start: movq %mm0, (%rax); movq_end:");
+	/* exit MMX mode */
+	asm volatile("fnclex; emms");
+	report(exceptions == 1, "movq mmx generates #MF");
+	handle_exception(MF_VECTOR, old);
 }
 
 static void test_jmp_noncanonical(uint64_t *mem)
 {
 	extern char nc_jmp_start, nc_jmp_end;
+	handler old;
 
 	*mem = 0x1111111111111111ul;
 
 	exceptions = 0;
 	rip_advance = &nc_jmp_end - &nc_jmp_start;
-	handle_exception(GP_VECTOR, advance_rip_and_note_exception);
+	old = handle_exception(GP_VECTOR, advance_rip_and_note_exception);
 	asm volatile ("nc_jmp_start: jmp *%0; nc_jmp_end:" : : "m"(*mem));
 	report(exceptions == 1, "jump to non-canonical address");
-	handle_exception(GP_VECTOR, 0);
+	handle_exception(GP_VECTOR, old);
 }
 
 static void test_movabs(uint64_t *mem)
@@ -979,22 +982,23 @@ static void ss_bad_rpl(struct ex_regs *regs)
 
 static void test_sreg(volatile uint16_t *mem)
 {
-    u16 ss = read_ss();
+	u16 ss = read_ss();
+	handler old;
 
-    // check for null segment load
-    *mem = 0;
-    asm volatile("mov %0, %%ss" : : "m"(*mem));
-    report(read_ss() == 0, "mov null, %%ss");
-
-    // check for exception when ss.rpl != cpl on null segment load
-    exceptions = 0;
-    handle_exception(GP_VECTOR, ss_bad_rpl);
-    *mem = 3;
-    asm volatile("mov %0, %%ss; ss_bad_rpl_cont:" : : "m"(*mem));
-    report(exceptions == 1 && read_ss() == 0,
-           "mov null, %%ss (with ss.rpl != cpl)");
-    handle_exception(GP_VECTOR, 0);
-    write_ss(ss);
+	// check for null segment load
+	*mem = 0;
+	asm volatile("mov %0, %%ss" : : "m"(*mem));
+	report(read_ss() == 0, "mov null, %%ss");
+
+	// check for exception when ss.rpl != cpl on null segment load
+	exceptions = 0;
+	old = handle_exception(GP_VECTOR, ss_bad_rpl);
+	*mem = 3;
+	asm volatile("mov %0, %%ss; ss_bad_rpl_cont:" : : "m"(*mem));
+	report(exceptions == 1 && read_ss() == 0,
+	       "mov null, %%ss (with ss.rpl != cpl)");
+	handle_exception(GP_VECTOR, old);
+	write_ss(ss);
 }
 
 static uint64_t usr_gs_mov(void)
-- 
2.37.1

Re: [kvm-unit-tests PATCH v2 1/5] x86: emulator.c cleanup: Save and restore exception handlers

[Sean Christopherson]

On Sun, Aug 07, 2022, Michal Luczaj wrote:
> Users of handle_exception() should always save and restore the handlers.

Might be worth calling out that #UD is intentionally left alone and will be fixed
separately.  No need for to spin a new version though, Paolo can add the note (or
not) if he wants.

> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[kvm-unit-tests PATCH v2 2/5] x86: emulator.c cleanup: Use ASM_TRY() for the UD_VECTOR cases

[Michal Luczaj]

For #UD handling use ASM_TRY() instead of handle_exception().

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
v1 -> v2: Change `mov` to `movl` to silence the compiler warning

 x86/emulator.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 769a049..96d3ccd 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -17,10 +17,8 @@
 
 static int exceptions;
 
-/* Forced emulation prefix, used to invoke the emulator unconditionally.  */
+/* Forced emulation prefix, used to invoke the emulator unconditionally. */
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
-#define KVM_FEP_LENGTH 5
-static int fep_available = 1;
 
 struct regs {
 	u64 rax, rbx, rcx, rdx;
@@ -1099,33 +1097,23 @@ static void test_simplealu(u32 *mem)
     report(*mem == 0x8400, "test");
 }
 
-static void illegal_movbe_handler(struct ex_regs *regs)
-{
-	extern char bad_movbe_cont;
-
-	++exceptions;
-	regs->rip = (ulong)&bad_movbe_cont;
-}
-
 static void test_illegal_movbe(void)
 {
+	unsigned int vector;
+
 	if (!this_cpu_has(X86_FEATURE_MOVBE)) {
-		report_skip("illegal movbe");
+		report_skip("MOVBE unsupported by CPU");
 		return;
 	}
 
-	exceptions = 0;
-	handle_exception(UD_VECTOR, illegal_movbe_handler);
-	asm volatile(".byte 0x0f; .byte 0x38; .byte 0xf0; .byte 0xc0;\n\t"
-		     " bad_movbe_cont:" : : : "rax");
-	report(exceptions == 1, "illegal movbe");
-	handle_exception(UD_VECTOR, 0);
-}
+	asm volatile(ASM_TRY("1f")
+		     ".byte 0x0f; .byte 0x38; .byte 0xf0; .byte 0xc0;\n\t"
+		     "1:"
+		     : : : "memory", "rax");
 
-static void record_no_fep(struct ex_regs *regs)
-{
-	fep_available = 0;
-	regs->rip += KVM_FEP_LENGTH;
+	vector = exception_vector();
+	report(vector == UD_VECTOR,
+	       "Wanted #UD on MOVBE with /reg, got vector = %u", vector);
 }
 
 int main(void)
@@ -1135,11 +1123,13 @@ int main(void)
 	void *insn_ram;
 	void *cross_mem;
 	unsigned long t1, t2;
+	int fep_available = 0;
 
 	setup_vm();
-	handle_exception(UD_VECTOR, record_no_fep);
-	asm(KVM_FEP "nop");
-	handle_exception(UD_VECTOR, 0);
+	asm volatile(ASM_TRY("1f")
+		     KVM_FEP "movl $1, %[fep_available]\n\t"
+		     "1:"
+		     : [fep_available] "=m" (fep_available) : : "memory");
 
 	mem = alloc_vpages(2);
 	install_page((void *)read_cr3(), IORAM_BASE_PHYS, mem);
-- 
2.37.1

[kvm-unit-tests PATCH v2 3/5] x86: Introduce ASM_TRY_FEP() to handle exceptions thrown by FEP-triggered emulator

[Michal Luczaj]

From: Paolo Bonzini <pbonzini@redhat.com>

TRY_ASM() mishandles exceptions thrown by the forced-emulation-triggered
emulator. While the faulting address stored in the exception table points
at forced emulation prefix, when an exceptions comes, RIP is 5 bytes
(size of KVM_FEP) ahead and the exception ends up unhandled.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
For the sake of completeness, I've added `prefix` to 32-bit __TRY_ASM()
as well.

 lib/x86/desc.h | 8 ++++++--
 x86/emulator.c | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 2a285eb..8854fcc 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -81,21 +81,25 @@ typedef struct  __attribute__((packed)) {
 } tss64_t;
 
 #ifdef __x86_64
-#define ASM_TRY(catch)			\
+#define __ASM_TRY(prefix, catch)	\
 	"movl $0, %%gs:4 \n\t"		\
 	".pushsection .data.ex \n\t"	\
 	".quad 1111f, " catch "\n\t"	\
 	".popsection \n\t"		\
+	prefix				\
 	"1111:"
 #else
-#define ASM_TRY(catch)			\
+#define __ASM_TRY(prefix, catch)	\
 	"movl $0, %%gs:4 \n\t"		\
 	".pushsection .data.ex \n\t"	\
 	".long 1111f, " catch "\n\t"	\
 	".popsection \n\t"		\
+	prefix				\
 	"1111:"
 #endif
 
+#define ASM_TRY(catch) __ASM_TRY("", catch)
+
 /*
  * selector     32-bit                        64-bit
  * 0x00         NULL descriptor               NULL descriptor
diff --git a/x86/emulator.c b/x86/emulator.c
index 96d3ccd..0eb7c1a 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -19,6 +19,7 @@ static int exceptions;
 
 /* Forced emulation prefix, used to invoke the emulator unconditionally. */
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
+#define ASM_TRY_FEP(catch) __ASM_TRY(KVM_FEP, catch)
 
 struct regs {
 	u64 rax, rbx, rcx, rdx;
-- 
2.37.1

[kvm-unit-tests PATCH v2 4/5] x86: Dedup 32-bit vs. 64-bit ASM_TRY() by stealing kernel's __ASM_SEL()

[Michal Luczaj]

From: Sean Christopherson <seanjc@google.com>

Steal the kernel's __ASM_SEL() implementation and use it to consolidate
ASM_TRY().  The only difference between the 32-bit and 64-bit versions is
the size of the address stored in the table.

No functional change intended.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 lib/x86/desc.h      | 22 ++++++----------------
 lib/x86/processor.h | 12 ++++++++++++
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 8854fcc..5bb6fcb 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -80,23 +80,13 @@ typedef struct  __attribute__((packed)) {
 	u16 iomap_base;
 } tss64_t;
 
-#ifdef __x86_64
-#define __ASM_TRY(prefix, catch)	\
-	"movl $0, %%gs:4 \n\t"		\
-	".pushsection .data.ex \n\t"	\
-	".quad 1111f, " catch "\n\t"	\
-	".popsection \n\t"		\
-	prefix				\
+#define __ASM_TRY(prefix, catch)				\
+	"movl $0, %%gs:4 \n\t"					\
+	".pushsection .data.ex \n\t"				\
+	__ASM_SEL(.long, .quad) " 1111f,  " catch "\n\t"	\
+	".popsection \n\t"					\
+	prefix							\
 	"1111:"
-#else
-#define __ASM_TRY(prefix, catch)	\
-	"movl $0, %%gs:4 \n\t"		\
-	".pushsection .data.ex \n\t"	\
-	".long 1111f, " catch "\n\t"	\
-	".popsection \n\t"		\
-	prefix				\
-	"1111:"
-#endif
 
 #define ASM_TRY(catch) __ASM_TRY("", catch)
 
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 0324220..30e2de8 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -19,6 +19,18 @@
 #  define S "4"
 #endif
 
+#ifdef __ASSEMBLY__
+#define __ASM_FORM(x, ...)	x,## __VA_ARGS__
+#else
+#define __ASM_FORM(x, ...)	" " xstr(x,##__VA_ARGS__) " "
+#endif
+
+#ifndef __x86_64__
+#define __ASM_SEL(a,b)		__ASM_FORM(a)
+#else
+#define __ASM_SEL(a,b)		__ASM_FORM(b)
+#endif
+
 #define DB_VECTOR 1
 #define BP_VECTOR 3
 #define UD_VECTOR 6
-- 
2.37.1

Re: [kvm-unit-tests PATCH v2 4/5] x86: Dedup 32-bit vs. 64-bit ASM_TRY() by stealing kernel's __ASM_SEL()

[Sean Christopherson]

On Sun, Aug 07, 2022, Michal Luczaj wrote:
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 0324220..30e2de8 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -19,6 +19,18 @@
>  #  define S "4"
>  #endif
>  
> +#ifdef __ASSEMBLY__
> +#define __ASM_FORM(x, ...)	x,## __VA_ARGS__
> +#else
> +#define __ASM_FORM(x, ...)	" " xstr(x,##__VA_ARGS__) " "
> +#endif
> +
> +#ifndef __x86_64__
> +#define __ASM_SEL(a,b)		__ASM_FORM(a)
> +#else
> +#define __ASM_SEL(a,b)		__ASM_FORM(b)
> +#endif

Argh, this can't go in processor.h, because processor.h includes desc.h (to use
ASM_TRY).  This patch "works" because emulator.c includes both process.or and
desc.h, but things go sideways if ASM_TRY_FEP() is moved into desc.h.

I'll post a new version of the entire series, the KVM_FEP macro and a helper to
check for FEP availability should really go in a common location, e.g. the PMU
test can use the common helper instead of requiring a separate unittest.cfg entry.

[kvm-unit-tests PATCH v2 5/5] x86: Test emulator's handling of LEA with /reg

[Michal Luczaj]

LEA with a register-direct source operand is illegal. Verify that the
emulator raises #UD.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
v1 -> v2: Use ASM_TRY_FEP()

 x86/emulator.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index 0eb7c1a..dc91ce3 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -897,6 +897,20 @@ static void test_mov_dr(uint64_t *mem)
 		report(rax == DR6_ACTIVE_LOW, "mov_dr6");
 }
 
+static void test_illegal_lea(void)
+{
+	unsigned int vector;
+
+	asm volatile (ASM_TRY_FEP("1f")
+		      ".byte 0x8d; .byte 0xc0\n\t"
+		      "1:"
+		      : : : "memory", "eax");
+
+	vector = exception_vector();
+	report(vector == UD_VECTOR,
+	       "Wanted #UD on LEA with /reg, got vector = %u", vector);
+}
+
 static void test_push16(uint64_t *mem)
 {
 	uint64_t rsp1, rsp2;
@@ -1188,6 +1202,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
+		test_illegal_lea();
 	} else {
 		report_skip("skipping register-only tests, "
 			    "use kvm.force_emulation_prefix=1 to enable");
-- 
2.37.1