| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 26 Feb 2021 12:27:49 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Thomas Lamprecht <t.lamprecht@...xmox.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org
Cc: stable@...r.kernel.org, Jim Mattson <jmattson@...hat.com>,
Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 5.4 12/47] KVM: x86: avoid incorrect writes to host
MSR_IA32_SPEC_CTRL
On 26/02/21 12:03, Thomas Lamprecht wrote:
> On 04.01.21 16:57, Greg Kroah-Hartman wrote:
>> From: Paolo Bonzini <pbonzini@...hat.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@...hat.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@...r.kernel.org
> Suggested-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> Message-Id: <20200708115731.180097-1-mlevitsk@...hat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.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@...xmox.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
>
Powered by blists - more mailing lists