| Message ID | 159016509437.3131.17229420966309596602.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [5.4] KVM: x86: Fix pkru save/restore when guest CR4.PKE=0, move it to x86.c | expand |
On 22/05/20 18:32, Babu Moger wrote: > [Backported upstream commit 37486135d3a7b03acc7755b63627a130437f066a] > > Though rdpkru and wrpkru are contingent upon CR4.PKE, the PKRU > resource isn't. It can be read with XSAVE and written with XRSTOR. > So, if we don't set the guest PKRU value here(kvm_load_guest_xsave_state), > the guest can read the host value. > > In case of kvm_load_host_xsave_state, guest with CR4.PKE clear could > potentially use XRSTOR to change the host PKRU value. > > While at it, move pkru state save/restore to common code and the > host_pkru field to kvm_vcpu_arch. This will let SVM support protection keys. > > Cc: stable@vger.kernel.org > Reported-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Babu Moger <babu.moger@amd.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/vmx/vmx.c | 18 ------------------ > arch/x86/kvm/x86.c | 17 +++++++++++++++++ > 3 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 4fc61483919a..e204c43ed4b0 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -550,6 +550,7 @@ struct kvm_vcpu_arch { > unsigned long cr4; > unsigned long cr4_guest_owned_bits; > unsigned long cr8; > + u32 host_pkru; > u32 pkru; > u32 hflags; > u64 efer; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 04a8212704c1..728758880cb6 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1384,7 +1384,6 @@ void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > > vmx_vcpu_pi_load(vcpu, cpu); > > - vmx->host_pkru = read_pkru(); > vmx->host_debugctlmsr = get_debugctlmsr(); > } > > @@ -6541,11 +6540,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) > > kvm_load_guest_xcr0(vcpu); > > - if (static_cpu_has(X86_FEATURE_PKU) && > - kvm_read_cr4_bits(vcpu, X86_CR4_PKE) && > - vcpu->arch.pkru != vmx->host_pkru) > - __write_pkru(vcpu->arch.pkru); > - > pt_guest_enter(vmx); > > atomic_switch_perf_msrs(vmx); > @@ -6634,18 +6628,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) > > pt_guest_exit(vmx); > > - /* > - * eager fpu is enabled if PKEY is supported and CR4 is switched > - * back on host, so it is safe to read guest PKRU from current > - * XSAVE. > - */ > - if (static_cpu_has(X86_FEATURE_PKU) && > - kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) { > - vcpu->arch.pkru = rdpkru(); > - if (vcpu->arch.pkru != vmx->host_pkru) > - __write_pkru(vmx->host_pkru); > - } > - > kvm_put_guest_xcr0(vcpu); > > vmx->nested.nested_run_pending = 0; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 5d530521f11d..502a23313e7b 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -821,11 +821,25 @@ void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu) > xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0); > vcpu->guest_xcr0_loaded = 1; > } > + > + if (static_cpu_has(X86_FEATURE_PKU) && > + (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) || > + (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU)) && > + vcpu->arch.pkru != vcpu->arch.host_pkru) > + __write_pkru(vcpu->arch.pkru); > } > EXPORT_SYMBOL_GPL(kvm_load_guest_xcr0); > > void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu) > { > + if (static_cpu_has(X86_FEATURE_PKU) && > + (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) || > + (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU))) { > + vcpu->arch.pkru = rdpkru(); > + if (vcpu->arch.pkru != vcpu->arch.host_pkru) > + __write_pkru(vcpu->arch.host_pkru); > + } > + > if (vcpu->guest_xcr0_loaded) { > if (vcpu->arch.xcr0 != host_xcr0) > xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0); > @@ -3437,6 +3451,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > > kvm_x86_ops->vcpu_load(vcpu, cpu); > > + /* Save host pkru register if supported */ > + vcpu->arch.host_pkru = read_pkru(); > + > fpregs_assert_state_consistent(); > if (test_thread_flag(TIF_NEED_FPU_LOAD)) > switch_fpu_return(); > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
On Fri, May 22, 2020 at 11:32:49AM -0500, Babu Moger wrote: > [Backported upstream commit 37486135d3a7b03acc7755b63627a130437f066a] > > Though rdpkru and wrpkru are contingent upon CR4.PKE, the PKRU > resource isn't. It can be read with XSAVE and written with XRSTOR. > So, if we don't set the guest PKRU value here(kvm_load_guest_xsave_state), > the guest can read the host value. > > In case of kvm_load_host_xsave_state, guest with CR4.PKE clear could > potentially use XRSTOR to change the host PKRU value. > > While at it, move pkru state save/restore to common code and the > host_pkru field to kvm_vcpu_arch. This will let SVM support protection keys. > > Cc: stable@vger.kernel.org > Reported-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Babu Moger <babu.moger@amd.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Now applied, thanks. greg k-h
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 4fc61483919a..e204c43ed4b0 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -550,6 +550,7 @@ struct kvm_vcpu_arch { unsigned long cr4; unsigned long cr4_guest_owned_bits; unsigned long cr8; + u32 host_pkru; u32 pkru; u32 hflags; u64 efer; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 04a8212704c1..728758880cb6 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1384,7 +1384,6 @@ void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) vmx_vcpu_pi_load(vcpu, cpu); - vmx->host_pkru = read_pkru(); vmx->host_debugctlmsr = get_debugctlmsr(); } @@ -6541,11 +6540,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) kvm_load_guest_xcr0(vcpu); - if (static_cpu_has(X86_FEATURE_PKU) && - kvm_read_cr4_bits(vcpu, X86_CR4_PKE) && - vcpu->arch.pkru != vmx->host_pkru) - __write_pkru(vcpu->arch.pkru); - pt_guest_enter(vmx); atomic_switch_perf_msrs(vmx); @@ -6634,18 +6628,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) pt_guest_exit(vmx); - /* - * eager fpu is enabled if PKEY is supported and CR4 is switched - * back on host, so it is safe to read guest PKRU from current - * XSAVE. - */ - if (static_cpu_has(X86_FEATURE_PKU) && - kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) { - vcpu->arch.pkru = rdpkru(); - if (vcpu->arch.pkru != vmx->host_pkru) - __write_pkru(vmx->host_pkru); - } - kvm_put_guest_xcr0(vcpu); vmx->nested.nested_run_pending = 0; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 5d530521f11d..502a23313e7b 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -821,11 +821,25 @@ void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu) xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0); vcpu->guest_xcr0_loaded = 1; } + + if (static_cpu_has(X86_FEATURE_PKU) && + (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) || + (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU)) && + vcpu->arch.pkru != vcpu->arch.host_pkru) + __write_pkru(vcpu->arch.pkru); } EXPORT_SYMBOL_GPL(kvm_load_guest_xcr0); void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu) { + if (static_cpu_has(X86_FEATURE_PKU) && + (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) || + (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU))) { + vcpu->arch.pkru = rdpkru(); + if (vcpu->arch.pkru != vcpu->arch.host_pkru) + __write_pkru(vcpu->arch.host_pkru); + } + if (vcpu->guest_xcr0_loaded) { if (vcpu->arch.xcr0 != host_xcr0) xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0); @@ -3437,6 +3451,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) kvm_x86_ops->vcpu_load(vcpu, cpu); + /* Save host pkru register if supported */ + vcpu->arch.host_pkru = read_pkru(); + fpregs_assert_state_consistent(); if (test_thread_flag(TIF_NEED_FPU_LOAD)) switch_fpu_return();