x86: gtests: add new test for vmread/vmwrite flags preservation

This commit adds new unit tests for commit a4d956b93904 ("KVM: nVMX:
vmread should not set rflags to specify success in case of #PF")

The two new tests force a vmread and a vmwrite on an unmapped address to cause a #PF and verify that the low byte of %rflags is preserved and that %rip is not advanced. The cherry-pick fixed a bug in vmread, but we include a test for vmwrite as well for completeness.

Before the aforementioned commit, the ALU flags would be incorrectly cleared and %rip would be advanced (for vmread).

Reviewed-by Jim Mattson <[email protected]>

Signed-off-by Simon Smith <[email protected]>

---
  x86/vmx.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 121 insertions(+)

x86/vmx.c +117 -0

index 647ab49408876..e9235ec4fcad9 100644
@@ -32,6 +32,7 @@
  #include "processor.h"
  #include "alloc_page.h"
  #include "vm.h"
+#include "vmalloc.h"
  #include "desc.h"
  #include "vmx.h"
  #include "msr.h"

@@ -368,6 +369,122 @@ static void test_vmwrite_vmread(void)
  	free_page(vmcs);
  }
 
+ulong finish_fault;
+u8 sentinel;
+bool handler_called;
+static void pf_handler(struct ex_regs *regs)
+{
+	// check that RIP was not improperly advanced and that the
+	// flags value was preserved.
+	report("RIP has not been advanced!",
+		regs->rip < finish_fault);
+	report("The low byte of RFLAGS was preserved!",
+		((u8)regs->rflags == ((sentinel | 2) & 0xd7)));
+
+	regs->rip = finish_fault;
+	handler_called = true;
+
+}
+
+static void prep_flags_test_env(void **vpage, struct vmcs **vmcs, handler *old)
+{
+	// get an unbacked address that will cause a #PF
+	*vpage = alloc_vpage();
+
+	// set up VMCS so we have something to read from
+	*vmcs = alloc_page();
+
+	memset(*vmcs, 0, PAGE_SIZE);
+	(*vmcs)->hdr.revision_id = basic.revision;
+	assert(!vmcs_clear(*vmcs));
+	assert(!make_vmcs_current(*vmcs));
+
+	*old = handle_exception(PF_VECTOR, &pf_handler);
+}
+
+static void test_read_sentinel(void)
+{
+	void *vpage;
+	struct vmcs *vmcs;
+	handler old;
+
+	prep_flags_test_env(&vpage, &vmcs, &old);
+
+	// set the proper label
+	extern char finish_read_fault;
+
+	finish_fault = (ulong)&finish_read_fault;
+
+	// execute the vmread instruction that will cause a #PF
+	handler_called = false;
+	asm volatile ("movb %[byte], %%ah\n\t"
+		      "sahf\n\t"
+		      "vmread %[enc], %[val]; finish_read_fault:"
+		      : [val] "=m" (*(u64 *)vpage)
+		      : [byte] "Krm" (sentinel),
+		      [enc] "r" ((u64)GUEST_SEL_SS)
+		      : "cc", "ah"
+		      );
+	report("The #PF handler was invoked", handler_called);
+
+	// restore old #PF handler
+	handle_exception(PF_VECTOR, old);
+}
+
+static void test_vmread_flags_touch(void)
+{
+	// set up the sentinel value in the flags register. we
+	// choose these two values because they candy-stripe
+	// the 5 flags that sahf sets.
+	sentinel = 0x91;
+	test_read_sentinel();
+
+	sentinel = 0x45;
+	test_read_sentinel();
+}
+
+static void test_write_sentinel(void)
+{
+	void *vpage;
+	struct vmcs *vmcs;
+	handler old;
+
+	prep_flags_test_env(&vpage, &vmcs, &old);
+
+	// set the proper label
+	extern char finish_write_fault;
+
+	finish_fault = (ulong)&finish_write_fault;
+
+	// execute the vmwrite instruction that will cause a #PF
+	handler_called = false;
+	asm volatile ("movb %[byte], %%ah\n\t"
+		      "sahf\n\t"
+		      "vmwrite %[val], %[enc]; finish_write_fault:"
+		      : [val] "=m" (*(u64 *)vpage)
+		      : [byte] "Krm" (sentinel),
+		      [enc] "r" ((u64)GUEST_SEL_SS)
+		      : "cc", "ah"
+		      );
+	report("The #PF handler was invoked", handler_called);
+
+	// restore old #PF handler
+	handle_exception(PF_VECTOR, old);
+}
+
+static void test_vmwrite_flags_touch(void)
+{
+	// set up the sentinel value in the flags register. we
+	// choose these two values because they candy-stripe
+	// the 5 flags that sahf sets.
+	sentinel = 0x91;
+	test_write_sentinel();
+
+	sentinel = 0x45;
+	test_write_sentinel();
+}
+
+
  static void test_vmcs_high(void)
  {
  	struct vmcs *vmcs = alloc_page();

Quoted 154 lines · Simon Smith

This commit adds new unit tests for commit a4d956b93904 ("KVM: nVMX:
vmread should not set rflags to specify success in case of #PF")

The two new tests force a vmread and a vmwrite on an unmapped address to cause a #PF and verify that the low byte of %rflags is preserved and that %rip is not advanced. The cherry-pick fixed a bug in vmread, but we include a test for vmwrite as well for completeness.

Before the aforementioned commit, the ALU flags would be incorrectly cleared and %rip would be advanced (for vmread).

Reviewed-by Jim Mattson <[email protected]>

Signed-off-by Simon Smith <[email protected]>

---
  x86/vmx.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 121 insertions(+)

x86/vmx.c +117 -0

index 647ab49408876..e9235ec4fcad9 100644
@@ -32,6 +32,7 @@
  #include "processor.h"
  #include "alloc_page.h"
  #include "vm.h"
+#include "vmalloc.h"
  #include "desc.h"
  #include "vmx.h"
  #include "msr.h"

@@ -368,6 +369,122 @@ static void test_vmwrite_vmread(void)
      free_page(vmcs);
  }
 
+ulong finish_fault;
+u8 sentinel;
+bool handler_called;
+static void pf_handler(struct ex_regs *regs)
+{
+     // check that RIP was not improperly advanced and that the
+     // flags value was preserved.
+     report("RIP has not been advanced!",
+             regs->rip < finish_fault);
+     report("The low byte of RFLAGS was preserved!",
+             ((u8)regs->rflags == ((sentinel | 2) & 0xd7)));
+
+     regs->rip = finish_fault;
+     handler_called = true;
+
+}
+
+static void prep_flags_test_env(void **vpage, struct vmcs **vmcs, handler *old)
+{
+     // get an unbacked address that will cause a #PF
+     *vpage = alloc_vpage();
+
+     // set up VMCS so we have something to read from
+     *vmcs = alloc_page();
+
+     memset(*vmcs, 0, PAGE_SIZE);
+     (*vmcs)->hdr.revision_id = basic.revision;
+     assert(!vmcs_clear(*vmcs));
+     assert(!make_vmcs_current(*vmcs));
+
+     *old = handle_exception(PF_VECTOR, &pf_handler);
+}
+
+static void test_read_sentinel(void)
+{
+     void *vpage;
+     struct vmcs *vmcs;
+     handler old;
+
+     prep_flags_test_env(&vpage, &vmcs, &old);
+
+     // set the proper label
+     extern char finish_read_fault;
+
+     finish_fault = (ulong)&finish_read_fault;
+
+     // execute the vmread instruction that will cause a #PF
+     handler_called = false;
+     asm volatile ("movb %[byte], %%ah\n\t"
+                   "sahf\n\t"
+                   "vmread %[enc], %[val]; finish_read_fault:"
+                   : [val] "=m" (*(u64 *)vpage)
+                   : [byte] "Krm" (sentinel),
+                   [enc] "r" ((u64)GUEST_SEL_SS)
+                   : "cc", "ah"
+                   );
+     report("The #PF handler was invoked", handler_called);
+
+     // restore old #PF handler
+     handle_exception(PF_VECTOR, old);
+}
+
+static void test_vmread_flags_touch(void)
+{
+     // set up the sentinel value in the flags register. we
+     // choose these two values because they candy-stripe
+     // the 5 flags that sahf sets.
+     sentinel = 0x91;
+     test_read_sentinel();
+
+     sentinel = 0x45;
+     test_read_sentinel();
+}
+
+static void test_write_sentinel(void)
+{
+     void *vpage;
+     struct vmcs *vmcs;
+     handler old;
+
+     prep_flags_test_env(&vpage, &vmcs, &old);
+
+     // set the proper label
+     extern char finish_write_fault;
+
+     finish_fault = (ulong)&finish_write_fault;
+
+     // execute the vmwrite instruction that will cause a #PF
+     handler_called = false;
+     asm volatile ("movb %[byte], %%ah\n\t"
+                   "sahf\n\t"
+                   "vmwrite %[val], %[enc]; finish_write_fault:"
+                   : [val] "=m" (*(u64 *)vpage)
+                   : [byte] "Krm" (sentinel),
+                   [enc] "r" ((u64)GUEST_SEL_SS)
+                   : "cc", "ah"
+                   );
+     report("The #PF handler was invoked", handler_called);
+
+     // restore old #PF handler
+     handle_exception(PF_VECTOR, old);
+}
+
+static void test_vmwrite_flags_touch(void)
+{
+     // set up the sentinel value in the flags register. we
+     // choose these two values because they candy-stripe
+     // the 5 flags that sahf sets.
+     sentinel = 0x91;
+     test_write_sentinel();
+
+     sentinel = 0x45;
+     test_write_sentinel();
+}
+
+
  static void test_vmcs_high(void)
  {
      struct vmcs *vmcs = alloc_page();

x86/vmx.c int main(int argc, const char *argv[]) · line 2120

Quoted 10 lines

x86/vmx.c +4 -0

@@ -1994,6 +2111,10 @@ int main(int argc, const char *argv[])
              test_vmcs_lifecycle();
      if (test_wanted("test_vmx_caps", argv, argc))
              test_vmx_caps();
+     if (test_wanted("test_vmread_flags_touch", argv, argc))
+             test_vmread_flags_touch();
+     if (test_wanted("test_vmwrite_flags_touch", argv, argc))
+             test_vmwrite_flags_touch();
 
      /* Balance vmxon from test_vmxon. */
      vmx_off();

Not related to your patch, but just thought of mentioning it here. I find the name 'handle_exception' odd, because we really don't handle an exception in there, we just set the handler passed in and return the old one. May be, we should call it set_exception_handler ?

Reviewed-by Krish Sadhukhan <[email protected]>