[PATCH 1/2] KVM: x86: emulator: Fix illegal LEA handling

[PATCH 1/2] KVM: x86: emulator: Fix illegal LEA handling

[Michal Luczaj]

The emulator mishandles LEA with register source operand. Even though such
LEA is illegal, it can be encoded and fed to CPU. In which case real
hardware throws #UD. The emulator, instead, returns address of
x86_emulate_ctxt._regs. This info leak hurts host's kASLR.

Tell the decoder that illegal LEA is not to be emulated.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
What the emulator does for LEA is simply:
	
	case 0x8d: /* lea r16/r32, m */
		ctxt->dst.val = ctxt->src.addr.mem.ea;
		break;

And it makes sense if you assume that LEA's source operand is always
memory. But because there is a race window between VM-exit and the decoder
instruction fetch, emulator can be force fed an arbitrary opcode of choice.
Including some that are simply illegal and would cause #UD in normal
circumstances. Such as a LEA with a register-direct source operand -- for
which the emulator sets `op->addr.reg`, but reads `op->addr.mem.ea`.

		union {
			unsigned long *reg;
			struct segmented_address {
				ulong ea;
				unsigned seg;
			} mem;
			...
		} addr;

Because `reg` and `mem` are in union, emulator reveals address in host's
memory.

I hope this patch is not considered an `instr_dual` abuse?

 arch/x86/kvm/emulate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f8382abe22ff..7c14706372d0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4566,6 +4566,10 @@ static const struct mode_dual mode_dual_63 = {
 	N, I(DstReg | SrcMem32 | ModRM | Mov, em_movsxd)
 };
 
+static const struct instr_dual instr_dual_8d = {
+	D(DstReg | SrcMem | ModRM | NoAccess), N
+};
+
 static const struct opcode opcode_table[256] = {
 	/* 0x00 - 0x07 */
 	F6ALU(Lock, em_add),
@@ -4622,7 +4626,7 @@ static const struct opcode opcode_table[256] = {
 	I2bv(DstMem | SrcReg | ModRM | Mov | PageTable, em_mov),
 	I2bv(DstReg | SrcMem | ModRM | Mov, em_mov),
 	I(DstMem | SrcNone | ModRM | Mov | PageTable, em_mov_rm_sreg),
-	D(ModRM | SrcMem | NoAccess | DstReg),
+	ID(0, &instr_dual_8d),
 	I(ImplicitOps | SrcMem16 | ModRM, em_mov_sreg_rm),
 	G(0, group1A),
 	/* 0x90 - 0x97 */
-- 
2.32.0

[PATCH 2/2] KVM: selftests: x86: Test illegal LEA handling

[Michal Luczaj]

Check if the emulator reveals host's kernel memory address.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Although checkpatch complains about the use of `extern`, I am sending this
as it is. Let me know if you need it in some better form.

[unpatched]$ ./tools/testing/selftests/kvm/x86_64/emulator_illegal_lea
==== Test Assertion Failure ====
  x86_64/emulator_illegal_lea.c:129: uc->args[0] == 0
  pid=957 tid=957 errno=4 - Interrupted system call
     1	: check_state at emulator_illegal_lea.c:129
     2	: main at emulator_illegal_lea.c:169 (discriminator 1)
     3	: ?? ??:0
     4	: ?? ??:0
     5	: _start at ??:?
  Host's address leaked: 0xffff88811a5d3620 (2)

[patched]$ ./tools/testing/selftests/kvm/x86_64/emulator_illegal_lea
[patched]$

 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../kvm/x86_64/emulator_illegal_lea.c         | 180 ++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/emulator_illegal_lea.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 4509a3a7eeae..8977458080b9 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -17,6 +17,7 @@
 /x86_64/debug_regs
 /x86_64/evmcs_test
 /x86_64/emulator_error_test
+/x86_64/emulator_illegal_lea
 /x86_64/fix_hypercall_test
 /x86_64/get_msr_index_features
 /x86_64/kvm_clock_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 22423c871ed6..453cd34e1fde 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -75,6 +75,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
 TEST_GEN_PROGS_x86_64 += x86_64/emulator_error_test
+TEST_GEN_PROGS_x86_64 += x86_64/emulator_illegal_lea
 TEST_GEN_PROGS_x86_64 += x86_64/fix_hypercall_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
diff --git a/tools/testing/selftests/kvm/x86_64/emulator_illegal_lea.c b/tools/testing/selftests/kvm/x86_64/emulator_illegal_lea.c
new file mode 100644
index 000000000000..5f0704e04fac
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/emulator_illegal_lea.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Test for handling of illegal (ModRM mod = 3) LEA.
+ *
+ * To trigger the emulator and feed it with LEA, we VM-exit on IO (with a
+ * single OUTS), then race decoder's instruction fetch - hoping to replace the
+ * initial IO op with an illegal LEA.
+ */
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#include <string.h>
+#include <pthread.h>
+#include <sys/mman.h>
+
+/* defined in tools/testing/selftests/kvm/lib/x86_64/ucall.c */
+#define UCALL_PIO_PORT ((uint16_t)0x1000)
+
+#define VCPU_ID 0
+#define NOT_UCALL_PORT ((uint16_t)~UCALL_PIO_PORT)
+#define MAX_ATTEMPTS 0x10000
+#define MAYBE_KERNEL(addr) ((int64_t)(addr) < 0)
+
+extern uint32_t race_op;
+
+static void guest_ud_handler(struct ex_regs *regs)
+{
+	GUEST_ASSERT(regs->rip == (uint64_t)&race_op);
+	regs->rip += 4;
+}
+
+__aligned(0x1000) __attribute__((naked)) static void guest_code(void)
+{
+	uint32_t n = 1;
+	void *readable;
+	uint64_t rax;
+
+	do {
+		rax = 0;
+		readable = &guest_code;
+		/*
+		 * This is where
+		 *   3e 3e 3e 6f    outsl (%rsi), %dx
+		 * will be turned into
+		 *   3e 48 8d c0    lea rax (!), rax
+		 * Both instructions padded to 32 bits with DS prefixes.
+		 */
+		asm volatile(".align 8\n\t"
+			     "race_op:\n\t"
+			     "ds\n\t"
+			     "ds\n\t"
+			     "ds\n\t"
+			     "outsl (%[readable]), %[port]\n\t"
+			     ".fill 8, 1, 0x90"
+			     : [rax] "=a" (rax)
+			     : [readable] "S" (readable),
+			       [port] "d" (NOT_UCALL_PORT));
+	} while (!MAYBE_KERNEL(rax) && n++ < MAX_ATTEMPTS);
+
+	ucall(UCALL_DONE, 2, rax, n);
+}
+
+__attribute__((naked)) static void asm_lea(void)
+{
+	asm volatile("ds\n\t"
+		     "lea (%rax), %rax");
+}
+
+__attribute__((naked)) static void asm_outs(void)
+{
+	asm volatile("ds\n\t"
+		     "ds\n\t"
+		     "ds\n\t"
+		     "outsl (%rsi), %dx");
+}
+
+void *racer(void *arg)
+{
+	uint32_t *ptr = arg;
+	uint32_t lea, outs;
+
+	memcpy(&lea, &asm_lea, sizeof(lea));
+	/* set modrm mod = 3 */
+	lea |= 0xc0000000;
+
+	memcpy(&outs, &asm_outs, sizeof(outs));
+
+	while (true) {
+		*ptr = outs;
+		sched_yield();
+		*ptr = lea;
+		sched_yield();
+		pthread_testcancel();
+	};
+}
+
+int check_exit_reason(struct kvm_run *run)
+{
+	switch (run->exit_reason) {
+	case KVM_EXIT_IO:
+		/* race lost: OUTS was executed; ignore and retry */
+		TEST_ASSERT(run->io.port == NOT_UCALL_PORT,
+			    "Unexpected IO port: %u",
+			    run->io.port);
+		return 1;
+	case KVM_EXIT_INTERNAL_ERROR:
+		/* emulator hit bad LEA and bailed out; well done */
+		ASSERT_EQ(*(uint32_t *)&run->emulation_failure.insn_bytes,
+			  *(uint32_t *)&asm_lea | 0xc0000000);
+		TEST_ASSERT(run->internal.suberror == KVM_INTERNAL_ERROR_EMULATION,
+			    "Unexpected suberror: %u",
+			    run->internal.suberror);
+		return 0;
+	default:
+		TEST_FAIL("Unexpected VM exit: %s",
+			  exit_reason_str(run->exit_reason));
+	}
+
+	__builtin_unreachable();
+}
+
+int check_state(struct kvm_run *run, struct ucall *uc)
+{
+	switch (uc->cmd) {
+	case UCALL_NONE:
+		return check_exit_reason(run);
+	case UCALL_DONE:
+		TEST_ASSERT(uc->args[0] == 0,
+			    "Host's address leaked: %#lx (%lu)",
+			    uc->args[0], uc->args[1]);
+		return 0;
+	case UCALL_ABORT:
+		TEST_FAIL("UCALL_ABORT: %s, line %ld\n",
+			  (char *)uc->args[0], uc->args[1]);
+	case UCALL_UNHANDLED:
+		TEST_FAIL("UCALL_UNHANDLED: #%ld\n", uc->args[0]);
+	default:
+		TEST_FAIL("Unexpected ucall");
+	}
+
+	__builtin_unreachable();
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	pthread_t thread;
+	struct ucall uc;
+	int ret;
+
+	ret = mprotect(&guest_code, 0x1000, PROT_READ | PROT_WRITE | PROT_EXEC);
+	TEST_ASSERT(ret == 0, "mprotect() failed: %s", strerror(errno));
+
+	vm = vm_create_default(VCPU_ID, 0, (void *)guest_code);
+	run = vcpu_state(vm, VCPU_ID);
+
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vm, VCPU_ID);
+	vm_install_exception_handler(vm, UD_VECTOR, guest_ud_handler);
+
+	ret = pthread_create(&thread, 0, racer, addr_gva2hva(vm, (vm_vaddr_t)&race_op));
+	TEST_ASSERT(ret == 0, "pthread_create() failed: %s", strerror(ret));
+
+	do {
+		vcpu_run(vm, VCPU_ID);
+		get_ucall(vm, VCPU_ID, &uc);
+	} while (check_state(run, &uc));
+
+	ret = pthread_cancel(thread);
+	TEST_ASSERT(ret == 0, "pthread_cancel() failed: %s", strerror(ret));
+
+	ret = pthread_join(thread, 0);
+	TEST_ASSERT(ret == 0, "pthread_join() failed: %s", strerror(ret));
+
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.32.0

Re: [PATCH 1/2] KVM: x86: emulator: Fix illegal LEA handling

[Sean Christopherson]

On Fri, Jul 29, 2022, Michal Luczaj wrote:
> The emulator mishandles LEA with register source operand. Even though such
> LEA is illegal, it can be encoded and fed to CPU. In which case real
> hardware throws #UD. The emulator, instead, returns address of
> x86_emulate_ctxt._regs. This info leak hurts host's kASLR.
> 
> Tell the decoder that illegal LEA is not to be emulated.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> What the emulator does for LEA is simply:
> 	
> 	case 0x8d: /* lea r16/r32, m */
> 		ctxt->dst.val = ctxt->src.addr.mem.ea;
> 		break;
> 
> And it makes sense if you assume that LEA's source operand is always
> memory. But because there is a race window between VM-exit and the decoder
> instruction fetch, emulator can be force fed an arbitrary opcode of choice.
> Including some that are simply illegal and would cause #UD in normal
> circumstances. Such as a LEA with a register-direct source operand -- for
> which the emulator sets `op->addr.reg`, but reads `op->addr.mem.ea`.
> 
> 		union {
> 			unsigned long *reg;
> 			struct segmented_address {
> 				ulong ea;
> 				unsigned seg;
> 			} mem;
> 			...
> 		} addr;
> 
> Because `reg` and `mem` are in union, emulator reveals address in host's
> memory.
> 
> I hope this patch is not considered an `instr_dual` abuse?

Nope, definitely not abuse.  This is very similar to how both SVM and VMX usurped
"reserved" Mod=3 (register) encodings from SGDT, SIDT, LGDT, LIDT, and INVLPG
to implement virtualization instructions.  I.e. if the Mod=3 encoding is ever
repurposed, then using instr_dual will become necessary.  I'm actually somewhat
surprised that Mod=3 hasn't already been repurposed.

>  arch/x86/kvm/emulate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f8382abe22ff..7c14706372d0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4566,6 +4566,10 @@ static const struct mode_dual mode_dual_63 = {
>  	N, I(DstReg | SrcMem32 | ModRM | Mov, em_movsxd)
>  };
>  
> +static const struct instr_dual instr_dual_8d = {
> +	D(DstReg | SrcMem | ModRM | NoAccess), N
> +};
> +
>  static const struct opcode opcode_table[256] = {
>  	/* 0x00 - 0x07 */
>  	F6ALU(Lock, em_add),
> @@ -4622,7 +4626,7 @@ static const struct opcode opcode_table[256] = {
>  	I2bv(DstMem | SrcReg | ModRM | Mov | PageTable, em_mov),
>  	I2bv(DstReg | SrcMem | ModRM | Mov, em_mov),
>  	I(DstMem | SrcNone | ModRM | Mov | PageTable, em_mov_rm_sreg),
> -	D(ModRM | SrcMem | NoAccess | DstReg),
> +	ID(0, &instr_dual_8d),

Somewhat tentatively because I'm terrible at reading the emulator's decoder, but
this looks correct...

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

>  	I(ImplicitOps | SrcMem16 | ModRM, em_mov_sreg_rm),
>  	G(0, group1A),
>  	/* 0x90 - 0x97 */
> -- 
> 2.32.0
> 

Re: [PATCH 2/2] KVM: selftests: x86: Test illegal LEA handling

[Sean Christopherson]

On Fri, Jul 29, 2022, Michal Luczaj wrote:
> + * To trigger the emulator and feed it with LEA, we VM-exit on IO (with a
> + * single OUTS), then race decoder's instruction fetch - hoping to replace the
> + * initial IO op with an illegal LEA.

Rather than play games with memory, can't we just require and use force_emulation_prefix
to force KVM to emulate a bogus LEA encoding?  emulator.c in KVM-unit-tests already has
most of what you need, e.g. I believe it's just a matter of implementing
test_illegal_lea().  That test already has test_smsw_reg(), which is darn near the
same thing, it just expects a different result (success instead of #UD).

diff --git a/x86/emulator.c b/x86/emulator.c
index cd78e3cb..dd50578d 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -1193,6 +1193,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");

Re: [PATCH 2/2] KVM: selftests: x86: Test illegal LEA handling

[Michal Luczaj]

On 7/29/22 18:53, Sean Christopherson wrote:
> On Fri, Jul 29, 2022, Michal Luczaj wrote:
>> + * To trigger the emulator and feed it with LEA, we VM-exit on IO (with a
>> + * single OUTS), then race decoder's instruction fetch - hoping to replace the
>> + * initial IO op with an illegal LEA.
> 
> Rather than play games with memory, can't we just require and use force_emulation_prefix
> to force KVM to emulate a bogus LEA encoding?  emulator.c in KVM-unit-tests already has
> most of what you need, e.g. I believe it's just a matter of implementing
> test_illegal_lea().  That test already has test_smsw_reg(), which is darn near the
> same thing, it just expects a different result (success instead of #UD).
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index cd78e3cb..dd50578d 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -1193,6 +1193,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");
> 

Ahh, right. Using force_emulation_prefix seems way better. Thanks!

I'll add "kvm-unit-tests" in the subject and send as v2 PATCH.

Michal

[kvm-unit-tests PATCH v2] x86: Test illegal LEA handling

[Michal Luczaj]

Check if the emulator throws #UD on illegal LEA.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
v1 -> v2: Instead of racing decoder make use of force_emulation_prefix

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

diff --git a/x86/emulator.c b/x86/emulator.c
index cd78e3c..c3898f2 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -895,6 +895,24 @@ static void test_mov_dr(uint64_t *mem)
 		report(rax == DR6_ACTIVE_LOW, "mov_dr6");
 }
 
+static void illegal_lea_handler(struct ex_regs *regs)
+{
+	extern char illegal_lea_cont;
+
+	++exceptions;
+	regs->rip = (ulong)&illegal_lea_cont;
+}
+
+static void test_illegal_lea(uint64_t *mem)
+{
+	exceptions = 0;
+	handle_exception(UD_VECTOR, illegal_lea_handler);
+	asm(KVM_FEP ".byte 0x48; .byte 0x8d; .byte 0xc0\n\t"
+	    "illegal_lea_cont:" : : : "rax");
+	report(exceptions == 1, "illegal lea");
+	handle_exception(UD_VECTOR, 0);
+}
+
 static void test_push16(uint64_t *mem)
 {
 	uint64_t rsp1, rsp2;
@@ -1193,6 +1211,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
+		test_illegal_lea(mem);
 	} else {
 		report_skip("skipping register-only tests, "
 			    "use kvm.force_emulation_prefix=1 to enable");
-- 
2.32.0

Re: [kvm-unit-tests PATCH v2] x86: Test illegal LEA handling

[Sean Christopherson]

On Sun, Jul 31, 2022, Michal Luczaj wrote:
> Check if the emulator throws #UD on illegal LEA.

Please explicitly state exactly what illegal LEA is being generated.  Requiring
readers to connect the dots of the LEA opcode and ModR/M encoding is unnecessarily
mean :-)
 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> v1 -> v2: Instead of racing decoder make use of force_emulation_prefix
> 
>  x86/emulator.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index cd78e3c..c3898f2 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -895,6 +895,24 @@ static void test_mov_dr(uint64_t *mem)
>  		report(rax == DR6_ACTIVE_LOW, "mov_dr6");
>  }
>  
> +static void illegal_lea_handler(struct ex_regs *regs)
> +{
> +	extern char illegal_lea_cont;
> +
> +	++exceptions;
> +	regs->rip = (ulong)&illegal_lea_cont;
> +}
> +
> +static void test_illegal_lea(uint64_t *mem)

@mem isn't needed.

> +{
> +	exceptions = 0;
> +	handle_exception(UD_VECTOR, illegal_lea_handler);

No need to use a custom handler (ignore any patterns in emulator.c that suggest
it's "mandatory", emulator is one of the oldest test).  ASM_TRY() can handle all
of this without any globals.

> +	asm(KVM_FEP ".byte 0x48; .byte 0x8d; .byte 0xc0\n\t"
> +	    "illegal_lea_cont:" : : : "rax");

"emulator" is compatible with 32-bit KUT, drop the REX prefix and clobber "eax"
instead of "xax".

> +	report(exceptions == 1, "illegal lea");

Be nice to future debuggers.  Call out what is illegal about LEA, capitalize LEA
to make it more obvious that its an instruction, and print the expected versus
actual.

> +	handle_exception(UD_VECTOR, 0);
> +}

So this?

static void test_illegal_lea(void)
{
	unsigned int vector;

	asm volatile (ASM_TRY("1f")
		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
		      "1:"
		      : : : "memory", "eax");

	vector = exception_vector();
	report(vector == UD_VECTOR,
	       "Wanted #UD on LEA with /reg, got vector = %d", vector);
}

> +
>  static void test_push16(uint64_t *mem)
>  {
>  	uint64_t rsp1, rsp2;
> @@ -1193,6 +1211,7 @@ int main(void)
>  		test_smsw_reg(mem);
>  		test_nop(mem);
>  		test_mov_dr(mem);
> +		test_illegal_lea(mem);
>  	} else {
>  		report_skip("skipping register-only tests, "
>  			    "use kvm.force_emulation_prefix=1 to enable");
> -- 
> 2.32.0
> 

Re: [kvm-unit-tests PATCH v2] x86: Test illegal LEA handling

[Michal Luczaj]

On 8/1/22 18:44, Sean Christopherson wrote:
> On Sun, Jul 31, 2022, Michal Luczaj wrote:
>> +{
>> +	exceptions = 0;
>> +	handle_exception(UD_VECTOR, illegal_lea_handler);
> 
> No need to use a custom handler (ignore any patterns in emulator.c that suggest
> it's "mandatory", emulator is one of the oldest test).  ASM_TRY() can handle all
> of this without any globals.
> ...
> static void test_illegal_lea(void)
> {
> 	unsigned int vector;
> 
> 	asm volatile (ASM_TRY("1f")
> 		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
> 		      "1:"
> 		      : : : "memory", "eax");
> 
> 	vector = exception_vector();
> 	report(vector == UD_VECTOR,
> 	       "Wanted #UD on LEA with /reg, got vector = %d", vector);
> }

I must be missing something important. There is
`handle_exception(UD_VECTOR, 0)` early in `main()` which simply undoes
`handle_exception(6, check_exception_table)` set by `setup_idt()`. If
there's no more exception table walk for #UD, `ASM_TRY` alone can't
possibly work, am I corrent?

If so, am I supposed to restore the `check_exception_table()` handler? Or
maybe using `test_for_exception()` would be more elegant:

static void illegal_lea(void *unused)
{
	asm volatile(KVM_FEP ".byte 0x8d, 0xc0" : : : "memory", "eax");
}

static void test_illegal_lea(void)
{
	bool fault;

	fault = test_for_exception(UD_VECTOR, &illegal_lea, NULL);
	report(fault, "Wanted #UD on LEA with /reg");
}

Thanks for hints,
Michal

Re: [kvm-unit-tests PATCH v2] x86: Test illegal LEA handling

[Sean Christopherson]

On Wed, Aug 03, 2022, Michal Luczaj wrote:
> On 8/1/22 18:44, Sean Christopherson wrote:
> > On Sun, Jul 31, 2022, Michal Luczaj wrote:
> >> +{
> >> +	exceptions = 0;
> >> +	handle_exception(UD_VECTOR, illegal_lea_handler);
> > 
> > No need to use a custom handler (ignore any patterns in emulator.c that suggest
> > it's "mandatory", emulator is one of the oldest test).  ASM_TRY() can handle all
> > of this without any globals.
> > ...
> > static void test_illegal_lea(void)
> > {
> > 	unsigned int vector;
> > 
> > 	asm volatile (ASM_TRY("1f")
> > 		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
> > 		      "1:"
> > 		      : : : "memory", "eax");
> > 
> > 	vector = exception_vector();
> > 	report(vector == UD_VECTOR,
> > 	       "Wanted #UD on LEA with /reg, got vector = %d", vector);
> > }
> 
> I must be missing something important. There is
> `handle_exception(UD_VECTOR, 0)` early in `main()` which simply undoes
> `handle_exception(6, check_exception_table)` set by `setup_idt()`. If
> there's no more exception table walk for #UD, `ASM_TRY` alone can't
> possibly work, am I corrent?

Argh, you're correct, I didn't realize the test zapped the IDT entry.  That's a
bug, the test shouldn't zap entries, the whole point of handle_exception() returning
the old handler is so that the caller can restore it.  Grr.

> If so, am I supposed to restore the `check_exception_table()` handler? Or
> maybe using `test_for_exception()` would be more elegant:

Hmm, I prefer ASM_TRY() over test_for_exception(), having to define a function
just to emit a single instruction is silly.  What I'd really prefer is that we
wouldn't have so many ways for doing the same basic thing (obviously not your
fault, just ranting/whining).

If you have bandwidth, can you create a small series to clean up emulator.c to at
least take a step in the right direction?

  1. Save/restore the handlers.
  2. Use ASM_TRY for the UD_VECTOR cases (KVM_FEP probing and illegal MOVBE)
  3. Add this testcase as described above.

Ideally the test wouldn't use handle_exception() at all, but that's a much bigger
mess and a future problem.

Re: [kvm-unit-tests PATCH v2] x86: Test illegal LEA handling

[Michal Luczaj]

On 8/3/22 01:41, Sean Christopherson wrote:
> On Wed, Aug 03, 2022, Michal Luczaj wrote:
>> If so, am I supposed to restore the `check_exception_table()` handler? Or
>> maybe using `test_for_exception()` would be more elegant:
> 
> Hmm, I prefer ASM_TRY() over test_for_exception(), having to define a function
> just to emit a single instruction is silly.  What I'd really prefer is that we
> wouldn't have so many ways for doing the same basic thing (obviously not your
> fault, just ranting/whining).

All right, ASM_TRY() then.

But it does seem to have a problem with #UD thrown by the FEP-triggered
emulator. Anyway, I've cobbled together a TRY_ASM_PREFIXED variant, but I'm
not sure if that's what you want.

> If you have bandwidth, can you create a small series to clean up emulator.c to at
> least take a step in the right direction?
> 
>   1. Save/restore the handlers.
>   2. Use ASM_TRY for the UD_VECTOR cases (KVM_FEP probing and illegal MOVBE)
>   3. Add this testcase as described above.

Yeah, no problem.

Thanks,
Michal

[kvm-unit-tests PATCH 1/4] 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>
---
I took the liberty of fixing the indentation of functions I've touched.

 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

[kvm-unit-tests PATCH 2/4] 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>
---
I've noticed test_illegal_movbe() does not execute with KVM_FEP.
Just making sure: is it intentional?

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

diff --git a/x86/emulator.c b/x86/emulator.c
index 769a049..d4488a7 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 "mov $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 3/4] 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>
---
 x86/emulator.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index d4488a7..df0bc49 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -896,6 +896,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("1f")
+		      KVM_FEP ".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;
@@ -1187,6 +1201,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

[kvm-unit-tests PATCH 4/4] x86: Extend ASM_TRY to handle #UD thrown by FEP-triggered emulator

[Michal Luczaj]

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

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
While here, I've also took the opportunity to merge both 32 and 64-bit
versions of ASM_TRY() (.dc.a for .long and .quad), but perhaps there
were some reasons for not using .dc.a?

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

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 2a285eb..99cc224 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -80,21 +80,20 @@ typedef struct  __attribute__((packed)) {
 	u16 iomap_base;
 } tss64_t;
 
-#ifdef __x86_64
 #define ASM_TRY(catch)			\
 	"movl $0, %%gs:4 \n\t"		\
 	".pushsection .data.ex \n\t"	\
-	".quad 1111f, " catch "\n\t"	\
+	".dc.a 1111f, " catch "\n\t"	\
 	".popsection \n\t"		\
 	"1111:"
-#else
-#define ASM_TRY(catch)			\
+
+#define ASM_TRY_PREFIXED(prefix, catch)	\
 	"movl $0, %%gs:4 \n\t"		\
 	".pushsection .data.ex \n\t"	\
-	".long 1111f, " catch "\n\t"	\
+	".dc.a 1111f, " catch "\n\t"	\
 	".popsection \n\t"		\
+	prefix "\n\t"			\
 	"1111:"
-#endif
 
 /*
  * selector     32-bit                        64-bit
diff --git a/x86/emulator.c b/x86/emulator.c
index df0bc49..d2a5302 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -900,8 +900,8 @@ static void test_illegal_lea(void)
 {
 	unsigned int vector;
 
-	asm volatile (ASM_TRY("1f")
-		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
+	asm volatile (ASM_TRY_PREFIXED(KVM_FEP, "1f")
+		      ".byte 0x8d; .byte 0xc0\n\t"
 		      "1:"
 		      : : : "memory", "eax");
 
-- 
2.37.1

Re: [kvm-unit-tests PATCH 4/4] x86: Extend ASM_TRY to handle #UD thrown by FEP-triggered emulator

[Sean Christopherson]

On Wed, Aug 03, 2022, Michal Luczaj wrote:
> TRY_ASM() mishandles #UD thrown by the forced-emulation-triggered emulator.
> While the faulting address stored in the exception table points at forced
> emulation prefix, when #UD comes, RIP is 5 bytes (size of KVM_FEP) ahead
> and the exception ends up unhandled.

Ah, but that's only the behavior if FEP is allowed.  If FEP is disabled, then the
#UD will be on the prefix.

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

Heh, I didn't really suggest this because I didn't even realize it was a problem :-)

> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> While here, I've also took the opportunity to merge both 32 and 64-bit
> versions of ASM_TRY() (.dc.a for .long and .quad), but perhaps there
> were some reasons for not using .dc.a?

This should be a separate patch, and probably as the very last patch in case dc.a
isn't viable for whatever reason.  I've never seen/used dc.a so I really have no
idea whether or not it's ok to use.

As a "safe" alternative that can be done before adding ASM_TRY_FEP(), we can steal
the kernel's __ASM_SEL() approach so that you don't have to implement two versions
of ASM_TRY_FEP().  That's worth doing because __ASM_SEL() can probably be used in
other places too.  Patch at the bottom.

>  lib/x86/desc.h | 11 +++++------
>  x86/emulator.c |  4 ++--
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index 2a285eb..99cc224 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -80,21 +80,20 @@ typedef struct  __attribute__((packed)) {
>  	u16 iomap_base;
>  } tss64_t;
>  
> -#ifdef __x86_64
>  #define ASM_TRY(catch)			\
>  	"movl $0, %%gs:4 \n\t"		\
>  	".pushsection .data.ex \n\t"	\
> -	".quad 1111f, " catch "\n\t"	\
> +	".dc.a 1111f, " catch "\n\t"	\
>  	".popsection \n\t"		\
>  	"1111:"
> -#else
> -#define ASM_TRY(catch)			\
> +
> +#define ASM_TRY_PREFIXED(prefix, catch)	\

Rather than a generic ASM_TRY_PREFIXED, I think it makes sense to add an explicit
ASM_TRY_FEP() that takes in only the label and hardcodes the FEP prefix.  The "#UD
skips the prefix" behavior is unique to "successful" forced emulation, so this is
literally the only scenario where skipping a prefix is expected/correct.

*sigh*

That'll require moving the KVM_FEP definition, but that's a good change on its
own.  Probably throw it in lib/x86/processor.h?

>  	"movl $0, %%gs:4 \n\t"		\
>  	".pushsection .data.ex \n\t"	\
> -	".long 1111f, " catch "\n\t"	\
> +	".dc.a 1111f, " catch "\n\t"	\
>  	".popsection \n\t"		\
> +	prefix "\n\t"			\
>  	"1111:"
> -#endif
>  
>  /*
>   * selector     32-bit                        64-bit
> diff --git a/x86/emulator.c b/x86/emulator.c
> index df0bc49..d2a5302 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -900,8 +900,8 @@ static void test_illegal_lea(void)
>  {
>  	unsigned int vector;
>  
> -	asm volatile (ASM_TRY("1f")
> -		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
> +	asm volatile (ASM_TRY_PREFIXED(KVM_FEP, "1f")
> +		      ".byte 0x8d; .byte 0xc0\n\t"

Put this patch earlier in the series, before test_illegal_lea() is introduced.
Both so that there's no unnecessary churn, and also because running the illegal
LEA testcase without this patch will fail.

>  		      "1:"
>  		      : : : "memory", "eax");

---
From: Sean Christopherson <seanjc@google.com>
Date: Wed, 3 Aug 2022 11:09:41 -0700
Subject: [PATCH] x86: Dedup 32-bit vs. 64-bit ASM_TRY() by stealing kernel's
 __ASM_SEL()

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: Sean Christopherson <seanjc@google.com>
---
 lib/x86/desc.h      | 19 +++++--------------
 lib/x86/processor.h | 12 ++++++++++++
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 2a285eb6..e670ebf2 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -80,21 +80,12 @@ typedef struct  __attribute__((packed)) {
 	u16 iomap_base;
 } tss64_t;

-#ifdef __x86_64
-#define ASM_TRY(catch)			\
-	"movl $0, %%gs:4 \n\t"		\
-	".pushsection .data.ex \n\t"	\
-	".quad 1111f, " catch "\n\t"	\
-	".popsection \n\t"		\
+#define ASM_TRY(catch)						\
+	"movl $0, %%gs:4 \n\t"					\
+	".pushsection .data.ex \n\t"				\
+	__ASM_SEL(.long, .quad) " 1111f,  " catch "\n\t"	\
+	".popsection \n\t"					\
 	"1111:"
-#else
-#define ASM_TRY(catch)			\
-	"movl $0, %%gs:4 \n\t"		\
-	".pushsection .data.ex \n\t"	\
-	".long 1111f, " catch "\n\t"	\
-	".popsection \n\t"		\
-	"1111:"
-#endif

 /*
  * selector     32-bit                        64-bit
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 03242206..30e2de87 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

base-commit: a106b30d39425b7afbaa3bbd4aab16fd26d333e7
--

Re: [kvm-unit-tests PATCH 2/4] x86: emulator.c cleanup: Use ASM_TRY for the UD_VECTOR cases

[Sean Christopherson]

On Wed, Aug 03, 2022, Michal Luczaj wrote:
> 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>
> ---
> I've noticed test_illegal_movbe() does not execute with KVM_FEP.
> Just making sure: is it intentional?

It's intentional.  FEP isn't needed because KVM emulates MOVBE on #UD when it's
not supported by the host, e.g. to allow migrating to an older host.

	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f0),
	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f1),

Re: [kvm-unit-tests PATCH 2/4] x86: emulator.c cleanup: Use ASM_TRY for the UD_VECTOR cases

[Paolo Bonzini]

On 8/3/22 20:21, Sean Christopherson wrote:
>> I've noticed test_illegal_movbe() does not execute with KVM_FEP.
>> Just making sure: is it intentional?
> It's intentional.  FEP isn't needed because KVM emulates MOVBE on #UD when it's
> not supported by the host, e.g. to allow migrating to an older host.
> 
> 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f0),
> 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f1),
> 

*puts historian hat on*

The original reason was to test Linux using MOVBE even on non-Atom 
machines, when MOVBE was only on Atoms. :)

Paolo

Re: [kvm-unit-tests PATCH 4/4] x86: Extend ASM_TRY to handle #UD thrown by FEP-triggered emulator

[Paolo Bonzini]

On 8/3/22 20:16, Sean Christopherson wrote:
>> While here, I've also took the opportunity to merge both 32 and 64-bit
>> versions of ASM_TRY() (.dc.a for .long and .quad), but perhaps there
>> were some reasons for not using .dc.a?
> This should be a separate patch, and probably as the very last patch in case dc.a
> isn't viable for whatever reason.  I've never seen/used dc.a so I really have no
> idea whether or not it's ok to use.

Yes, for now I'll squash this, which is similar to Michal's idea but 
using the trusty double underscore prefix:

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 2a285eb..5b21820 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -81,11 +81,12 @@ 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 "\n\t"			\
  	"1111:"
  #else
  #define ASM_TRY(catch)			\
@@ -96,6 +97,8 @@ typedef struct  __attribute__((packed)) {
  	"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 df0bc49..6d2f166 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;
@@ -900,8 +901,8 @@ static void test_illegal_lea(void)
  {
  	unsigned int vector;

-	asm volatile (ASM_TRY("1f")
-		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
+	asm volatile (ASM_TRY_FEP("1f")
+		      ".byte 0x8d; .byte 0xc0\n\t"
  		      "1:"
  		      : : : "memory", "eax");


and the __ASM_SEL() idea can be sent as a separate patch.

Re: [kvm-unit-tests PATCH 2/4] x86: emulator.c cleanup: Use ASM_TRY for the UD_VECTOR cases

[Michal Luczaj]

On Fri, 5 Aug 2022 13:42:40 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 8/3/22 20:21, Sean Christopherson wrote:
> >> I've noticed test_illegal_movbe() does not execute with KVM_FEP.
> >> Just making sure: is it intentional?
> > It's intentional.  FEP isn't needed because KVM emulates MOVBE on #UD when it's
> > not supported by the host, e.g. to allow migrating to an older host.
> > 
> > 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f0),
> > 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f1),
> > 
> 
> *puts historian hat on*
> 
> The original reason was to test Linux using MOVBE even on non-Atom 
> machines, when MOVBE was only on Atoms. :)

So the emulator's logic for MOVBE is meant to be tested only when the
guest supports MOVBE while the host does not?

Michal

Re: [kvm-unit-tests PATCH 2/4] x86: emulator.c cleanup: Use ASM_TRY for the UD_VECTOR cases

[Sean Christopherson]

On Fri, Aug 05, 2022, Michal Luczaj wrote:
> On Fri, 5 Aug 2022 13:42:40 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 8/3/22 20:21, Sean Christopherson wrote:
> > >> I've noticed test_illegal_movbe() does not execute with KVM_FEP.
> > >> Just making sure: is it intentional?
> > > It's intentional.  FEP isn't needed because KVM emulates MOVBE on #UD when it's
> > > not supported by the host, e.g. to allow migrating to an older host.
> > > 
> > > 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f0),
> > > 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f1),
> > > 
> > 
> > *puts historian hat on*
> > 
> > The original reason was to test Linux using MOVBE even on non-Atom 
> > machines, when MOVBE was only on Atoms. :)
> 
> So the emulator's logic for MOVBE is meant to be tested only when the
> guest supports MOVBE while the host does not?

Ah, I see what you're asking.  No, it's perfectly legal to test MOVBE emulation
on hosts that support MOVBE, i.e. using FEP is allowed.  But because KVM emulates
MOVBE on #UD and the KUT testcase is guaranteed to generate a #UD (barring a
hardware bug), there's no need to use FEP.  And not using FEP is advantageous
because it avoids depending on an opt-in non-production module param.

Re: [kvm-unit-tests PATCH 2/4] x86: emulator.c cleanup: Use ASM_TRY for the UD_VECTOR cases

[Nadav Amit]

On Aug 5, 2022, at 12:59 PM, Sean Christopherson <seanjc@google.com> wrote:

> On Fri, Aug 05, 2022, Michal Luczaj wrote:
>> On Fri, 5 Aug 2022 13:42:40 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 8/3/22 20:21, Sean Christopherson wrote:
>>>>> I've noticed test_illegal_movbe() does not execute with KVM_FEP.
>>>>> Just making sure: is it intentional?
>>>> It's intentional.  FEP isn't needed because KVM emulates MOVBE on #UD when it's
>>>> not supported by the host, e.g. to allow migrating to an older host.
>>>> 
>>>> 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f0),
>>>> 	GP(EmulateOnUD | ModRM, &three_byte_0f_38_f1),
>>> 
>>> *puts historian hat on*
>>> 
>>> The original reason was to test Linux using MOVBE even on non-Atom 
>>> machines, when MOVBE was only on Atoms. :)
>> 
>> So the emulator's logic for MOVBE is meant to be tested only when the
>> guest supports MOVBE while the host does not?
> 
> Ah, I see what you're asking.  No, it's perfectly legal to test MOVBE emulation
> on hosts that support MOVBE, i.e. using FEP is allowed.  But because KVM emulates
> MOVBE on #UD and the KUT testcase is guaranteed to generate a #UD (barring a
> hardware bug), there's no need to use FEP.  And not using FEP is advantageous
> because it avoids depending on an opt-in non-production module param.

If history is discussed, the test was created long before FEP. Without FEP,
the way to force the emulator to emulate an instruction was to set the
instruction in memory that is not mapped to the guest. But, as Sean stated,
this test always triggers #UD, so it was not necessary.

The purpose of this test was to check a KVM fix for a bug that was found
during fuzzing:

https://lore.kernel.org/all/5475DC42.6000201@redhat.com/T/#m3a0da02d7c750c28816b08c43cf2ca03252b8bad

Re: [kvm-unit-tests PATCH 2/4] x86: emulator.c cleanup: Use ASM_TRY for the UD_VECTOR cases

[Michal Luczaj]

On Fri, 5 Aug 2022 19:00:12 -0700, Nadav Amit <nadav.amit@gmail.com> wrote:
> On Aug 5, 2022, at 12:59 PM, Sean Christopherson <seanjc@google.com> wrote:
> > On Fri, Aug 05, 2022, Michal Luczaj wrote:
> >> On Fri, 5 Aug 2022 13:42:40 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>> The original reason was to test Linux using MOVBE even on non-Atom 
> >>> machines, when MOVBE was only on Atoms. :)
> >> 
> >> So the emulator's logic for MOVBE is meant to be tested only when the
> >> guest supports MOVBE while the host does not?
> > 
> > Ah, I see what you're asking.  No, it's perfectly legal to test MOVBE emulation
> > on hosts that support MOVBE, i.e. using FEP is allowed.  But because KVM emulates
> > MOVBE on #UD and the KUT testcase is guaranteed to generate a #UD (barring a
> > hardware bug), there's no need to use FEP.  And not using FEP is advantageous
> > because it avoids depending on an opt-in non-production module param.
> 
> If history is discussed, the test was created long before FEP. Without FEP,
> the way to force the emulator to emulate an instruction was to set the
> instruction in memory that is not mapped to the guest. But, as Sean stated,
> this test always triggers #UD, so it was not necessary.
> 
> The purpose of this test was to check a KVM fix for a bug that was found
> during fuzzing:
> 
> https://lore.kernel.org/all/5475DC42.6000201@redhat.com/T/#m3a0da02d7c750c28816b08c43cf2ca03252b8bad

OK, I think I finally get it. Thank you, guys, for all the details.

Michal