diff mbox series

[kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes

Message ID 20210410144234.32124-2-pbonzini@redhat.com (mailing list archive)
State New
Headers show
Series [kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes | expand

Commit Message

Paolo Bonzini April 10, 2021, 2:42 p.m. UTC
After CR0/CR4/EFER changes a stale TLB entry can be observed, because MOV
to CR4 only invalidates TLB entries if CR4.SMEP is changed from 0 to 1.

The TLB is already flushed in ac_set_expected_status,
but if kvm-unit-tests is migrated to another CPU and CR4 is
changed after the flush, a stale entry can be used.

Reported-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/access.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

Comments

Sean Christopherson April 12, 2021, 6:40 p.m. UTC | #1
On Sat, Apr 10, 2021, Paolo Bonzini wrote:
> After CR0/CR4/EFER changes a stale TLB entry can be observed, because MOV
> to CR4 only invalidates TLB entries if CR4.SMEP is changed from 0 to 1.
> 
> The TLB is already flushed in ac_set_expected_status,
> but if kvm-unit-tests is migrated to another CPU and CR4 is
> changed after the flush, a stale entry can be used.

I don't think the issue is CR0/CR4/EFER being changed after at->virt, I think
it's more precisely setting PT_USER_MASK in ptl2[2] without an INVPLG.  That
happens after CR4.SMEP is cleared, so theoretically it could cause problems even
if the TLB were flushed on _any_ CR4 write, e.g. if the CPU prefetched at->virt
after clearing CR4.SMEP and before setting ptl2[2].PT_USER_MASK.

If my guess is correct, that also means this isn't strictly a migration issue,
it's just that the window is small without migration since it would require the
CPU to grab at->virt between the beginning of ac_set_expected_status() and the
toggling of PT_USER_MASK in ac_test_do_access().

> Reported-by: Yang Weijiang <weijiang.yang@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  x86/access.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/x86/access.c b/x86/access.c
> index 66bd466..e5d5c00 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -448,8 +448,6 @@ fault:
>  
>  static void ac_set_expected_status(ac_test_t *at)
>  {
> -    invlpg(at->virt);
> -
>      if (at->ptep)
>  	at->expected_pte = *at->ptep;
>      at->expected_pde = *at->pdep;
> @@ -561,6 +559,18 @@ static void __ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool,
>  	root = vroot[index];
>      }
>      ac_set_expected_status(at);
> +
> +    set_cr0_wp(F(AC_CPU_CR0_WP));
> +    set_efer_nx(F(AC_CPU_EFER_NX));
> +    set_cr4_pke(F(AC_CPU_CR4_PKE));
> +    if (F(AC_CPU_CR4_PKE)) {
> +        /* WD2=AD2=1, WD1=F(AC_PKU_WD), AD1=F(AC_PKU_AD) */
> +        write_pkru(0x30 | (F(AC_PKU_WD) ? 8 : 0) |
> +                   (F(AC_PKU_AD) ? 4 : 0));
> +    }
> +
> +    set_cr4_smep(F(AC_CPU_CR4_SMEP));
> +    invlpg(at->virt);
>  }
>  
>  static void ac_test_setup_pte(ac_test_t *at, ac_pool_t *pool)
> @@ -644,17 +654,6 @@ static int ac_test_do_access(ac_test_t *at)
>      *((unsigned char *)at->phys) = 0xc3; /* ret */
>  
>      unsigned r = unique;
> -    set_cr0_wp(F(AC_CPU_CR0_WP));
> -    set_efer_nx(F(AC_CPU_EFER_NX));
> -    set_cr4_pke(F(AC_CPU_CR4_PKE));
> -    if (F(AC_CPU_CR4_PKE)) {
> -        /* WD2=AD2=1, WD1=F(AC_PKU_WD), AD1=F(AC_PKU_AD) */
> -        write_pkru(0x30 | (F(AC_PKU_WD) ? 8 : 0) |
> -                   (F(AC_PKU_AD) ? 4 : 0));
> -    }
> -
> -    set_cr4_smep(F(AC_CPU_CR4_SMEP));
> -
>      if (F(AC_ACCESS_TWICE)) {
>  	asm volatile (
>  	    "mov $fixed2, %%rsi \n\t"
> -- 
> 2.30.1
>
diff mbox series

Patch

diff --git a/x86/access.c b/x86/access.c
index 66bd466..e5d5c00 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -448,8 +448,6 @@  fault:
 
 static void ac_set_expected_status(ac_test_t *at)
 {
-    invlpg(at->virt);
-
     if (at->ptep)
 	at->expected_pte = *at->ptep;
     at->expected_pde = *at->pdep;
@@ -561,6 +559,18 @@  static void __ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool,
 	root = vroot[index];
     }
     ac_set_expected_status(at);
+
+    set_cr0_wp(F(AC_CPU_CR0_WP));
+    set_efer_nx(F(AC_CPU_EFER_NX));
+    set_cr4_pke(F(AC_CPU_CR4_PKE));
+    if (F(AC_CPU_CR4_PKE)) {
+        /* WD2=AD2=1, WD1=F(AC_PKU_WD), AD1=F(AC_PKU_AD) */
+        write_pkru(0x30 | (F(AC_PKU_WD) ? 8 : 0) |
+                   (F(AC_PKU_AD) ? 4 : 0));
+    }
+
+    set_cr4_smep(F(AC_CPU_CR4_SMEP));
+    invlpg(at->virt);
 }
 
 static void ac_test_setup_pte(ac_test_t *at, ac_pool_t *pool)
@@ -644,17 +654,6 @@  static int ac_test_do_access(ac_test_t *at)
     *((unsigned char *)at->phys) = 0xc3; /* ret */
 
     unsigned r = unique;
-    set_cr0_wp(F(AC_CPU_CR0_WP));
-    set_efer_nx(F(AC_CPU_EFER_NX));
-    set_cr4_pke(F(AC_CPU_CR4_PKE));
-    if (F(AC_CPU_CR4_PKE)) {
-        /* WD2=AD2=1, WD1=F(AC_PKU_WD), AD1=F(AC_PKU_AD) */
-        write_pkru(0x30 | (F(AC_PKU_WD) ? 8 : 0) |
-                   (F(AC_PKU_AD) ? 4 : 0));
-    }
-
-    set_cr4_smep(F(AC_CPU_CR4_SMEP));
-
     if (F(AC_ACCESS_TWICE)) {
 	asm volatile (
 	    "mov $fixed2, %%rsi \n\t"