linux-kernel - Re: [PATCH 06/30] KVM: SVM: always update CR3 in VMCB

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]

Message-ID: <07139fc2-39bd-5bc5-ef23-a98681013665@oracle.com>
Date:   Fri, 29 May 2020 10:41:58 -0700
From:   Krish Sadhukhan <krish.sadhukhan@...cle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH 06/30] KVM: SVM: always update CR3 in VMCB


On 5/29/20 8:39 AM, Paolo Bonzini wrote:
> svm_load_mmu_pgd is delaying the write of GUEST_CR3 to prepare_vmcs02

Did you mean to say enter_svm_guest_mode here ?
> as
> an optimization, but this is only correct before the nested vmentry.
> If userspace is modifying CR3 with KVM_SET_SREGS after the VM has
> already been put in guest mode, the value of CR3 will not be updated.
> Remove the optimization, which almost never triggers anyway.
> This was was added in commit 689f3bf21628 ("KVM: x86: unify callbacks
> to load paging root", 2020-03-16) just to keep the two vendor-specific
> modules closer, but we'll fix VMX too.
>
> Fixes: 689f3bf21628 ("KVM: x86: unify callbacks to load paging root")
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>   arch/x86/kvm/svm/nested.c |  6 +-----
>   arch/x86/kvm/svm/svm.c    | 16 +++++-----------
>   2 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index dcac4c3510ab..8756c9f463fd 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -256,11 +256,7 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
>   	svm_set_efer(&svm->vcpu, nested_vmcb->save.efer);
>   	svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0);
>   	svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4);
> -	if (npt_enabled) {
> -		svm->vmcb->save.cr3 = nested_vmcb->save.cr3;
> -		svm->vcpu.arch.cr3 = nested_vmcb->save.cr3;
> -	} else
> -		(void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
> +	(void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
>   
>   	/* Guest paging mode is active - reset mmu */
>   	kvm_mmu_reset_context(&svm->vcpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 545f63ebc720..feb96a410f2d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3447,7 +3447,6 @@ static fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>   static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long root)
>   {
>   	struct vcpu_svm *svm = to_svm(vcpu);
> -	bool update_guest_cr3 = true;
>   	unsigned long cr3;
>   
>   	cr3 = __sme_set(root);
> @@ -3456,18 +3455,13 @@ static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long root)
>   		mark_dirty(svm->vmcb, VMCB_NPT);
>   
>   		/* Loading L2's CR3 is handled by enter_svm_guest_mode.  */
> -		if (is_guest_mode(vcpu))
> -			update_guest_cr3 = false;
> -		else if (test_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail))
> -			cr3 = vcpu->arch.cr3;
> -		else /* CR3 is already up-to-date.  */
> -			update_guest_cr3 = false;
> +		if (!test_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail))
> +			return;
> +		cr3 = vcpu->arch.cr3;
>   	}
>   
> -	if (update_guest_cr3) {
> -		svm->vmcb->save.cr3 = cr3;
> -		mark_dirty(svm->vmcb, VMCB_CR);
> -	}
> +	svm->vmcb->save.cr3 = cr3;
> +	mark_dirty(svm->vmcb, VMCB_CR);
>   }
>   
>   static int is_disabled(void)