* [kvm-unit-tests PATCH] x86: nVMX: add new test for vmread/vmwrite flags preservation
@ 2020-04-14 17:59 Simon Smith
2020-04-14 22:27 ` Oliver Upton
0 siblings, 1 reply; 2+ messages in thread
From: Simon Smith @ 2020-04-14 17:59 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, Simon Smith, Jim Mattson, Peter Shier, Krish Sadhukhan
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 commit 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).
v1: https://www.spinics.net/lists/kvm/msg212817.html
Reviewed-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.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.110.g2183baf09c-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: nVMX: add new test for vmread/vmwrite flags preservation
2020-04-14 17:59 [kvm-unit-tests PATCH] x86: nVMX: add new test for vmread/vmwrite flags preservation Simon Smith
@ 2020-04-14 22:27 ` Oliver Upton
0 siblings, 0 replies; 2+ messages in thread
From: Oliver Upton @ 2020-04-14 22:27 UTC (permalink / raw)
To: Simon Smith
Cc: kvm list, Paolo Bonzini, Jim Mattson, Peter Shier, Krish Sadhukhan
Hi Simon,
On Tue, Apr 14, 2020 at 1:03 PM Simon Smith <brigidsmith@google.com> 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 commit 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).
>
> v1: https://www.spinics.net/lists/kvm/msg212817.html
>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.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;
nit: newline here
> +static void pf_handler(struct ex_regs *regs)
> +{
> + // check that RIP was not improperly advanced and that the
> + // flags value was preserved.
Throughout the patch, could you use C/kernel style comments?
/*
* check that RIP was not improperly advanced and that the
* flags value was perserved.
*/
This should also be used for single line comments.
> + report("RIP has not been advanced!",
> + regs->rip < finish_fault);
> + report("The low byte of RFLAGS was preserved!",
> + ((u8)regs->rflags == ((sentinel | 2) & 0xd7)));
This doesn't compile. The signature of report is:
extern void report(bool pass, const char *msg_fmt, ...)
However, this is completely understandable, as report(...) used to be
parameterized the way you have it :)
> +
> + 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);
Same thing here, you'll need to reorder the parameters.
> +
> + // 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);
report(...) issue also here
> +
> + // 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.110.g2183baf09c-goog
>
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-04-14 22:28 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 17:59 [kvm-unit-tests PATCH] x86: nVMX: add new test for vmread/vmwrite flags preservation Simon Smith
2020-04-14 22:27 ` Oliver Upton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).