From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level
Date: Thu, 29 Jun 2017 20:08:57 +0200
Message-ID: <20170629180857.GA3836@potion>
References: <20170629172647.22188-1-rkrcmar@redhat.com>
 <20170629172647.22188-4-rkrcmar@redhat.com>
 <CAM3pwhHSutM85iGMJVerOaw-434hS+DUuzu4tdCdZvF-HLZ5PQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        David Matlack <dmatlack@google.com>
To: Peter Feiner <pfeiner@google.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:52952 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752713AbdF2SJB (ORCPT <rfc822;kvm@vger.kernel.org>);
        Thu, 29 Jun 2017 14:09:01 -0400
Content-Disposition: inline
In-Reply-To: <CAM3pwhHSutM85iGMJVerOaw-434hS+DUuzu4tdCdZvF-HLZ5PQ@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.