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@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
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
