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

["Radim Krčmář" <rkrcmar@redhat.com>]

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.