| Message ID | 20170629172647.22188-4-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > x86/vmx.c | 4 ++-- > x86/vmx_tests.c | 5 +++-- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/x86/vmx.c b/x86/vmx.c > index 5e3832727f05..56c2c079ebc5 100644 > --- 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 > index 181c3c73cb60..567f7143b427 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -2582,6 +2582,7 @@ static void ept_access_test_setup(void) > unsigned long npages = 1ul << PAGE_1G_ORDER; > unsigned long size = npages * PAGE_SIZE; > unsigned long *page_table = current_page_table(); > + unsigned long pte; > > if (setup_ept(false)) > test_skip("EPT not supported"); > @@ -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. > install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT); > > data->hva[0] = MAGIC_VAL_1; > -- > 2.13.2 >
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.
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!
diff --git a/x86/vmx.c b/x86/vmx.c index 5e3832727f05..56c2c079ebc5 100644 --- 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 index 181c3c73cb60..567f7143b427 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -2582,6 +2582,7 @@ static void ept_access_test_setup(void) unsigned long npages = 1ul << PAGE_1G_ORDER; unsigned long size = npages * PAGE_SIZE; unsigned long *page_table = current_page_table(); + unsigned long pte; if (setup_ept(false)) test_skip("EPT not supported"); @@ -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); install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT); data->hva[0] = MAGIC_VAL_1;
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> --- x86/vmx.c | 4 ++-- x86/vmx_tests.c | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-)