Re: [kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level

From: Peter Feiner <pfeiner@google.com>
To: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	David Matlack <dmatlack@google.com>
Subject: Re: [kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level
Date: Thu, 29 Jun 2017 11:17:24 -0700	[thread overview]
Message-ID: <CAM3pwhHuOK5ivYjoGZ0_JrWuk2s9mXp=fKnFnAxx8gYec10-Wg@mail.gmail.com> (raw)
In-Reply-To: <20170629180857.GA3836@potion>

On Thu, Jun 29, 2017 at 11:08 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2017-06-29 10:51-0700, Peter Feiner:
>> On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> > vmx_EPT_AD_* tests mark the last level as non-present, but that doesn't
>> > mean we cannot look at A/D bits of that last level.
>> > This fixes "EPT - guest physical address is not mapped" in case 3.
>> >
>> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> > ---
>> > diff --git a/x86/vmx.c b/x86/vmx.c
>> > @@ -821,12 +821,12 @@ bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
>> >         for (l = EPT_PAGE_LEVEL; ; --l) {
>> >                 offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>> >                 iter_pte = pt[offset];
>> > -               if (!(iter_pte & (EPT_PRESENT)))
>> > -                       return false;
>> >                 if (l == level)
>> >                         break;
>> >                 if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
>> >                         return false;
>> > +               if (!(iter_pte & (EPT_PRESENT)))
>> > +                       return false;
>> >                 pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
>> >         }
>> >         offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> > @@ -2603,8 +2604,8 @@ static void ept_access_test_setup(void)
>> >          * Make sure nothing's mapped here so the tests that screw with the
>> >          * pml4 entry don't inadvertently break something.
>> >          */
>> > -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
>> > -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
>> > +       TEST_ASSERT(get_ept_pte(pml4, data->gpa, 4, &pte) && pte == 0);
>> > +       TEST_ASSERT(get_ept_pte(pml4, data->gpa + size - 1, 4, &pte) && pte == 0);
>>
>> This isn't right. The PML4 for 1 TiB shouldn't be present ("Make sure
>> nothing's mapped
>> here so the tests that screw with the pml4 entry don't inadvertently
>> break something."),
>> so the walk definitely shouldn't get to the leaf entry.  I'd actually expect
>> get_ept_pte(pml4, data->gpa, 2, &pte) to return false.
>
> The assert asks for 'level 4', which is the topmost level at the moment.
>
> "get_ept_pte(pml4, data->gpa, 2, &pte)" would return false here, even 3,
> as level 4 is not present, but 4 returns true and gives pte of that
> level, because the pte is actually accessible without any walk ...
>
> The patch adds a check for 'pte == 0' afterwards to ensure that nothing
> is actually mapped there (0 implies unset EPT_PRESENT).
>
> Any idea how to improve it?
>
> Thanks.

Sorry, I'm an idiot. I was thinking 4 levels deep, which is pretty
dumb because we use pml4 to indicate the 4th from the bottom :-)

Patch looks good!