Messages in this thread | | | Subject | Re: [PATCH 5.4 12/47] KVM: x86: avoid incorrect writes to host MSR_IA32_SPEC_CTRL | From | Paolo Bonzini <> | Date | Fri, 26 Feb 2021 12:27:49 +0100 |
| |
On 26/02/21 12:03, Thomas Lamprecht wrote: > On 04.01.21 16:57, Greg Kroah-Hartman wrote: >> From: Paolo Bonzini <pbonzini@redhat.com> >> >> [ Upstream commit 6441fa6178f5456d1d4b512c08798888f99db185 ] >> >> If the guest is configured to have SPEC_CTRL but the host does not >> (which is a nonsensical configuration but these are not explicitly >> forbidden) then a host-initiated MSR write can write vmx->spec_ctrl >> (respectively svm->spec_ctrl) and trigger a #GP when KVM tries to >> restore the host value of the MSR. Add a more comprehensive check >> for valid bits of SPEC_CTRL, covering host CPUID flags and, >> since we are at it and it is more correct that way, guest CPUID >> flags too. >> >> For AMD, remove the unnecessary is_guest_mode check around setting >> the MSR interception bitmap, so that the code looks the same as >> for Intel. >> > > A git bisect between 5.4.86 and 5.4.98 showed that this breaks boot of QEMU > guests running Windows 10 20H2 on AMD Ryzen X3700 CPUs with a BSOD showing > "KERNEL SECURITY CHECK FAILURE". > > Reverting this commit or setting the CPU type of the QEMU/KVM command from > host to qemu64 allows one to boot Windows 10 in the VM again. > > I found a followup, commit 841c2be09fe4f495fe5224952a419bd8c7e5b455 [0], > which has a fixes line for this commit and mentions Zen2 AMD CPUs (which > the X3700 is). > Applying a backport of that commit on top of 5.4.98 stable tree fixed the > issue here see below for the backport I used, it applies also cleanly on the > more current 5.4.101 release. > > So can you please add this patch to the stable trees that backported the > problematic upstream commit 6441fa6178f5456d1d4b512c08798888f99db185 ? > > If I should submit this in any other way just ask, was not sure about > what works best with a patch which cannot be cherry-picked cleanly.
Ok, I'll submit it.
Thanks for the testing.
Paolo
> > cheers, > Thomas > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=841c2be09fe4f495fe5224952a419bd8c7e5b455 > > ----8<--- > From: Maxim Levitsky <mlevitsk@redhat.com> > Date: Wed, 8 Jul 2020 14:57:31 +0300 > Subject: [PATCH] kvm: x86: replace kvm_spec_ctrl_test_value with runtime test > on the host > > To avoid complex and in some cases incorrect logic in > kvm_spec_ctrl_test_value, just try the guest's given value on the host > processor instead, and if it doesn't #GP, allow the guest to set it. > > One such case is when host CPU supports STIBP mitigation > but doesn't support IBRS (as is the case with some Zen2 AMD cpus), > and in this case we were giving guest #GP when it tried to use STIBP > > The reason why can can do the host test is that IA32_SPEC_CTRL msr is > passed to the guest, after the guest sets it to a non zero value > for the first time (due to performance reasons), > and as as result of this, it is pointless to emulate #GP condition on > this first access, in a different way than what the host CPU does. > > This is based on a patch from Sean Christopherson, who suggested this idea. > > Fixes: 6441fa6178f5 ("KVM: x86: avoid incorrect writes to host MSR_IA32_SPEC_CTRL") > Cc: stable@vger.kernel.org > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > Message-Id: <20200708115731.180097-1-mlevitsk@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > (cherry picked from commit 841c2be09fe4f495fe5224952a419bd8c7e5b455) > [ Thomas: resolved merge conflict in arch/x86/kvm/x86.h and > replicated the change to arch/x86/kvm/svm/svm.c to the in 5.4 not > yet moved out arch/x86/kvm/svm.c ] > Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> > --- > arch/x86/kvm/svm.c | 2 +- > arch/x86/kvm/vmx/vmx.c | 2 +- > arch/x86/kvm/x86.c | 38 +++++++++++++++++++++----------------- > arch/x86/kvm/x86.h | 2 +- > 4 files changed, 24 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 8d386efc2540..372a958bec72 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -4327,7 +4327,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > !guest_has_spec_ctrl_msr(vcpu)) > return 1; > > - if (data & ~kvm_spec_ctrl_valid_bits(vcpu)) > + if (kvm_spec_ctrl_test_value(data)) > return 1; > > svm->spec_ctrl = data; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index e7fd2f00edc1..e177848a3631 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1974,7 +1974,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > !guest_has_spec_ctrl_msr(vcpu)) > return 1; > > - if (data & ~kvm_spec_ctrl_valid_bits(vcpu)) > + if (kvm_spec_ctrl_test_value(data)) > return 1; > > vmx->spec_ctrl = data; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index f5a827150664..1330fc4dc7c5 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10376,28 +10376,32 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu) > } > EXPORT_SYMBOL_GPL(kvm_arch_no_poll); > > -u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu) > + > +int kvm_spec_ctrl_test_value(u64 value) > { > - uint64_t bits = SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD; > + /* > + * test that setting IA32_SPEC_CTRL to given value > + * is allowed by the host processor > + */ > > - /* The STIBP bit doesn't fault even if it's not advertised */ > - if (!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > - !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS)) > - bits &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP); > - if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL) && > - !boot_cpu_has(X86_FEATURE_AMD_IBRS)) > - bits &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP); > + u64 saved_value; > + unsigned long flags; > + int ret = 0; > > - if (!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL_SSBD) && > - !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) > - bits &= ~SPEC_CTRL_SSBD; > - if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD) && > - !boot_cpu_has(X86_FEATURE_AMD_SSBD)) > - bits &= ~SPEC_CTRL_SSBD; > + local_irq_save(flags); > > - return bits; > + if (rdmsrl_safe(MSR_IA32_SPEC_CTRL, &saved_value)) > + ret = 1; > + else if (wrmsrl_safe(MSR_IA32_SPEC_CTRL, value)) > + ret = 1; > + else > + wrmsrl(MSR_IA32_SPEC_CTRL, saved_value); > + > + local_irq_restore(flags); > + > + return ret; > } > -EXPORT_SYMBOL_GPL(kvm_spec_ctrl_valid_bits); > +EXPORT_SYMBOL_GPL(kvm_spec_ctrl_test_value); > > EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit); > EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_fast_mmio); > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > index 301286d92432..c520d373790a 100644 > --- a/arch/x86/kvm/x86.h > +++ b/arch/x86/kvm/x86.h > @@ -368,6 +368,6 @@ static inline bool kvm_pat_valid(u64 data) > > void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu); > void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu); > -u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu); > +int kvm_spec_ctrl_test_value(u64 value); > > #endif >
| |