| 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
| ||
|
Message-ID: <87tty6o9k9.fsf@redhat.com>
Date: Mon, 27 Mar 2023 10:57:58 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: jpiotrowski@...ux.microsoft.com, linux-kernel@...r.kernel.org
Cc: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>,
Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
Tianyu Lan <ltykernel@...il.com>,
Michael Kelley <mikelley@...rosoft.com>,
Sean Christopherson <seanjc@...gle.com>, stable@...r.kernel.org
Subject: Re: [RESEND PATCH v2] KVM: SVM: Flush Hyper-V TLB when required
jpiotrowski@...ux.microsoft.com writes:
> From: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
>
> The Hyper-V "EnlightenedNptTlb" enlightenment is always enabled when KVM
> is running on top of Hyper-V and Hyper-V exposes support for it (which
> is always). On AMD CPUs this enlightenment results in ASID invalidations
> not flushing TLB entries derived from the NPT. To force the underlying
> (L0) hypervisor to rebuild its shadow page tables, an explicit hypercall
> is needed.
>
> The original KVM implementation of Hyper-V's "EnlightenedNptTlb" on SVM
> only added remote TLB flush hooks. This worked out fine for a while, as
> sufficient remote TLB flushes where being issued in KVM to mask the
> problem. Since v5.17, changes in the TDP code reduced the number of
> flushes and the out-of-sync TLB prevents guests from booting
> successfully.
>
> Split svm_flush_tlb_current() into separate callbacks for the 3 cases
> (guest/all/current), and issue the required Hyper-V hypercall when a
> Hyper-V TLB flush is needed. The most important case where the TLB flush
> was missing is when loading a new PGD, which is followed by what is now
> svm_flush_tlb_current().
>
> Cc: stable@...r.kernel.org # v5.17+
> Fixes: 1e0c7d40758b ("KVM: SVM: hyper-v: Remote TLB flush for SVM")
> Link: https://lore.kernel.org/lkml/43980946-7bbf-dcef-7e40-af904c456250@linux.microsoft.com/
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
> ---
> Resending because I accidentally used the wrong "From:" address and it bounced
> from some recipients.
>
> Changes since v1:
> - lookup enlightened_npt_tlb in vmcb to determine whether to do the
> flush
> - when KVM wants a hyperv_flush_guest_mapping() call, don't try to
> optimize it out
> - don't hide hyperv flush behind helper, make it visible in
> svm.c
>
> arch/x86/kvm/kvm_onhyperv.h | 5 +++++
> arch/x86/kvm/svm/svm.c | 37 ++++++++++++++++++++++++++++++---
> arch/x86/kvm/svm/svm_onhyperv.h | 15 +++++++++++++
> 3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/kvm_onhyperv.h b/arch/x86/kvm/kvm_onhyperv.h
> index 287e98ef9df3..67b53057e41c 100644
> --- a/arch/x86/kvm/kvm_onhyperv.h
> +++ b/arch/x86/kvm/kvm_onhyperv.h
> @@ -12,6 +12,11 @@ int hv_remote_flush_tlb_with_range(struct kvm *kvm,
> int hv_remote_flush_tlb(struct kvm *kvm);
> void hv_track_root_tdp(struct kvm_vcpu *vcpu, hpa_t root_tdp);
> #else /* !CONFIG_HYPERV */
> +static inline int hv_remote_flush_tlb(struct kvm *kvm)
> +{
> + return -1;
> +}
Nitpick: -ENOTSUPP?
> +
> static inline void hv_track_root_tdp(struct kvm_vcpu *vcpu, hpa_t root_tdp)
> {
> }
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 252e7f37e4e2..f25bc3cbb250 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3729,7 +3729,7 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> }
>
> -static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
> +static void svm_flush_tlb_asid(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
>
> @@ -3753,6 +3753,37 @@ static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
> svm->current_vmcb->asid_generation--;
> }
>
> +static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
> +{
> + hpa_t root_tdp = vcpu->arch.mmu->root.hpa;
> +
> + /*
> + * When running on Hyper-V with EnlightenedNptTlb enabled, explicitly
> + * flush the NPT mappings via hypercall as flushing the ASID only
> + * affects virtual to physical mappings, it does not invalidate guest
> + * physical to host physical mappings.
> + */
> + if (svm_hv_is_enlightened_tlb_enabled(vcpu) && VALID_PAGE(root_tdp))
> + hyperv_flush_guest_mapping(root_tdp);
Nitpick: it could also make sense to yell with a WARN_ON_ONCE() or
something if hyperv_flush_guest_mapping() ever fails.
> +
> + svm_flush_tlb_asid(vcpu);
> +}
> +
> +static void svm_flush_tlb_all(struct kvm_vcpu *vcpu)
> +{
> + /*
> + * When running on Hyper-V with EnlightenedNptTlb enabled, remote TLB
> + * flushes should be routed to hv_remote_flush_tlb() without requesting
> + * a "regular" remote flush. Reaching this point means either there's
> + * a KVM bug or a prior hv_remote_flush_tlb() call failed, both of
> + * which might be fatal to the guest. Yell, but try to recover.
> + */
> + if (WARN_ON_ONCE(svm_hv_is_enlightened_tlb_enabled(vcpu)))
> + hv_remote_flush_tlb(vcpu->kvm);
> +
> + svm_flush_tlb_asid(vcpu);
> +}
> +
> static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4745,10 +4776,10 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
> .set_rflags = svm_set_rflags,
> .get_if_flag = svm_get_if_flag,
>
> - .flush_tlb_all = svm_flush_tlb_current,
> + .flush_tlb_all = svm_flush_tlb_all,
> .flush_tlb_current = svm_flush_tlb_current,
> .flush_tlb_gva = svm_flush_tlb_gva,
> - .flush_tlb_guest = svm_flush_tlb_current,
> + .flush_tlb_guest = svm_flush_tlb_asid,
>
> .vcpu_pre_run = svm_vcpu_pre_run,
> .vcpu_run = svm_vcpu_run,
> diff --git a/arch/x86/kvm/svm/svm_onhyperv.h b/arch/x86/kvm/svm/svm_onhyperv.h
> index cff838f15db5..786d46d73a8e 100644
> --- a/arch/x86/kvm/svm/svm_onhyperv.h
> +++ b/arch/x86/kvm/svm/svm_onhyperv.h
> @@ -6,6 +6,8 @@
> #ifndef __ARCH_X86_KVM_SVM_ONHYPERV_H__
> #define __ARCH_X86_KVM_SVM_ONHYPERV_H__
>
> +#include <asm/mshyperv.h>
> +
> #if IS_ENABLED(CONFIG_HYPERV)
>
> #include "kvm_onhyperv.h"
> @@ -15,6 +17,14 @@ static struct kvm_x86_ops svm_x86_ops;
>
> int svm_hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu);
>
> +static inline bool svm_hv_is_enlightened_tlb_enabled(struct kvm_vcpu *vcpu)
> +{
> + struct hv_vmcb_enlightenments *hve = &to_svm(vcpu)->vmcb->control.hv_enlightenments;
> +
> + return ms_hyperv.nested_features & HV_X64_NESTED_ENLIGHTENED_TLB &&
> + !!hve->hv_enlightenments_control.enlightened_npt_tlb;
> +}
> +
> static inline void svm_hv_init_vmcb(struct vmcb *vmcb)
> {
> struct hv_vmcb_enlightenments *hve = &vmcb->control.hv_enlightenments;
> @@ -80,6 +90,11 @@ static inline void svm_hv_update_vp_id(struct vmcb *vmcb, struct kvm_vcpu *vcpu)
> }
> #else
>
> +static inline bool svm_hv_is_enlightened_tlb_enabled(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> static inline void svm_hv_init_vmcb(struct vmcb *vmcb)
> {
> }
Apart from the two nitpicks above,
Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
--
Vitaly
Powered by blists - more mailing lists