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@xxxxxxxxxx>
> ---
> 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;
