On Fri, Oct 29, 2021, Aaron Lewis wrote:
> Rather than assuming which PTE the SMEP test is running on, look it up
> to ensure we have the correct entry. If this test were to run on a
> different page table (ie: run in an L2 test) the wrong PTE would be set.
> Switch to looking up the PTE to avoid this from happening.
>
> Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx>
> ---
> x86/access.c | 9 ++++++---
> x86/cstart64.S | 1 -
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/x86/access.c b/x86/access.c
> index 4725bbd..a4d72d9 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -204,7 +204,7 @@ static void set_cr0_wp(int wp)
> static unsigned set_cr4_smep(int smep)
> {
> unsigned long cr4 = shadow_cr4;
> - extern u64 ptl2[];
> + pteval_t *pte;
> unsigned r;
>
> cr4 &= ~CR4_SMEP_MASK;
> @@ -213,11 +213,14 @@ static unsigned set_cr4_smep(int smep)
> if (cr4 == shadow_cr4)
> return 0;
>
> + pte = get_pte(phys_to_virt(read_cr3()), set_cr4_smep);
What guarantees are there that set_cr4_smep() and the rest of the test are mapped
by the same PTE? I 100% agree the current code is ugly, e.g. I can't remember how
ptl2[2] is guaranteed to be used, but if we're going to fix it then we should aim
for a more robust solution.
> + assert(pte);
> +
> if (smep)
> - ptl2[2] &= ~PT_USER_MASK;
> + *pte &= ~PT_USER_MASK;
> r = write_cr4_checking(cr4);
> if (r || !smep) {
> - ptl2[2] |= PT_USER_MASK;
> + *pte |= PT_USER_MASK;
>
> /* Flush to avoid spurious #PF */
> invlpg((void *)(2 << 21));
This invlpg() should be updated as well.
> 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
> --
> 2.33.1.1089.g2158813163f-goog
>
