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