diff mbox series

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

Message ID 20220731204653.2516-1-mhal@rbox.co (mailing list archive)
State New
Headers show
Series [kvm-unit-tests,v2] x86: Test illegal LEA handling | expand

Commit Message

Michal Luczaj July 31, 2022, 8:46 p.m. UTC
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(+)

Comments

Sean Christopherson Aug. 1, 2022, 4:44 p.m. UTC | #1
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
>
Michal Luczaj Aug. 2, 2022, 11:07 p.m. UTC | #2
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
Sean Christopherson Aug. 2, 2022, 11:41 p.m. UTC | #3
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.
Michal Luczaj Aug. 3, 2022, 5:21 p.m. UTC | #4
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
diff mbox series

Patch

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");