On Thu, Jun 29, 2017 at 11:08 AM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote:
> 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.
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!
