linux-kernel - Re: [PATCH] KVM: nSVM: Enter guest mode before initializing nested NPT MMU

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: <Z5rhH342Jghl2tgL@google.com>
Date: Thu, 30 Jan 2025 02:17:03 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: nSVM: Enter guest mode before initializing nested
 NPT MMU

On Wed, Jan 29, 2025 at 05:08:25PM -0800, Sean Christopherson wrote:
> When preparing vmcb02 for nested VMRUN (or state restore), "enter" guest
> mode prior to initializing the MMU for nested NPT so that guest_mode is
> set in the MMU's role.  KVM's model is that all L2 MMUs are tagged with
> guest_mode, as the behavior of hypervisor MMUs tends to be significantly
> different than kernel MMUs.
> 
> Practically speaking, the bug is relatively benign, as KVM only directly
> queries role.guest_mode in kvm_mmu_free_guest_mode_roots(), which SVM
> doesn't use, and in paths that are optimizations (mmu_page_zap_pte() and
> shadow_mmu_try_split_huge_pages()).

Just curious, what about kvm_mmu_page_ad_need_write_protect()? Is it
also bengin?

> 
> And while the role is incorprated into shadow page usage, because nested
> NPT requires KVM to be using NPT for L1, reusing shadow pages across L1
> and L2 is impossible as L1 MMUs will always have direct=1, while L2 MMUs
> will have direct=0.
> 
> Hoist the TLB processing and setting of HF_GUEST_MASK to the beginning
> of the flow instead of forcing guest_mode in the MMU, as nothing in
> nested_vmcb02_prepare_control() between the old and new locations touches
> TLB flush requests or HF_GUEST_MASK, i.e. there's no reason to present
> inconsistent vCPU state to the MMU.
> 
> Fixes: 69cb877487de ("KVM: nSVM: move MMU setup to nested_prepare_vmcb_control")
> Cc: stable@...r.kernel.org
> Reported-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>

FWIW:

Reviewed-by: Yosry Ahmed <yosry.ahmed@...ux.dev>

> ---
>  arch/x86/kvm/mmu/mmu.c    |  2 +-
>  arch/x86/kvm/svm/nested.c | 10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 74fa38ebddbf..3ff55a18347d 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5524,7 +5524,7 @@ void kvm_init_shadow_npt_mmu(struct kvm_vcpu *vcpu, unsigned long cr0,
>  	union kvm_mmu_page_role root_role;
>  
>  	/* NPT requires CR0.PG=1. */
> -	WARN_ON_ONCE(cpu_role.base.direct);
> +	WARN_ON_ONCE(cpu_role.base.direct || !cpu_role.base.guest_mode);
>  
>  	root_role = cpu_role.base;
>  	root_role.level = kvm_mmu_get_tdp_level(vcpu);
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index d77b094d9a4d..04c375bf1ac2 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -646,6 +646,11 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  	u32 pause_count12;
>  	u32 pause_thresh12;
>  
> +	nested_svm_transition_tlb_flush(vcpu);
> +
> +	/* Enter Guest-Mode */
> +	enter_guest_mode(vcpu);
> +
>  	/*
>  	 * Filled at exit: exit_code, exit_code_hi, exit_info_1, exit_info_2,
>  	 * exit_int_info, exit_int_info_err, next_rip, insn_len, insn_bytes.
> @@ -762,11 +767,6 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  		}
>  	}
>  
> -	nested_svm_transition_tlb_flush(vcpu);
> -
> -	/* Enter Guest-Mode */
> -	enter_guest_mode(vcpu);
> -
>  	/*
>  	 * Merge guest and host intercepts - must be called with vcpu in
>  	 * guest-mode to take effect.
> 
> base-commit: eb723766b1030a23c38adf2348b7c3d1409d11f0
> -- 
> 2.48.1.262.g85cc9f2d1e-goog
>