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