linux-kernel - Re: [PATCH] KVM: x86/mmu: Drop "struct kvm_mmu *mmu" from __kvm_mmu_invalidate

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] [day] [month] [year] [list]

Message-ID: <ZGeR+bRA56KgEf/2@google.com>
Date:   Fri, 19 May 2023 08:12:57 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86/mmu: Drop "struct kvm_mmu *mmu" from __kvm_mmu_invalidate_addr()

On Fri, May 19, 2023, Like Xu wrote:
> From: Like Xu <likexu@...cent.com>
> 
> Remove incoming parameter "struct kvm_mmu *mmu" that are no longer used.
> Whether the func is using "vcpu->arch.root_mmu" or "vcpu->arch.guest_mmu",
> it can be referenced as expected via "vcpu->arch.mmu". Thus the "*mmu"
> is replaced by the use of "vcpu->arch.mmu" in commit 19ace7d6ca15 ("KVM:
> x86/mmu: Skip calling mmu->sync_spte() when the spte is 0").
> 
> No functional change is intended.
> 
> Signed-off-by: Like Xu <likexu@...cent.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index c8961f45e3b1..160c40c83330 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5790,8 +5790,8 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
>  }
>  EXPORT_SYMBOL_GPL(kvm_mmu_page_fault);
>  
> -static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> -				      u64 addr, hpa_t root_hpa)
> +static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, u64 addr,
> +				      hpa_t root_hpa)
>  {
>  	struct kvm_shadow_walk_iterator iterator;

Oof, there's a mess waiting to happen here.  for_each_shadow_entry_using_root()
and kvm_sync_spte() do indeed operate on vcpu->arch.mmu, but the only reason that
doesn't cause explosions is because handle_invept() frees roots instead of doing
a manual invalidation.  At a glance, I don't see any major roadblocks to switching
INVEPT emulation over to use kvm_mmu_invalidate_addr().

I'm leaning towards asserting on @mmu instead of deleting it.

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c8961f45e3b1..258f12235874 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5797,6 +5797,14 @@ static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu
 
 	vcpu_clear_mmio_info(vcpu, addr);
 
+	/*
+	 * Walking and synchronizing SPTEs both assume they are operating in
+	 * the context of the current MMU, and would need to be reworked if
+	 * this is ever used to sync the guest_mmu, e.g. to emulate INVEPT.
+	 */
+	if (WARN_ON_ONCE(mmu != vcpu->arch.mmu))
+		return;
+
 	if (!VALID_PAGE(root_hpa))
 		return;
 

base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5
--