| Message ID | 20220731204653.2516-1-mhal@rbox.co (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] x86: Test illegal LEA handling | expand |
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 >
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
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.
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 --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");
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(+)