On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote:
> check_ept_ad() was testing get_ept_pte() for 0, which meant that the PTE
> lookup failed. dff740c00197 ("x86: ept assertions") changed the return
> value in that case to -1, which broke the test.
>
> Returning 0 and -1 is ambiguous, so let's return false instead and get
> the PTE through a pointer argument. This skips a test that was failing
> before, because it was looking at invalid type (the meta -1) instead of
> the pte.
>
> Signed-off-by: Radim Krčmář <rkrcmar@xxxxxxxxxx>
Reviewed-by: Peter Feiner <pfeiner@xxxxxxxxxx>
> ---
> I'm not sure if the case was supposed to be tested more, rather than
> called as "ok".
> ---
> x86/vmx.c | 33 +++++++++++++++++++--------------
> x86/vmx.h | 4 ++--
> x86/vmx_tests.c | 20 ++++++++++----------
> 3 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 9189a66759ec..5e3832727f05 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -809,29 +809,30 @@ void setup_ept_range(unsigned long *pml4, unsigned long start,
>
> /* get_ept_pte : Get the PTE of a given level in EPT,
> @level == 1 means get the latest level*/
> -unsigned long get_ept_pte(unsigned long *pml4,
> - unsigned long guest_addr, int level)
> +bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> + unsigned long *pte)
> {
> int l;
> - unsigned long *pt = pml4, pte;
> + unsigned long *pt = pml4, iter_pte;
> unsigned offset;
>
> assert(level >= 1 && level <= 4);
>
> for (l = EPT_PAGE_LEVEL; ; --l) {
> offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> - pte = pt[offset];
> - if (!(pte & (EPT_PRESENT)))
> - return -1;
> + iter_pte = pt[offset];
> + if (!(iter_pte & (EPT_PRESENT)))
> + return false;
> if (l == level)
> break;
> - if (l < 4 && (pte & EPT_LARGE_PAGE))
> - return -1;
> - pt = (unsigned long *)(pte & EPT_ADDR_MASK);
> + if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
> + return false;
> + pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
> }
> offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> - pte = pt[offset];
> - return pte;
> + if (pte)
> + *pte = pt[offset];
> + return true;
> }
>
> static void clear_ept_ad_pte(unsigned long *pml4, unsigned long guest_addr)
> @@ -894,9 +895,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
> for (l = EPT_PAGE_LEVEL; ; --l) {
> offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>
> - ept_pte = get_ept_pte(pml4, (u64) &pt[offset], 1);
> - if (ept_pte == 0)
> + if (!get_ept_pte(pml4, (u64) &pt[offset], 1, &ept_pte)) {
> + printf("EPT - guest level %d page table is not mapped.\n", l);
> return;
> + }
>
> if (!bad_pt_ad) {
> bad_pt_ad |= (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) != expected_pt_ad;
> @@ -925,7 +927,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
> offset_in_page = guest_addr & ((1 << EPT_LEVEL_SHIFT(l)) - 1);
> gpa = (pt[offset] & PT_ADDR_MASK) | (guest_addr & offset_in_page);
>
> - ept_pte = get_ept_pte(pml4, gpa, 1);
> + if (!get_ept_pte(pml4, gpa, 1, &ept_pte)) {
> + report("EPT - guest physical address is not mapped", false);
> + return;
> + }
> report("EPT - guest physical address A=%d/D=%d",
> (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) == expected_gpa_ad,
> !!(expected_gpa_ad & EPT_ACCESS_FLAG),
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 787466653f8b..efbb320f44c7 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -697,8 +697,8 @@ void install_ept(unsigned long *pml4, unsigned long phys,
> unsigned long guest_addr, u64 perm);
> void setup_ept_range(unsigned long *pml4, unsigned long start,
> unsigned long len, int map_1g, int map_2m, u64 perm);
> -unsigned long get_ept_pte(unsigned long *pml4,
> - unsigned long guest_addr, int level);
> +bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> + unsigned long *pte);
> void set_ept_pte(unsigned long *pml4, unsigned long guest_addr,
> int level, u64 pte_val);
> void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 7be016ce4fbc..181c3c73cb60 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1212,17 +1212,18 @@ static int ept_exit_handler_common(bool have_ad)
> break;
> case 3:
> clear_ept_ad(pml4, guest_cr3, (unsigned long)data_page1);
> - data_page1_pte = get_ept_pte(pml4,
> - (unsigned long)data_page1, 1);
> + TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
> + 1, &data_page1_pte));
> set_ept_pte(pml4, (unsigned long)data_page1,
> 1, data_page1_pte & ~EPT_PRESENT);
> ept_sync(INVEPT_SINGLE, eptp);
> break;
> case 4:
> - data_page1_pte = get_ept_pte(pml4,
> - (unsigned long)data_page1, 2);
> + TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
> + 2, &data_page1_pte));
> data_page1_pte &= PAGE_MASK;
> - data_page1_pte_pte = get_ept_pte(pml4, data_page1_pte, 2);
> + TEST_ASSERT(get_ept_pte(pml4, data_page1_pte,
> + 2, &data_page1_pte_pte));
> set_ept_pte(pml4, data_page1_pte, 2,
> data_page1_pte_pte & ~EPT_PRESENT);
> ept_sync(INVEPT_SINGLE, eptp);
> @@ -1267,7 +1268,7 @@ static int ept_exit_handler_common(bool have_ad)
> if (exit_qual == (EPT_VLT_WR | EPT_VLT_LADDR_VLD |
> EPT_VLT_PADDR))
> vmx_inc_test_stage();
> - set_ept_pte(pml4, (unsigned long)data_page1,
> + set_ept_pte(pml4, (unsigned long)data_page1,
> 1, data_page1_pte | (EPT_PRESENT));
> ept_sync(INVEPT_SINGLE, eptp);
> break;
> @@ -2175,8 +2176,7 @@ static unsigned long ept_twiddle(unsigned long gpa, bool mkhuge, int level,
> unsigned long pte;
>
> /* Screw with the mapping at the requested level. */
> - orig_pte = get_ept_pte(pml4, gpa, level);
> - TEST_ASSERT(orig_pte != -1);
> + TEST_ASSERT(get_ept_pte(pml4, gpa, level, &orig_pte));
> pte = orig_pte;
> if (mkhuge)
> pte = (orig_pte & ~EPT_ADDR_MASK) | data->hpa | EPT_LARGE_PAGE;
> @@ -2603,8 +2603,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_EQ(get_ept_pte(pml4, data->gpa, 4), -1);
> - TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa + size - 1, 4), -1);
> + TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
> + TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
> install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT);
>
> data->hva[0] = MAGIC_VAL_1;
> --
> 2.13.2
>
Very nice fix and cleanup overall. Thanks!
