diff mbox series

[kvm-unit-tests,v2] x86: Look up the PTEs rather than assuming them

Message ID 20211102221456.2662560-1-aaronlewis@google.com (mailing list archive)
State New, archived
Headers show
Series [kvm-unit-tests,v2] x86: Look up the PTEs rather than assuming them | expand

Commit Message

Aaron Lewis Nov. 2, 2021, 10:14 p.m. UTC 
Rather than assuming which PTEs the SMEP test runs on, look them up to
ensure they are correct.  If this test were to run on a different page
table (ie: run in an L2 test) the wrong PTEs would be set.  Switch to
looking up the PTEs to avoid this from happening.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 lib/libcflat.h |  1 +
 lib/x86/vm.c   | 19 +++++++++++++++++++
 lib/x86/vm.h   |  3 +++
 x86/access.c   | 23 +++++++++++++++--------
 x86/cstart64.S |  1 -
 x86/flat.lds   |  1 +
 6 files changed, 39 insertions(+), 9 deletions(-)

Comments

Sean Christopherson Nov. 9, 2021, 10:54 p.m. UTC | #1 
On Tue, Nov 02, 2021, Aaron Lewis wrote:
> Rather than assuming which PTEs the SMEP test runs on, look them up to
> ensure they are correct.  If this test were to run on a different page
> table (ie: run in an L2 test) the wrong PTEs would be set.  Switch to
> looking up the PTEs to avoid this from happening.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  lib/libcflat.h |  1 +
>  lib/x86/vm.c   | 19 +++++++++++++++++++
>  lib/x86/vm.h   |  3 +++
>  x86/access.c   | 23 +++++++++++++++--------
>  x86/cstart64.S |  1 -
>  x86/flat.lds   |  1 +
>  6 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 9bb7e08..c1fd31f 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -35,6 +35,7 @@
>  #define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
>  #define __ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
>  #define ALIGN(x, a)		__ALIGN((x), (a))
> +#define ALIGN_DOWN(x, a)	__ALIGN((x) - ((a) - 1), (a))
>  #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
>  
>  #define MIN(a, b)		((a) < (b) ? (a) : (b))
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 5cd2ee4..cbecddc 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -281,3 +281,22 @@ void force_4k_page(void *addr)
>  	if (pte & PT_PAGE_SIZE_MASK)
>  		split_large_page(ptep, 2);
>  }
> +
> +/*
> + * Call the callback, function, on each page from va_start to va_end.
> + */
> +void walk_pte(void *va_start, void *va_end,

To align with other helpers in vm.c, "void *virt, size_t len" would be more
approriate.  That would also avoid having to document whether or not va_end is
inclusive or exclusive.

> +	      void (*function)(pteval_t *pte, void *va)){

Curly brace goes on a separate line for functions.

> +    struct pte_search search;
> +    uintptr_t curr_addr;

Maybe just curr?  To match other code and maybe squeeze the for-loop on a single
line?

	for (curr = virt; curr < end; curr = ALIGN_DOWN(curr + page_size, page_size)) {

	}

If you do split the loop, it's usually easier to read if all three parts are on
separate lines, e.g.

	for (curr = virt;
	     curr < end;
	     curr = curr = ALIGN_DOWN(curr + page_size, page_size)) {

	}

> +    u64 page_size;

Probably should be size_t.

> +    for (curr_addr = (uintptr_t)va_start; curr_addr < (uintptr_t)va_end;
> +         curr_addr = ALIGN_DOWN(curr_addr + page_size, page_size)) {


> +        search = find_pte_level(current_page_table(), (void *)curr_addr, 1);

Probably worth caching current_page_table().  On x86 with shadow paging, that'll
trigger an exit on every iteration to read CR3.

> +        assert(found_leaf_pte(search));
> +        page_size = 1ul << PGDIR_BITS(search.level);
> +
> +        function(search.pte, (void *)curr_addr);
> +    }
> +}
> diff --git a/lib/x86/vm.h b/lib/x86/vm.h
> index d9753c3..f4ac2c5 100644
> --- a/lib/x86/vm.h
> +++ b/lib/x86/vm.h
> @@ -52,4 +52,7 @@ struct vm_vcpu_info {
>          u64 cr0;
>  };
>  
> +void walk_pte(void *va_start, void *va_end,
> +	      void (*function)(pteval_t *pte, void *va));

A typedef for the function pointer would be helpful.  Maybe pte_callback_t?
And pass "struct pte_search" instead of pteval_t so that future users can get
at the level?

>  #endif
> diff --git a/x86/access.c b/x86/access.c
> index 4725bbd..17a6ef9 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -201,10 +201,21 @@ static void set_cr0_wp(int wp)
>      }
>  }
>  
> +static void clear_user_mask(pteval_t *pte, void *va) {

Curly brace thing again.

> +	*pte &= ~PT_USER_MASK;
> +}
> +
> +static void set_user_mask(pteval_t *pte, void *va) {

And here.

> +	*pte |= PT_USER_MASK;
> +
> +	/* Flush to avoid spurious #PF */
> +	invlpg(va);
> +}
> +
>  static unsigned set_cr4_smep(int smep)
>  {
> +    extern char stext, etext;
>      unsigned long cr4 = shadow_cr4;
> -    extern u64 ptl2[];
>      unsigned r;
>  
>      cr4 &= ~CR4_SMEP_MASK;
diff mbox series

Patch

diff --git a/lib/libcflat.h b/lib/libcflat.h
index 9bb7e08..c1fd31f 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -35,6 +35,7 @@ 
 #define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
 #define __ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
 #define ALIGN(x, a)		__ALIGN((x), (a))
+#define ALIGN_DOWN(x, a)	__ALIGN((x) - ((a) - 1), (a))
 #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
 
 #define MIN(a, b)		((a) < (b) ? (a) : (b))
diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 5cd2ee4..cbecddc 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -281,3 +281,22 @@  void force_4k_page(void *addr)
 	if (pte & PT_PAGE_SIZE_MASK)
 		split_large_page(ptep, 2);
 }
+
+/*
+ * Call the callback, function, on each page from va_start to va_end.
+ */
+void walk_pte(void *va_start, void *va_end,
+	      void (*function)(pteval_t *pte, void *va)){
+    struct pte_search search;
+    uintptr_t curr_addr;
+    u64 page_size;
+
+    for (curr_addr = (uintptr_t)va_start; curr_addr < (uintptr_t)va_end;
+         curr_addr = ALIGN_DOWN(curr_addr + page_size, page_size)) {
+        search = find_pte_level(current_page_table(), (void *)curr_addr, 1);
+        assert(found_leaf_pte(search));
+        page_size = 1ul << PGDIR_BITS(search.level);
+
+        function(search.pte, (void *)curr_addr);
+    }
+}
diff --git a/lib/x86/vm.h b/lib/x86/vm.h
index d9753c3..f4ac2c5 100644
--- a/lib/x86/vm.h
+++ b/lib/x86/vm.h
@@ -52,4 +52,7 @@  struct vm_vcpu_info {
         u64 cr0;
 };
 
+void walk_pte(void *va_start, void *va_end,
+	      void (*function)(pteval_t *pte, void *va));
+
 #endif
diff --git a/x86/access.c b/x86/access.c
index 4725bbd..17a6ef9 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -201,10 +201,21 @@  static void set_cr0_wp(int wp)
     }
 }
 
+static void clear_user_mask(pteval_t *pte, void *va) {
+	*pte &= ~PT_USER_MASK;
+}
+
+static void set_user_mask(pteval_t *pte, void *va) {
+	*pte |= PT_USER_MASK;
+
+	/* Flush to avoid spurious #PF */
+	invlpg(va);
+}
+
 static unsigned set_cr4_smep(int smep)
 {
+    extern char stext, etext;
     unsigned long cr4 = shadow_cr4;
-    extern u64 ptl2[];
     unsigned r;
 
     cr4 &= ~CR4_SMEP_MASK;
@@ -214,14 +225,10 @@  static unsigned set_cr4_smep(int smep)
         return 0;
 
     if (smep)
-        ptl2[2] &= ~PT_USER_MASK;
+        walk_pte(&stext, &etext, clear_user_mask);
     r = write_cr4_checking(cr4);
-    if (r || !smep) {
-        ptl2[2] |= PT_USER_MASK;
-
-	/* Flush to avoid spurious #PF */
-	invlpg((void *)(2 << 21));
-    }
+    if (r || !smep)
+        walk_pte(&stext, &etext, set_user_mask);
     if (!r)
         shadow_cr4 = cr4;
     return r;
diff --git a/x86/cstart64.S b/x86/cstart64.S
index 5c6ad38..4ba9943 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -26,7 +26,6 @@  ring0stacktop:
 .data
 
 .align 4096
-.globl ptl2
 ptl2:
 i = 0
 	.rept 512 * 4
diff --git a/x86/flat.lds b/x86/flat.lds
index a278b56..337bc44 100644
--- a/x86/flat.lds
+++ b/x86/flat.lds
@@ -3,6 +3,7 @@  SECTIONS
     . = 4M + SIZEOF_HEADERS;
     stext = .;
     .text : { *(.init) *(.text) *(.text.*) }
+    etext = .;
     . = ALIGN(4K);
     .data : {
           *(.data)