linux-kernel - Re: [PATCH 3/4] KVM: x86: SVM: use vmcb01 in avic_init

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Mar 2022 16:21:06 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     kvm@...r.kernel.org, Jim Mattson <jmattson@...gle.com>,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Borislav Petkov <bp@...en8.de>, x86@...nel.org
Subject: Re: [PATCH 3/4] KVM: x86: SVM: use vmcb01 in avic_init_vmcb

Just "KVM: SVM:" for the shortlog, please.

On Tue, Mar 01, 2022, Maxim Levitsky wrote:
> Out of precation use vmcb01 when enabling host AVIC.
> No functional change intended.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> ---
>  arch/x86/kvm/svm/avic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index e23159f3a62ba..9656e192c646b 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -167,7 +167,7 @@ int avic_vm_init(struct kvm *kvm)
>  
>  void avic_init_vmcb(struct vcpu_svm *svm)
>  {
> -	struct vmcb *vmcb = svm->vmcb;
> +	struct vmcb *vmcb = svm->vmcb01.ptr;

I don't like this change.  It's not bad code, but it'll be confusing because it
implies that it's legal for svm->vmcb to be something other than svm->vmcb01.ptr
when this is called.

If we want to guard AVIC, I'd much rather we do something like:

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7038c76fa841..dcc856bd628d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -992,8 +992,12 @@ static inline void init_vmcb_after_set_cpuid(struct kvm_vcpu *vcpu)
 static void init_vmcb(struct kvm_vcpu *vcpu)
 {
        struct vcpu_svm *svm = to_svm(vcpu);
-       struct vmcb_control_area *control = &svm->vmcb->control;
-       struct vmcb_save_area *save = &svm->vmcb->save;
+       struct vmcb *vmcb = svm->vmcb01.ptr;
+       struct vmcb_control_area *control = &vmcb->control;
+       struct vmcb_save_area *save = &vmcb->save;
+
+       if (WARN_ON_ONCE(vmcb != svm->vmcb))
+               svm_leave_nested(vcpu);

        svm_set_intercept(svm, INTERCEPT_CR0_READ);
        svm_set_intercept(svm, INTERCEPT_CR3_READ);


On a related topic, init_vmcb_after_set_cpuid() is broken for nested, it needs to
play nice with being called when svm->vmcb == &svm->nested.vmcb02, e.g. update
vmcb01 and re-merge (or just recalc?) vmcb02's intercepts.

>  	struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm);
>  	phys_addr_t bpa = __sme_set(page_to_phys(svm->avic_backing_page));
>  	phys_addr_t lpa = __sme_set(page_to_phys(kvm_svm->avic_logical_id_table_page));
> -- 
> 2.26.3
>