[kvm-unit-tests PATCH v2 0/4] Tests for CR0.WP=0/1 r/o write access

[kvm-unit-tests PATCH v2 0/4] Tests for CR0.WP=0/1 r/o write access

[Mathias Krause]

This series adds explicit tests that verify a page fault will occur for
attempts to write to an r/o page while CR0.WP is 1 as well as access is
granted when CR0.WP is 0.

There are existing tests already, e.g. in pks.c, pku.c, smap.c or even
access.c that implicitly test this. However, they all either explicitly
(via INVLPG) or implicitly (via CR3 reload) flush the TLB before doing
the access which might lead to false positives if the access succeeded
before, e.g. because CR0.WP was 0 before.

Better to have an explicit test, especially to back up the changes of
[1] which were missing the emulator case, initially.

Please apply!

Thanks,
Mathias

[1] https://lore.kernel.org/kvm/20230322013731.102955-1-minipli@grsecurity.net/

Mathias Krause (4):
  x86: Use existing CR0.WP / CR4.SMEP bit definitions
  x86/access: CR0.WP toggling write to r/o data test
  x86/access: Forced emulation support
  x86/access: Try emulation for CR0.WP test as well

 x86/access.c | 120 +++++++++++++++++++++++++++++++++++++++++++++------
 x86/pks.c    |   5 +--
 x86/pku.c    |   5 +--
 3 files changed, 110 insertions(+), 20 deletions(-)

-- 
2.39.2

[kvm-unit-tests PATCH v2 1/4] x86: Use existing CR0.WP / CR4.SMEP bit definitions

[Mathias Krause]

Use the existing bit definitions from x86/processor.h instead of
defining our own.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/access.c | 11 ++++-------
 x86/pks.c    |  5 ++---
 x86/pku.c    |  5 ++---
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 4dfec23073f7..203353a3f74f 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -20,9 +20,6 @@ static int invalid_mask;
 #define PT_BASE_ADDR_MASK ((pt_element_t)((((pt_element_t)1 << 36) - 1) & PAGE_MASK))
 #define PT_PSE_BASE_ADDR_MASK (PT_BASE_ADDR_MASK & ~(1ull << 21))
 
-#define CR0_WP_MASK (1UL << 16)
-#define CR4_SMEP_MASK (1UL << 20)
-
 #define PFERR_PRESENT_MASK (1U << 0)
 #define PFERR_WRITE_MASK (1U << 1)
 #define PFERR_USER_MASK (1U << 2)
@@ -239,9 +236,9 @@ static void set_cr0_wp(int wp)
 {
 	unsigned long cr0 = shadow_cr0;
 
-	cr0 &= ~CR0_WP_MASK;
+	cr0 &= ~X86_CR0_WP;
 	if (wp)
-		cr0 |= CR0_WP_MASK;
+		cr0 |= X86_CR0_WP;
 	if (cr0 != shadow_cr0) {
 		write_cr0(cr0);
 		shadow_cr0 = cr0;
@@ -272,9 +269,9 @@ static unsigned set_cr4_smep(ac_test_t *at, int smep)
 	unsigned long cr4 = shadow_cr4;
 	unsigned r;
 
-	cr4 &= ~CR4_SMEP_MASK;
+	cr4 &= ~X86_CR4_SMEP;
 	if (smep)
-		cr4 |= CR4_SMEP_MASK;
+		cr4 |= X86_CR4_SMEP;
 	if (cr4 == shadow_cr4)
 		return 0;
 
diff --git a/x86/pks.c b/x86/pks.c
index ef95fb96c597..bda15efc546d 100644
--- a/x86/pks.c
+++ b/x86/pks.c
@@ -5,7 +5,6 @@
 #include "x86/vm.h"
 #include "x86/msr.h"
 
-#define CR0_WP_MASK      (1UL << 16)
 #define PTE_PKEY_BIT     59
 #define SUPER_BASE        (1 << 23)
 #define SUPER_VAR(v)      (*((__typeof__(&(v))) (((unsigned long)&v) + SUPER_BASE)))
@@ -18,9 +17,9 @@ static void set_cr0_wp(int wp)
 {
     unsigned long cr0 = read_cr0();
 
-    cr0 &= ~CR0_WP_MASK;
+    cr0 &= ~X86_CR0_WP;
     if (wp)
-        cr0 |= CR0_WP_MASK;
+        cr0 |= X86_CR0_WP;
     write_cr0(cr0);
 }
 
diff --git a/x86/pku.c b/x86/pku.c
index 51ff412cef82..6c0d72cc6f81 100644
--- a/x86/pku.c
+++ b/x86/pku.c
@@ -5,7 +5,6 @@
 #include "x86/vm.h"
 #include "x86/msr.h"
 
-#define CR0_WP_MASK      (1UL << 16)
 #define PTE_PKEY_BIT     59
 #define USER_BASE        (1 << 23)
 #define USER_VAR(v)      (*((__typeof__(&(v))) (((unsigned long)&v) + USER_BASE)))
@@ -18,9 +17,9 @@ static void set_cr0_wp(int wp)
 {
     unsigned long cr0 = read_cr0();
 
-    cr0 &= ~CR0_WP_MASK;
+    cr0 &= ~X86_CR0_WP;
     if (wp)
-        cr0 |= CR0_WP_MASK;
+        cr0 |= X86_CR0_WP;
     write_cr0(cr0);
 }
 
-- 
2.39.2

Re: [kvm-unit-tests PATCH v2 1/4] x86: Use existing CR0.WP / CR4.SMEP bit definitions

[Sean Christopherson]

On Fri, Mar 31, 2023, Mathias Krause wrote:
> Use the existing bit definitions from x86/processor.h instead of
> defining our own.

Please avoid pronouns.  E.g. in this case, "our" is really confusing because if
"our" mean KVM-Unit-Tests, then x86/processor.h is also "ours".

  defining one-off versions in individual tests.

[kvm-unit-tests PATCH v2 2/4] x86/access: CR0.WP toggling write to r/o data test

[Mathias Krause]

We already have tests that verify a write access to an r/o page is
successful when CR0.WP=0, but we lack a test that explicitly verifies
that the same access will fail after we set CR0.WP=1 without flushing
any associated TLB entries either explicitly (INVLPG) or implicitly
(write to CR3). Add such a test.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/access.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 203353a3f74f..d1ec99b4fa73 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -575,9 +575,10 @@ fault:
 		at->expected_error &= ~PFERR_FETCH_MASK;
 }
 
-static void ac_set_expected_status(ac_test_t *at)
+static void __ac_set_expected_status(ac_test_t *at, bool flush)
 {
-	invlpg(at->virt);
+	if (flush)
+		invlpg(at->virt);
 
 	if (at->ptep)
 		at->expected_pte = *at->ptep;
@@ -599,6 +600,11 @@ static void ac_set_expected_status(ac_test_t *at)
 	ac_emulate_access(at, at->flags);
 }
 
+static void ac_set_expected_status(ac_test_t *at)
+{
+	__ac_set_expected_status(at, true);
+}
+
 static pt_element_t ac_get_pt(ac_test_t *at, int i, pt_element_t *ptep)
 {
 	pt_element_t pte;
@@ -1061,6 +1067,51 @@ err:
 	return 0;
 }
 
+static int check_write_cr0wp(ac_pt_env_t *pt_env)
+{
+	ac_test_t at;
+	int err = 0;
+
+	ac_test_init(&at, 0xffff923042007000ul, pt_env);
+	at.flags = AC_PDE_PRESENT_MASK | AC_PTE_PRESENT_MASK |
+		   AC_PDE_ACCESSED_MASK | AC_PTE_ACCESSED_MASK |
+		   AC_ACCESS_WRITE_MASK;
+	ac_test_setup_ptes(&at);
+
+	/*
+	 * Under VMX the guest might own the CR0.WP bit, requiring KVM to
+	 * manually keep track of its state where needed, e.g. in the guest
+	 * page table walker.
+	 *
+	 * We load CR0.WP with the inverse value of what would be used during
+	 * the access test and toggle EFER.NX to flush and rebuild the current
+	 * MMU context based on that value.
+	 */
+
+	set_cr0_wp(1);
+	set_efer_nx(1);
+	set_efer_nx(0);
+
+	if (!ac_test_do_access(&at)) {
+		printf("%s: CR0.WP=0 r/o write fail\n", __FUNCTION__);
+		err++;
+	}
+
+	at.flags |= AC_CPU_CR0_WP_MASK;
+	__ac_set_expected_status(&at, false);
+
+	set_cr0_wp(0);
+	set_efer_nx(1);
+	set_efer_nx(0);
+
+	if (!ac_test_do_access(&at)) {
+		printf("%s: CR0.WP=1 r/o write deny fail\n", __FUNCTION__);
+		err++;
+	}
+
+	return err == 0;
+}
+
 static int check_effective_sp_permissions(ac_pt_env_t *pt_env)
 {
 	unsigned long ptr1 = 0xffff923480000000;
@@ -1150,6 +1201,7 @@ const ac_test_fn ac_test_cases[] =
 	check_pfec_on_prefetch_pte,
 	check_large_pte_dirty_for_nowp,
 	check_smep_andnot_wp,
+	check_write_cr0wp,
 	check_effective_sp_permissions,
 };
 
-- 
2.39.2

Re: [kvm-unit-tests PATCH v2 2/4] x86/access: CR0.WP toggling write to r/o data test

[Sean Christopherson]

On Fri, Mar 31, 2023, Mathias Krause wrote:
> We already have tests that verify a write access to an r/o page is

"supervisor write access"

> successful when CR0.WP=0, but we lack a test that explicitly verifies
> that the same access will fail after we set CR0.WP=1 without flushing

s/fail/fault to be more precise about the expected behavior.

> any associated TLB entries either explicitly (INVLPG) or implicitly
> (write to CR3). Add such a test.

Without pronouns:

    KUT has tests that verify a supervisor write access to an r/o page is
    successful when CR0.WP=0, but lacks a test that explicitly verifies that
    the same access faults after setting CR0.WP=1 without flushing any
    associated TLB entries, either explicitly (INVLPG) or implicitly (write
    to CR3). Add such a test.

> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  x86/access.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/access.c b/x86/access.c
> index 203353a3f74f..d1ec99b4fa73 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -575,9 +575,10 @@ fault:
>  		at->expected_error &= ~PFERR_FETCH_MASK;
>  }
>  
> -static void ac_set_expected_status(ac_test_t *at)
> +static void __ac_set_expected_status(ac_test_t *at, bool flush)
>  {
> -	invlpg(at->virt);
> +	if (flush)
> +		invlpg(at->virt);
>  
>  	if (at->ptep)
>  		at->expected_pte = *at->ptep;
> @@ -599,6 +600,11 @@ static void ac_set_expected_status(ac_test_t *at)
>  	ac_emulate_access(at, at->flags);
>  }
>  
> +static void ac_set_expected_status(ac_test_t *at)
> +{
> +	__ac_set_expected_status(at, true);
> +}
> +
>  static pt_element_t ac_get_pt(ac_test_t *at, int i, pt_element_t *ptep)
>  {
>  	pt_element_t pte;
> @@ -1061,6 +1067,51 @@ err:
>  	return 0;
>  }
>  
> +static int check_write_cr0wp(ac_pt_env_t *pt_env)

How about check_toggle_cr0_wp() so that it's (hopefully) obvious that the testcase
does more than just check writes to CR0.WP?  Ah, or maybe the "write" is 

> +{
> +	ac_test_t at;
> +	int err = 0;
> +
> +	ac_test_init(&at, 0xffff923042007000ul, pt_env);
> +	at.flags = AC_PDE_PRESENT_MASK | AC_PTE_PRESENT_MASK |
> +		   AC_PDE_ACCESSED_MASK | AC_PTE_ACCESSED_MASK |
> +		   AC_ACCESS_WRITE_MASK;
> +	ac_test_setup_ptes(&at);
> +
> +	/*
> +	 * Under VMX the guest might own the CR0.WP bit, requiring KVM to
> +	 * manually keep track of its state where needed, e.g. in the guest
> +	 * page table walker.
> +	 *
> +	 * We load CR0.WP with the inverse value of what would be used during

Avoid pronouns in comments too.  If the immediate code is doing something, phrase
the comment as a command (same "rule" as changelogs), e.g.

	/*
	 * Load CR0.WP with the inverse value of what will be used during the
	 * access test, and toggle EFER.NX to coerce KVM into rebuilding the
	 * current MMU context based on the soon-to-be-stale CR0.WP.
	 */

> +	 * the access test and toggle EFER.NX to flush and rebuild the current
> +	 * MMU context based on that value.
> +	 */
> +
> +	set_cr0_wp(1);
> +	set_efer_nx(1);
> +	set_efer_nx(0);

Rather than copy+paste and end up with a superfluous for-loop, through the guts
of the test into a separate inner function, e.g.

  static int __check_toggle_cr0_wp(ac_test_t *at, bool cr0_wp_initially_set)

and then use @cr0_wp_initially_set to set/clear AC_CPU_CR0_WP_MASK.  And for the
printf(), check "at.flags & AC_CPU_CR0_WP_MASK" to determine whether the access
was expected to fault or succeed.  That should make it easy to test all the
combinations.

And then when FEP comes along, add that as a param too.  FEP is probably better
off passing the flag instead of a bool, e.g.

  static int __check_toggle_cr0_wp(ac_test_t *at, bool cr0_wp_initially_set,
				   int fep_flag)

Ah, a better approach would be to capture the flags in a global macro:

  #define TOGGLE_CR0_WP_BASE_FLAGS (base flags without CR0_WP_MASK or FEP_MASK)

and then take the "extra" flags as a param

  static int __check_toggle_cr0_wp(ac_test_t *at, int flags)

which will yield simple code in the helper

  ac->flags = TOGGLE_CR0_WP_BASE_FLAGS | flags;

and somewhat self-documenting code in the caller:

  ret = __check_toggle_cr0_wp(&at, AC_CPU_CR0_WP_MASK);

  ret = __check_toggle_cr0_wp(&at, 0);

  ret = __check_toggle_cr0_wp(&at, AC_CPU_CR0_WP_MASK | FEP_MASK);

  ...

> +
> +	if (!ac_test_do_access(&at)) {
> +		printf("%s: CR0.WP=0 r/o write fail\n", __FUNCTION__);

"fail" is ambiguous.  Did the access fault, or did the test fail?  Better would
be something like (in the inner helper):

		printf("%s: supervisor write with CR0.WP=%d did not %S as expected\n",
		       __FUNCTION__, !!(at->flags & AC_CPU_CR0_WP_MASK),
		       (at->flags & AC_CPU_CR0_WP_MASK) ? "FAULT" : "SUCCEED");

Re: [kvm-unit-tests PATCH v2 2/4] x86/access: CR0.WP toggling write to r/o data test

[Mathias Krause]

On 31.03.23 18:20, Sean Christopherson wrote:
> On Fri, Mar 31, 2023, Mathias Krause wrote:
>> We already have tests that verify a write access to an r/o page is
> 
> "supervisor write access"
> 
>> successful when CR0.WP=0, but we lack a test that explicitly verifies
>> that the same access will fail after we set CR0.WP=1 without flushing
> 
> s/fail/fault to be more precise about the expected behavior.
> 
>> any associated TLB entries either explicitly (INVLPG) or implicitly
>> (write to CR3). Add such a test.
> 
> Without pronouns:
> 
>     KUT has tests that verify a supervisor write access to an r/o page is
>     successful when CR0.WP=0, but lacks a test that explicitly verifies that
>     the same access faults after setting CR0.WP=1 without flushing any
>     associated TLB entries, either explicitly (INVLPG) or implicitly (write
>     to CR3). Add such a test.

Ok.

>>
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>> ---
>>  x86/access.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/x86/access.c b/x86/access.c
>> index 203353a3f74f..d1ec99b4fa73 100644
>> --- a/x86/access.c
>> +++ b/x86/access.c
>> @@ -575,9 +575,10 @@ fault:
>>  		at->expected_error &= ~PFERR_FETCH_MASK;
>>  }
>>  
>> -static void ac_set_expected_status(ac_test_t *at)
>> +static void __ac_set_expected_status(ac_test_t *at, bool flush)
>>  {
>> -	invlpg(at->virt);
>> +	if (flush)
>> +		invlpg(at->virt);
>>  
>>  	if (at->ptep)
>>  		at->expected_pte = *at->ptep;
>> @@ -599,6 +600,11 @@ static void ac_set_expected_status(ac_test_t *at)
>>  	ac_emulate_access(at, at->flags);
>>  }
>>  
>> +static void ac_set_expected_status(ac_test_t *at)
>> +{
>> +	__ac_set_expected_status(at, true);
>> +}
>> +
>>  static pt_element_t ac_get_pt(ac_test_t *at, int i, pt_element_t *ptep)
>>  {
>>  	pt_element_t pte;
>> @@ -1061,6 +1067,51 @@ err:
>>  	return 0;
>>  }
>>  
>> +static int check_write_cr0wp(ac_pt_env_t *pt_env)
> 
> How about check_toggle_cr0_wp() so that it's (hopefully) obvious that the testcase
> does more than just check writes to CR0.WP?  Ah, or maybe the "write" is 

That last sentence lacks a few words. But yeah, the test is about
verifying access permissions of a write operation to a read-only page
wrt toggling CR0.WP.

> 
>> +{
>> +	ac_test_t at;
>> +	int err = 0;
>> +
>> +	ac_test_init(&at, 0xffff923042007000ul, pt_env);
>> +	at.flags = AC_PDE_PRESENT_MASK | AC_PTE_PRESENT_MASK |
>> +		   AC_PDE_ACCESSED_MASK | AC_PTE_ACCESSED_MASK |
>> +		   AC_ACCESS_WRITE_MASK;
>> +	ac_test_setup_ptes(&at);
>> +
>> +	/*
>> +	 * Under VMX the guest might own the CR0.WP bit, requiring KVM to
>> +	 * manually keep track of its state where needed, e.g. in the guest
>> +	 * page table walker.
>> +	 *
>> +	 * We load CR0.WP with the inverse value of what would be used during
> 
> Avoid pronouns in comments too.  If the immediate code is doing something, phrase
> the comment as a command (same "rule" as changelogs), e.g.

Ok, will try to pay more attention to the wording in the future!

> 
> 	/*
> 	 * Load CR0.WP with the inverse value of what will be used during the
> 	 * access test, and toggle EFER.NX to coerce KVM into rebuilding the
> 	 * current MMU context based on the soon-to-be-stale CR0.WP.
> 	 */
> 
>> +	 * the access test and toggle EFER.NX to flush and rebuild the current
>> +	 * MMU context based on that value.
>> +	 */
>> +
>> +	set_cr0_wp(1);
>> +	set_efer_nx(1);
>> +	set_efer_nx(0);
> 
> Rather than copy+paste and end up with a superfluous for-loop, through the guts
> of the test into a separate inner function, e.g.
> 
>   static int __check_toggle_cr0_wp(ac_test_t *at, bool cr0_wp_initially_set)
> 
> and then use @cr0_wp_initially_set to set/clear AC_CPU_CR0_WP_MASK.  And for the
> printf(), check "at.flags & AC_CPU_CR0_WP_MASK" to determine whether the access
> was expected to fault or succeed.  That should make it easy to test all the
> combinations.

Well, I thought of a helper function too and folding the value of CR0.WP
into the error message. But looking at other tests I got the impression,
it's more important to make the test conditions more obvious than it is
to write compact code. This not only makes it easier to see what gets
tested but also shows what the test was intended to do. If, instead, the
error message is based on the value of AC_CPU_CR0_WP_MASK, one not only
has to check what value it was set to but also look up what its meaning
really is, like if set, does it mean CR0.WP=0 or 1?

That said, your below changes make it more obvious, so the helper might
not be such a bad idea in the end.

> 
> And then when FEP comes along, add that as a param too.  FEP is probably better
> off passing the flag instead of a bool, e.g.
> 
>   static int __check_toggle_cr0_wp(ac_test_t *at, bool cr0_wp_initially_set,
> 				   int fep_flag)
> 
> Ah, a better approach would be to capture the flags in a global macro:
> 
>   #define TOGGLE_CR0_WP_BASE_FLAGS (base flags without CR0_WP_MASK or FEP_MASK)
> 
> and then take the "extra" flags as a param
> 
>   static int __check_toggle_cr0_wp(ac_test_t *at, int flags)
> 
> which will yield simple code in the helper
> 
>   ac->flags = TOGGLE_CR0_WP_BASE_FLAGS | flags;
> 
> and somewhat self-documenting code in the caller:
> 
>   ret = __check_toggle_cr0_wp(&at, AC_CPU_CR0_WP_MASK);
> 
>   ret = __check_toggle_cr0_wp(&at, 0);
> 
>   ret = __check_toggle_cr0_wp(&at, AC_CPU_CR0_WP_MASK | FEP_MASK);
> 
>   ...

Yeah, looks good indeed. Will change!

> 
>> +
>> +	if (!ac_test_do_access(&at)) {
>> +		printf("%s: CR0.WP=0 r/o write fail\n", __FUNCTION__);
> 
> "fail" is ambiguous.  Did the access fault, or did the test fail?  Better would
> be something like (in the inner helper):
> 
> 		printf("%s: supervisor write with CR0.WP=%d did not %S as expected\n",
> 		       __FUNCTION__, !!(at->flags & AC_CPU_CR0_WP_MASK),
> 		       (at->flags & AC_CPU_CR0_WP_MASK) ? "FAULT" : "SUCCEED");

Thanks,
Mathias

Re: [kvm-unit-tests PATCH v2 2/4] x86/access: CR0.WP toggling write to r/o data test

[Sean Christopherson]

On Mon, Apr 03, 2023, Mathias Krause wrote:
> On 31.03.23 18:20, Sean Christopherson wrote:
> > On Fri, Mar 31, 2023, Mathias Krause wrote:
> >> +	 * the access test and toggle EFER.NX to flush and rebuild the current
> >> +	 * MMU context based on that value.
> >> +	 */
> >> +
> >> +	set_cr0_wp(1);
> >> +	set_efer_nx(1);
> >> +	set_efer_nx(0);
> > 
> > Rather than copy+paste and end up with a superfluous for-loop, through the guts
> > of the test into a separate inner function, e.g.
> > 
> >   static int __check_toggle_cr0_wp(ac_test_t *at, bool cr0_wp_initially_set)
> > 
> > and then use @cr0_wp_initially_set to set/clear AC_CPU_CR0_WP_MASK.  And for the
> > printf(), check "at.flags & AC_CPU_CR0_WP_MASK" to determine whether the access
> > was expected to fault or succeed.  That should make it easy to test all the
> > combinations.
> 
> Well, I thought of a helper function too and folding the value of CR0.WP
> into the error message. But looking at other tests I got the impression,
> it's more important to make the test conditions more obvious than it is
> to write compact code. 

LOL, you're looking for reasoning/logic where there is none.  The sad truth is
that the vast majority of tests were "written" by copy+paste+tweak.

> This not only makes it easier to see what gets tested but also shows what the
> test was intended to do. If, instead, the error message is based on the value
> of AC_CPU_CR0_WP_MASK, one not only has to check what value it was set to but
> also look up what its meaning really is, like if set, does it mean CR0.WP=0
> or 1?

I generally agree with having self-documenting code and/or assertions, but that
only works to a certain point in tests, especially for tests that are verifying
architectural behavior.  Oftentimes I know what KUT code is doing, but it takes
far too much digging to understand what is actually expected to happen and why.
The architectural behavior cases are especially problematic because there are so
many details that aren't really captured in the test itself.

IMO, given how KUT is structured and what it is testing, documenting the exact
testscase is the role of the error message.  E.g. if the error message explicitly
states the test case, then it documents the code _and_ provides a helpful message
to allow users to quickly triage failures.  Of course, the vast majority of error
messages in KUT are awful IMO, and are a constant source of frustration, but one
can dream :-)

[kvm-unit-tests PATCH v2 3/4] x86/access: Forced emulation support

[Mathias Krause]

Add support to enforce access tests to be handled by the emulator, if
supported by KVM. Exclude it from the ac_test_exec() test, though, to
not slow it down too much.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/access.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index d1ec99b4fa73..ae5e7d8e8892 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -82,6 +82,13 @@ enum {
 	AC_CPU_CR4_SMEP_BIT,
 	AC_CPU_CR4_PKE_BIT,
 
+	NR_AC_TEST_FLAGS,
+
+	/*
+	 *  synthetic flags follow, won't be exercised by ac_test_exec().
+	 */
+	AC_FEP_BIT,
+
 	NR_AC_FLAGS
 };
 
@@ -121,6 +128,8 @@ enum {
 #define AC_CPU_CR4_SMEP_MASK  (1 << AC_CPU_CR4_SMEP_BIT)
 #define AC_CPU_CR4_PKE_MASK   (1 << AC_CPU_CR4_PKE_BIT)
 
+#define AC_FEP_MASK           (1 << AC_FEP_BIT)
+
 const char *ac_names[] = {
 	[AC_PTE_PRESENT_BIT] = "pte.p",
 	[AC_PTE_ACCESSED_BIT] = "pte.a",
@@ -152,6 +161,7 @@ const char *ac_names[] = {
 	[AC_CPU_CR0_WP_BIT] = "cr0.wp",
 	[AC_CPU_CR4_SMEP_BIT] = "cr4.smep",
 	[AC_CPU_CR4_PKE_BIT] = "cr4.pke",
+	[AC_FEP_BIT] = "fep",
 };
 
 static inline void *va(pt_element_t phys)
@@ -190,6 +200,7 @@ typedef struct {
 
 static void ac_test_show(ac_test_t *at);
 
+static bool fep_available;
 static unsigned long shadow_cr0;
 static unsigned long shadow_cr3;
 static unsigned long shadow_cr4;
@@ -396,7 +407,7 @@ static void ac_test_init(ac_test_t *at, unsigned long virt, ac_pt_env_t *pt_env)
 static int ac_test_bump_one(ac_test_t *at)
 {
 	at->flags = ((at->flags | invalid_mask) + 1) & ~invalid_mask;
-	return at->flags < (1 << NR_AC_FLAGS);
+	return at->flags < (1 << NR_AC_TEST_FLAGS);
 }
 
 #define F(x)  ((flags & x##_MASK) != 0)
@@ -799,10 +810,13 @@ static int ac_test_do_access(ac_test_t *at)
 
 	if (F(AC_ACCESS_TWICE)) {
 		asm volatile ("mov $fixed2, %%rsi \n\t"
-			      "mov (%[addr]), %[reg] \n\t"
+			      "cmp $0, %[fep] \n\t"
+			      "jz 1f \n\t"
+			      KVM_FEP
+			      "1: mov (%[addr]), %[reg] \n\t"
 			      "fixed2:"
 			      : [reg]"=r"(r), [fault]"=a"(fault), "=b"(e)
-			      : [addr]"r"(at->virt)
+			      : [addr]"r"(at->virt), [fep]"r"(F(AC_FEP))
 			      : "rsi");
 		fault = 0;
 	}
@@ -823,9 +837,15 @@ static int ac_test_do_access(ac_test_t *at)
 		      "jnz 2f \n\t"
 		      "cmp $0, %[write] \n\t"
 		      "jnz 1f \n\t"
-		      "mov (%[addr]), %[reg] \n\t"
+		      "cmp $0, %[fep] \n\t"
+		      "jz 0f \n\t"
+		      KVM_FEP
+		      "0: mov (%[addr]), %[reg] \n\t"
 		      "jmp done \n\t"
-		      "1: mov %[reg], (%[addr]) \n\t"
+		      "1: cmp $0, %[fep] \n\t"
+		      "jz 0f \n\t"
+		      KVM_FEP
+		      "0: mov %[reg], (%[addr]) \n\t"
 		      "jmp done \n\t"
 		      "2: call *%[addr] \n\t"
 		      "done: \n"
@@ -843,6 +863,7 @@ static int ac_test_do_access(ac_test_t *at)
 			[write]"r"(F(AC_ACCESS_WRITE)),
 			[user]"r"(F(AC_ACCESS_USER)),
 			[fetch]"r"(F(AC_ACCESS_FETCH)),
+			[fep]"r"(F(AC_FEP)),
 			[user_ds]"i"(USER_DS),
 			[user_cs]"i"(USER_CS),
 			[user_stack_top]"r"(user_stack + sizeof user_stack),
@@ -1228,6 +1249,12 @@ int ac_test_run(int pt_levels)
 		invalid_mask |= AC_PTE_BIT36_MASK;
 	}
 
+	fep_available = is_fep_available();
+	if (!fep_available) {
+		printf("FEP not available, skipping emulation tests\n");
+		invalid_mask |= AC_FEP_MASK;
+	}
+
 	ac_env_int(&pt_env, pt_levels);
 	ac_test_init(&at, 0xffff923400000000ul, &pt_env);
 
-- 
2.39.2

Re: [kvm-unit-tests PATCH v2 3/4] x86/access: Forced emulation support

[Sean Christopherson]

On Fri, Mar 31, 2023, Mathias Krause wrote:
> Add support to enforce access tests to be handled by the emulator, if
> supported by KVM. Exclude it from the ac_test_exec() test, though, to
> not slow it down too much.

/enthusiastic high five

I was going to ask if you could extend the test to utilize FEP, and woke up this
morning to see it already done.  Thanks!!!!!

> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---

...

> @@ -152,6 +161,7 @@ const char *ac_names[] = {
>  	[AC_CPU_CR0_WP_BIT] = "cr0.wp",
>  	[AC_CPU_CR4_SMEP_BIT] = "cr4.smep",
>  	[AC_CPU_CR4_PKE_BIT] = "cr4.pke",
> +	[AC_FEP_BIT] = "fep",
>  };
>  
>  static inline void *va(pt_element_t phys)
> @@ -190,6 +200,7 @@ typedef struct {
>  
>  static void ac_test_show(ac_test_t *at);
>  
> +static bool fep_available;

I don't think fep_available needs to be captured in a global bool, the usage in
the CR0_WP test can do

	if (invalid_mask & AC_FEP_MASK)
		<skip>

Re: [kvm-unit-tests PATCH v2 3/4] x86/access: Forced emulation support

[Mathias Krause]

On 31.03.23 18:24, Sean Christopherson wrote:
> On Fri, Mar 31, 2023, Mathias Krause wrote:
>> Add support to enforce access tests to be handled by the emulator, if
>> supported by KVM. Exclude it from the ac_test_exec() test, though, to
>> not slow it down too much.
> 
> /enthusiastic high five

*clap*

> 
> I was going to ask if you could extend the test to utilize FEP, and woke up this
> morning to see it already done.  Thanks!!!!!
> 
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>> ---
> 
> ...
> 
>> @@ -152,6 +161,7 @@ const char *ac_names[] = {
>>  	[AC_CPU_CR0_WP_BIT] = "cr0.wp",
>>  	[AC_CPU_CR4_SMEP_BIT] = "cr4.smep",
>>  	[AC_CPU_CR4_PKE_BIT] = "cr4.pke",
>> +	[AC_FEP_BIT] = "fep",
>>  };
>>  
>>  static inline void *va(pt_element_t phys)
>> @@ -190,6 +200,7 @@ typedef struct {
>>  
>>  static void ac_test_show(ac_test_t *at);
>>  
>> +static bool fep_available;
> 
> I don't think fep_available needs to be captured in a global bool, the usage in
> the CR0_WP test can do
> 
> 	if (invalid_mask & AC_FEP_MASK)
> 		<skip>

Ok, makes it a little uglier, IMHO, but will change.

Again, I was just looking at other tests and copied what I thought was
sensible. But looking at x86/emulator.c again, I see it's not consistent
either. It caches the value in test_mov_pop_ss_code_db() but does not in
main().

Maybe I should just use is_fep_available() twice as well, as the
additional exception handling shouldn't matter much.

Thanks,
Mathias

[kvm-unit-tests PATCH v2 4/4] x86/access: Try emulation for CR0.WP test as well

[Mathias Krause]

Enhance the CR.WP toggling test to do additional tests via the emulator
as these used to trigger bugs when CR0.WP is guest owned.

Link: https://lore.kernel.org/kvm/ea3a8fbc-2bf8-7442-e498-3e5818384c83@grsecurity.net/
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/access.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index ae5e7d8e8892..21967434bc18 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -1107,27 +1107,43 @@ static int check_write_cr0wp(ac_pt_env_t *pt_env)
 	 * We load CR0.WP with the inverse value of what would be used during
 	 * the access test and toggle EFER.NX to flush and rebuild the current
 	 * MMU context based on that value.
+	 *
+	 * This used to trigger a bug in the emulator we try to test via FEP.
 	 */
+	for (;;) {
+		const char *fep = (at.flags & AC_FEP_MASK) ? "FEP " : "";
 
-	set_cr0_wp(1);
-	set_efer_nx(1);
-	set_efer_nx(0);
+		set_cr0_wp(1);
+		set_efer_nx(1);
+		set_efer_nx(0);
 
-	if (!ac_test_do_access(&at)) {
-		printf("%s: CR0.WP=0 r/o write fail\n", __FUNCTION__);
-		err++;
-	}
+		if (!ac_test_do_access(&at)) {
+			printf("%s: %sCR0.WP=0 r/o write fail\n", __FUNCTION__, fep);
+			err++;
+		}
 
-	at.flags |= AC_CPU_CR0_WP_MASK;
-	__ac_set_expected_status(&at, false);
+		at.flags |= AC_CPU_CR0_WP_MASK;
+		__ac_set_expected_status(&at, false);
 
-	set_cr0_wp(0);
-	set_efer_nx(1);
-	set_efer_nx(0);
+		set_cr0_wp(0);
+		set_efer_nx(1);
+		set_efer_nx(0);
 
-	if (!ac_test_do_access(&at)) {
-		printf("%s: CR0.WP=1 r/o write deny fail\n", __FUNCTION__);
-		err++;
+		if (!ac_test_do_access(&at)) {
+			printf("%s: %sCR0.WP=1 r/o write deny fail\n", __FUNCTION__, fep);
+			err++;
+		}
+
+		if (!fep_available)
+			break;
+
+		if (at.flags & AC_FEP_MASK)
+			break;
+
+		/* Re-test via the emulator */
+		at.flags |= AC_FEP_MASK;
+		at.flags ^= AC_CPU_CR0_WP_MASK;
+		__ac_set_expected_status(&at, false);
 	}
 
 	return err == 0;
-- 
2.39.2

Re: [kvm-unit-tests PATCH v2 4/4] x86/access: Try emulation for CR0.WP test as well

[Sean Christopherson]

On Fri, Mar 31, 2023, Mathias Krause wrote:
> Enhance the CR.WP toggling test to do additional tests via the emulator
> as these used to trigger bugs when CR0.WP is guest owned.
> 
> Link: https://lore.kernel.org/kvm/ea3a8fbc-2bf8-7442-e498-3e5818384c83@grsecurity.net/
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  x86/access.c | 46 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/x86/access.c b/x86/access.c
> index ae5e7d8e8892..21967434bc18 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -1107,27 +1107,43 @@ static int check_write_cr0wp(ac_pt_env_t *pt_env)
>  	 * We load CR0.WP with the inverse value of what would be used during
>  	 * the access test and toggle EFER.NX to flush and rebuild the current
>  	 * MMU context based on that value.
> +	 *
> +	 * This used to trigger a bug in the emulator we try to test via FEP.
>  	 */
> +	for (;;) {

As suggested in patch 2, I'm pretty sure it's easier to use a helper, and have
the print statement react to at->flags.