* [PATCH kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes
@ 2021-04-10 14:42 Paolo Bonzini
2021-04-12 18:40 ` Sean Christopherson
2021-05-18 8:31 ` Yang Weijiang
0 siblings, 2 replies; 3+ messages in thread
From: Paolo Bonzini @ 2021-04-10 14:42 UTC (permalink / raw)
To: kvm; +Cc: seanjc, Yang Weijiang
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes
2021-04-10 14:42 [PATCH kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes Paolo Bonzini
@ 2021-04-12 18:40 ` Sean Christopherson
2021-05-18 8:31 ` Yang Weijiang
1 sibling, 0 replies; 3+ messages in thread
From: Sean Christopherson @ 2021-04-12 18:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, Yang Weijiang
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes
2021-04-10 14:42 [PATCH kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes Paolo Bonzini
2021-04-12 18:40 ` Sean Christopherson
@ 2021-05-18 8:31 ` Yang Weijiang
1 sibling, 0 replies; 3+ messages in thread
From: Yang Weijiang @ 2021-05-18 8:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, seanjc, Yang Weijiang
On Sat, Apr 10, 2021 at 04:42:34PM +0200, Paolo Bonzini wrote:
Hi, Paolo,
Has this patch been merged? We need this patch to fix the issue. Thanks!
> 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(-)
>
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-18 8:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-10 14:42 [PATCH kvm-unit-tests] access: change CR0/CR4/EFER before TLB flushes Paolo Bonzini
2021-04-12 18:40 ` Sean Christopherson
2021-05-18 8:31 ` Yang Weijiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).