2017-06-29 10:51-0700, Peter Feiner:
> On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
