[kvm-unit-tests PATCH] x86: gtests: add new test for vmread/vmwrite flags preservation

[kvm-unit-tests PATCH] x86: gtests: add new test for vmread/vmwrite flags preservation

[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 <jmattson@google.com>
Signed-off-by: Simon Smith <brigidsmith@google.com>
---
 x86/vmx.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/x86/vmx.c b/x86/vmx.c
index 647ab49408876..e9235ec4fcad9 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -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();
@@ -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();
-- 
2.26.0.292.g33ef6b2f38-goog

Re: [kvm-unit-tests PATCH] x86: gtests: add new test for vmread/vmwrite flags preservation

[Krish Sadhukhan]

On 4/6/20 3:55 PM, Simon Smith wrote:
> 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 <jmattson@google.com>
> Signed-off-by: Simon Smith <brigidsmith@google.com>
> ---
>   x86/vmx.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 121 insertions(+)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 647ab49408876..e9235ec4fcad9 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -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();
> @@ -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 <krish.sadhukhan@oracle.com>

Re: [kvm-unit-tests PATCH] x86: gtests: add new test for vmread/vmwrite flags preservation

[Peter Shier]

On Mon, Apr 6, 2020 at 6:42 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
> On 4/6/20 3:55 PM, Simon Smith wrote:
> > 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 <jmattson@google.com>
> > Signed-off-by: Simon Smith <brigidsmith@google.com>
> > ---
> >   x86/vmx.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 121 insertions(+)
> >
> > diff --git a/x86/vmx.c b/x86/vmx.c
> > index 647ab49408876..e9235ec4fcad9 100644
> > --- a/x86/vmx.c
> > +++ b/x86/vmx.c
> > @@ -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();
> > @@ -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 <krish.sadhukhan@oracle.com>

Reviewed-by: Peter Shier <pshier@google.com>