Re: [PATCH v4 3/4] KVM: x86/mmu: Move slot checks from __kvm_faultin_pfn() to kvm_faultin_pfn()

[Yan Zhao]

On Mon, Feb 19, 2024 at 11:44:54AM -0800, Sean Christopherson wrote:
> +Jim
> 
> On Mon, Feb 19, 2024, Yan Zhao wrote:
> > On Fri, Feb 09, 2024 at 02:28:57PM -0800, Sean Christopherson wrote:
> > > @@ -4406,6 +4379,37 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
> > >  	fault->mmu_seq = vcpu->kvm->mmu_invalidate_seq;
> > >  	smp_rmb();
> > >  
> > > +	if (!slot)
> > > +		goto faultin_pfn;
> > > +
> > > +	/*
> > > +	 * Retry the page fault if the gfn hit a memslot that is being deleted
> > > +	 * or moved.  This ensures any existing SPTEs for the old memslot will
> > > +	 * be zapped before KVM inserts a new MMIO SPTE for the gfn.
> > > +	 */
> > > +	if (slot->flags & KVM_MEMSLOT_INVALID)
> > > +		return RET_PF_RETRY;
> > > +
> > > +	if (!kvm_is_visible_memslot(slot)) {
> > > +		/* Don't expose KVM's internal memslots to L2. */
> > > +		if (is_guest_mode(vcpu)) {
> > > +			fault->slot = NULL;
> > > +			fault->pfn = KVM_PFN_NOSLOT;
> > > +			fault->map_writable = false;
> > > +			return RET_PF_CONTINUE;
> > Call kvm_handle_noslot_fault() to replace returning RET_PF_CONTINUE?
> 
> Oof.  Yes.  But there is a pre-existing bug here too, though it's very theoretical
> and unlikely to ever cause problems.
> 
> If KVM is using TDP, but L1 is using shadow paging for L2, then routing through
> kvm_handle_noslot_fault() will incorrectly cache the gfn as MMIO, and create an
> MMIO SPTE.  Creating an MMIO SPTE is ok, but only because kvm_mmu_page_role.guest_mode
> ensure KVM uses different roots for L1 vs. L2.  But mmio_gfn will remain valid,
> and could (quite theoretically) cause KVM to incorrectly treat an L1 access to
> the private TSS or identity mapped page tables as MMIO.
Why would KVM treat L1 access to the private TSS and identity mapped page
tables as MMIO even though mmio_gfn is valid?
It looks that (for Intel platform) EPT for L1 will only install normal SPTEs
(non-MMIO SPTEs) for the two private slots, so there would not have EPT
misconfiguration and would not go to emulation path incorrectly.
Am I missing something?

> Furthermore, this check doesn't actually prevent exposing KVM's internal memslots
> to L2, it simply forces KVM to emulate the access.  In most cases, that will trigger
> MMIO, amusingly due to filling arch.mmio_gfn.  And vcpu_is_mmio_gpa() always
> treats APIC accesses as MMIO, so those are fine.  But the private TSS and identity
> mapped page tables could go either way (MMIO or access the private memslot's backing
> memory).
Yes, this is also my question when reading that part of code.
mmio_gen mismatching may lead to accessing the backing memory directly.

> We could "fix" the issue by forcing MMIO emulation for L2 access to all internal
> memslots, not just to the APIC.  But I think that's actually less correct than
> letting L2 access the private TSS and indentity mapped page tables (not to mention
> that I can't imagine anyone cares what KVM does in this case).  From L1's perspective,
> there is R/W memory at those memslot, the memory just happens to be initialized
> with non-zero data, and I don't see a good argument for hiding that memory from L2.
> Making the memory disappear is far more magical than the memory existing in the
> first place.
> 
> The APIC access page is special because KVM _must_ emulate the access to do the
> right thing.  And despite what commit 3a2936dedd20 ("kvm: mmu: Don't expose private
> memslots to L2") said, it's not just when L1 is accelerating L2's virtual APIC,
> it's just as important (likely *more* imporant* for correctness when L1 is passing
> through its own APIC to L2.
>
> Unless I'm missing someting, I think it makes sense to throw in the below before
> moving code around.
> 
> Ouch, and looking at this further, patch 1 introduced a bug (technically) by caching
> fault->slot; in this case KVM will unnecessarily check mmu_notifiers.  That's
> obviously a very benign bug, as a false positive just means an unnecessary retry,
> but yikes.
>
Patch 3 & 4 removed the bug immediately :)

> --
> Subject: [PATCH] KVM: x86/mmu: Don't force emulation of L2 accesses to
>  non-APIC internal slots
> 
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/mmu/mmu.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 488f522f09c6..4ce824cec5b9 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4341,8 +4341,18 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  	if (slot && (slot->flags & KVM_MEMSLOT_INVALID))
>  		return RET_PF_RETRY;
>  
> -	if (!kvm_is_visible_memslot(slot)) {
> -		/* Don't expose private memslots to L2. */
> +	if (slot && slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT) {
> +		/*
> +		 * Don't map L1's APIC access page into L2, KVM doesn't support
> +		 * using APICv/AVIC to accelerate L2 accesses to L1's APIC,
> +		 * i.e. the access needs to be emulated.  Emulating access to
> +		 * L1's APIC is also correct if L1 is accelerating L2's own
> +		 * virtual APIC, but for some reason L1 also maps _L1's_ APIC
> +		 * into L2.  Note, vcpu_is_mmio_gpa() always treats access to
> +		 * the APIC as MMIO.  Allow an MMIO SPTE to be created, as KVM
> +		 * uses different roots for L1 vs. L2, i.e. there is no danger
> +		 * of breaking APICv/AVIC for L1.
> +		 */
>  		if (is_guest_mode(vcpu)) {
>  			fault->slot = NULL;
>  			fault->pfn = KVM_PFN_NOSLOT;
Checking fault->is_private before calling kvm_handle_noslot_fault()?
And do we need a centralized check of fault->is_private in kvm_mmu_do_page_fault()
before returning RET_PF_EMULATE?

> @@ -4355,8 +4365,7 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  		 * MMIO SPTE.  That way the cache doesn't need to be purged
>  		 * when the AVIC is re-enabled.
>  		 */
> -		if (slot && slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT &&
> -		    !kvm_apicv_activated(vcpu->kvm))
> +		if (!kvm_apicv_activated(vcpu->kvm))
>  			return RET_PF_EMULATE;
Otherwise, here also needs a checking of fault->is_private?
Maybe also for where RET_PF_EMULATE is returned when page_fault_handle_page_track()
is true (though I know it's always false for TDX).

>  	}
>  
> 
> base-commit: ec98c2c1a07fb341ba2230eab9a31065d12d9de6
> --