linux-kernel - Re: [PATCH 22/22] KVM: x86/mmu: Detect if unprotect will do anything based on invalid

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: <Zr4P86YRZvefE95k@google.com>
Date: Thu, 15 Aug 2024 07:25:55 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Peter Gonda <pgonda@...gle.com>, Michael Roth <michael.roth@....com>, 
	Vishal Annapurve <vannapurve@...gle.com>, Ackerly Tng <ackerleytng@...gle.com>
Subject: Re: [PATCH 22/22] KVM: x86/mmu: Detect if unprotect will do anything
 based on invalid_list

On Wed, Aug 14, 2024, Paolo Bonzini wrote:
> On 8/9/24 21:03, Sean Christopherson wrote:
> > Explicitly query the list of to-be-zapped shadow pages when checking to
> > see if unprotecting a gfn for retry has succeeded, i.e. if KVM should
> > retry the faulting instruction.
> > 
> > Add a comment to explain why the list needs to be checked before zapping,
> > which is the primary motivation for this change.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > ---
> >   arch/x86/kvm/mmu/mmu.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 300a47801685..50695eb2ee22 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -2731,12 +2731,15 @@ bool __kvm_mmu_unprotect_gfn_and_retry(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> >   			goto out;
> >   	}
> > -	r = false;
> >   	write_lock(&kvm->mmu_lock);
> > -	for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) {
> > -		r = true;
> > +	for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa))
> >   		kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
> > -	}
> > +
> > +	/*
> > +	 * Snapshot the result before zapping, as zapping will remove all list
> > +	 * entries, i.e. checking the list later would yield a false negative.
> > +	 */
> 
> Hmm, the comment is kinda overkill?  Maybe just
> 
> 	/* Return whether there were sptes to zap.  */
> 	r = !list_empty(&invalid_test);

I would strongly prefer to keep the verbose comment.  I was "this" close to
removing the local variable and checking list_empty() after the commit phase.
If we made that goof, it would only show up at the worst time, i.e. when a guest
triggers retry and gets stuck.  And the logical outcome of fixing such a bug
would be to add a comment to prevent it from happening again, so I say just add
the comment straightaway.

> I'm not sure about patch 21 - I like the simple kvm_mmu_unprotect_page()
> function.

>From a code perspective, I kinda like having a separate helper too.  As you
likely suspect given your below suggestion, KVM should never unprotect a gfn
without retry protection, i.e. there should never be another caller, and I want
to enforce that.

> Maybe rename it to kvm_mmu_zap_gfn() and make it static in the same patch?

kvm_mmu_zap_gfn() would be quite misleading.  Unlike kvm_zap_gfn_range(), it only
zaps non-leaf shadow pages.  E.g. the name would suggest that it could be used by
__kvm_set_or_clear_apicv_inhibit(), but it would do the complete wrong thing.

kvm_mmu_zap_shadow_pages() is the least awful I can come up with (it needs to be
plural because it zaps all SPs related to the gfn), but that's something confusing
too since it would take in a single gfn.  So I think my vote is to keep patch 21
and dodge the naming entirely.