Re: [PATCH 18/24] KVM: x86/mmu: make gva_walk a value

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Decoupling `gva_walk` into an embedded struct breaks pointer equality checks, classifying L1 shadow page faults as nested and completely bypassing TLB flushes and shadow page invalidations.
--

commit e0579a768a48ab33f8084b00e1f9e241b8879e43
Author: Paolo Bonzini <pbonzini@redhat.com>

    KVM: x86/mmu: make gva_walk a value

This commit changes gva_walk from a pointer to an embedded struct in
kvm_vcpu_arch. It aims to use a single instance of kvm_pagewalk for GVA
to GPA translations instead of flipping the gva_walk pointer back and
forth.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 386fdbc34b025..2fe4d53590064 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c

[ ... ]

> @@ -6598,17 +6588,22 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_pagewalk *w,
>  	WARN_ON_ONCE(roots & ~KVM_MMU_ROOTS_ALL);
>  
>  	/* It's actually a GPA for vcpu->arch.guest_mmu.  */
> -	if (w != &vcpu->arch.guest_mmu.w) {
> +	if (w == &vcpu->arch.gva_walk) {
>  		/* INVLPG on a non-canonical address is a NOP according to the SDM.  */
>  		if (is_noncanonical_invlpg_address(addr, vcpu))
>  			return;
>  
>  		kvm_x86_call(flush_tlb_gva)(vcpu, addr);
> -		if (w == &vcpu->arch.ngva_walk)
> +
> +		if (tdp_enabled)
>  			return;
> +
> +		mmu = &vcpu->arch.root_mmu;
> +	} else {
> +		mmu = &vcpu->arch.guest_mmu;
>  	}

[Severity: High]
If kvm_mmu_invalidate_addr() is called with w pointing to
&vcpu->arch.root_mmu.w, this pointer check will fail and fall through to the
else block, assigning mmu = &vcpu->arch.guest_mmu.

Since guest_mmu is inactive when not nested, mmu->sync_spte would be NULL,
causing an early return. Does this silently skip both the hardware TLB flush
and the root_mmu shadow page invalidation, potentially creating an infinite
page fault loop?

I noticed this behavior can be triggered due to the changes in paging_tmpl.h
and x86.c, which I've commented on below. I also see this is fixed later in
the series by commit 9a1a629159ef9003 ("KVM: x86/mmu: pull struct
kvm_pagewalk out of struct kvm_mmu"), but it seems this intermediate commit
breaks bisection.

> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index 6b21778e83404..b12c6b5e4a2fb 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h

[ ... ]

> @@ -541,7 +541,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
>  	}
>  #endif
>  	walker->fault.address = addr;
> -	walker->fault.nested_page_fault = w != vcpu->arch.gva_walk;
> +	walker->fault.nested_page_fault = w != &vcpu->arch.gva_walk;

[Severity: High]
Does this break the pointer equality check for shadow paging?

When w points to &vcpu->arch.root_mmu.w (for L1 shadow page faults),
w != &vcpu->arch.gva_walk will now evaluate to true since they are distinct
struct instances in memory. Would this incorrectly flag L1 shadow page faults
as nested?

>  	walker->fault.async_page_fault = false;

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 147cef7b23b60..14af0f4d010ed 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -995,7 +995,7 @@ void kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu,
>  	WARN_ON_ONCE(fault->vector != PF_VECTOR);
>  
>  	fault_walk = fault->nested_page_fault ? &vcpu->arch.mmu->w :
> -						vcpu->arch.gva_walk;
> +						&vcpu->arch.gva_walk;

[Severity: High]
If nested_page_fault is incorrectly set to true by walk_addr_generic(),
doesn't this select &vcpu->arch.mmu->w instead of &vcpu->arch.gva_walk?

This results in kvm_mmu_invalidate_addr() being called with root_mmu.w,
leading to the skipped invalidation mentioned above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603105814.10236-1-pbonzini@redhat.com?part=18