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

[Sean Christopherson <seanjc@google.com>]

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");