[PATCH kvm-unit-tests] emulator: test 64-bit mov with immediate operand

[PATCH kvm-unit-tests] emulator: test 64-bit mov with immediate operand

[Paolo Bonzini]

MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
Some hypervisor implementations assumed the operand is 32-bit.  This
should never happen because the instruction has no memory operand, but
(like the existing test_mmx_movq_mf) the testcase tricks the emulator
into executing one by mismatching the page tables and the corresponding
TLB entry.

Cc: Nadav Amit <nadav.amit@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/emulator.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index 969944a..d578347 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -694,6 +694,38 @@ static void test_mmx_movq_mf(uint64_t *mem, uint8_t *insn_page,
     handle_exception(MF_VECTOR, 0);
 }
 
+static void test_movabs(uint64_t *mem, uint8_t *insn_page,
+		       uint8_t *alt_insn_page, void *insn_ram)
+{
+    uint64_t val = 0;
+    ulong *cr3 = (ulong *)read_cr3();
+
+    // Pad with RET instructions
+    memset(insn_page, 0xc3, 4096);
+    memset(alt_insn_page, 0xc3, 4096);
+    // Place a trapping instruction in the page to trigger a VMEXIT
+    insn_page[0] = 0x89; // mov %eax, (%rax)
+    insn_page[1] = 0x00;
+    // Place the instruction we want the hypervisor to see in the alternate
+    // page. A buggy hypervisor will fetch a 32-bit immediate and return
+    // 0xffffffffc3c3c3c3.
+    alt_insn_page[0] = 0x48; // mov $0xc3c3c3c3c3c3c3c3, %rcx
+    alt_insn_page[1] = 0xb9;
+
+    // Load the code TLB with insn_page, but point the page tables at
+    // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
+    // This will make the CPU trap on the insn_page instruction but the
+    // hypervisor will see alt_insn_page.
+    install_page(cr3, virt_to_phys(insn_page), insn_ram);
+    // Load code TLB
+    invlpg(insn_ram);
+    asm volatile("call *%0" : : "r"(insn_ram + 3));
+    // Trap, let hypervisor emulate at alt_insn_page
+    install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
+    asm volatile("call *%1" : "=c"(val) : "r"(insn_ram), "a"(mem), "c"(0));
+    report("64-bit mov imm", val == 0xc3c3c3c3c3c3c3c3);
+}
+
 static void test_crosspage_mmio(volatile uint8_t *mem)
 {
     volatile uint16_t w, *pw;
@@ -759,6 +791,7 @@ int main()
 	test_shld_shrd(mem);
 
 	test_mmx_movq_mf(mem, insn_page, alt_insn_page, insn_ram);
+	test_movabs(mem, insn_page, alt_insn_page, insn_ram);
 
 	test_crosspage_mmio(mem);
 
-- 
1.8.0.2

Re: [PATCH kvm-unit-tests] emulator: test 64-bit mov with immediate operand

[Gleb Natapov]

On Thu, Dec 13, 2012 at 01:11:55PM +0100, Paolo Bonzini wrote:
> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> Some hypervisor implementations assumed the operand is 32-bit.  This
> should never happen because the instruction has no memory operand, but
> (like the existing test_mmx_movq_mf) the testcase tricks the emulator
> into executing one by mismatching the page tables and the corresponding
> TLB entry.
> 
BTW how the bug was found? Why instruction was emulated at all? May be
there is bug somewhere that makes KVM emulate something it should not.

> Cc: Nadav Amit <nadav.amit@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  x86/emulator.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index 969944a..d578347 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -694,6 +694,38 @@ static void test_mmx_movq_mf(uint64_t *mem, uint8_t *insn_page,
>      handle_exception(MF_VECTOR, 0);
>  }
>  
> +static void test_movabs(uint64_t *mem, uint8_t *insn_page,
> +		       uint8_t *alt_insn_page, void *insn_ram)
> +{
> +    uint64_t val = 0;
> +    ulong *cr3 = (ulong *)read_cr3();
> +
> +    // Pad with RET instructions
> +    memset(insn_page, 0xc3, 4096);
> +    memset(alt_insn_page, 0xc3, 4096);
> +    // Place a trapping instruction in the page to trigger a VMEXIT
> +    insn_page[0] = 0x89; // mov %eax, (%rax)
> +    insn_page[1] = 0x00;
> +    // Place the instruction we want the hypervisor to see in the alternate
> +    // page. A buggy hypervisor will fetch a 32-bit immediate and return
> +    // 0xffffffffc3c3c3c3.
> +    alt_insn_page[0] = 0x48; // mov $0xc3c3c3c3c3c3c3c3, %rcx
> +    alt_insn_page[1] = 0xb9;
> +
> +    // Load the code TLB with insn_page, but point the page tables at
> +    // alt_insn_page (and keep the data TLB clear, for AMD decode assist).
> +    // This will make the CPU trap on the insn_page instruction but the
> +    // hypervisor will see alt_insn_page.
> +    install_page(cr3, virt_to_phys(insn_page), insn_ram);
> +    // Load code TLB
> +    invlpg(insn_ram);
> +    asm volatile("call *%0" : : "r"(insn_ram + 3));
> +    // Trap, let hypervisor emulate at alt_insn_page
> +    install_page(cr3, virt_to_phys(alt_insn_page), insn_ram);
> +    asm volatile("call *%1" : "=c"(val) : "r"(insn_ram), "a"(mem), "c"(0));
> +    report("64-bit mov imm", val == 0xc3c3c3c3c3c3c3c3);
> +}
> +
>  static void test_crosspage_mmio(volatile uint8_t *mem)
>  {
>      volatile uint16_t w, *pw;
> @@ -759,6 +791,7 @@ int main()
>  	test_shld_shrd(mem);
>  
>  	test_mmx_movq_mf(mem, insn_page, alt_insn_page, insn_ram);
> +	test_movabs(mem, insn_page, alt_insn_page, insn_ram);
>  
>  	test_crosspage_mmio(mem);
>  
> -- 
> 1.8.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH kvm-unit-tests] emulator: test 64-bit mov with immediate operand

[Marcelo Tosatti]

On Thu, Dec 13, 2012 at 02:24:18PM +0200, Gleb Natapov wrote:
> On Thu, Dec 13, 2012 at 01:11:55PM +0100, Paolo Bonzini wrote:
> > MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> > Some hypervisor implementations assumed the operand is 32-bit.  This
> > should never happen because the instruction has no memory operand, but
> > (like the existing test_mmx_movq_mf) the testcase tricks the emulator
> > into executing one by mismatching the page tables and the corresponding
> > TLB entry.
> > 
> BTW how the bug was found? Why instruction was emulated at all? May be
> there is bug somewhere that makes KVM emulate something it should not.

During switch to protected mode. SS.DPL=3, SS.RPL=0.

Re: [PATCH kvm-unit-tests] emulator: test 64-bit mov with immediate operand

[Gleb Natapov]

On Thu, Dec 13, 2012 at 06:45:26PM -0200, Marcelo Tosatti wrote:
> On Thu, Dec 13, 2012 at 02:24:18PM +0200, Gleb Natapov wrote:
> > On Thu, Dec 13, 2012 at 01:11:55PM +0100, Paolo Bonzini wrote:
> > > MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> > > Some hypervisor implementations assumed the operand is 32-bit.  This
> > > should never happen because the instruction has no memory operand, but
> > > (like the existing test_mmx_movq_mf) the testcase tricks the emulator
> > > into executing one by mismatching the page tables and the corresponding
> > > TLB entry.
> > > 
> > BTW how the bug was found? Why instruction was emulated at all? May be
> > there is bug somewhere that makes KVM emulate something it should not.
> 
> During switch to protected mode. SS.DPL=3, SS.RPL=0.
Yes, looks like a bug. We set SS.DPL to 3 to enter vm86 and this leaks
to protected mode. There are a lot of those. I am trying to fix this
mess.

--
			Gleb.

Re: [PATCH kvm-unit-tests] emulator: test 64-bit mov with immediate operand

[Marcelo Tosatti]

On Thu, Dec 13, 2012 at 01:11:55PM +0100, Paolo Bonzini wrote:
> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> Some hypervisor implementations assumed the operand is 32-bit.  This
> should never happen because the instruction has no memory operand, but
> (like the existing test_mmx_movq_mf) the testcase tricks the emulator
> into executing one by mismatching the page tables and the corresponding
> TLB entry.
> 
> Cc: Nadav Amit <nadav.amit@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  x86/emulator.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

Wicked, missed that. If you're unlucky, though, TLB entry can
be gone by then (unlikely, though). 

Applied, thanks.