Re: [PATCH 5.15.y v3 3/7] KVM: x86/mmu: Derive shadow MMU page role from parent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Direct shadow pages erroneously inherit `gpte_is_8_bytes = 0` and are incorrectly assigned quadrants.
--

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

KVM: x86/mmu: Derive shadow MMU page role from parent

This patch modifies the shadow MMU page role calculation to derive most
information from the parent shadow page rather than computing it from scratch.
It preemptively splits the role calculation into a separate function to reduce
dependencies on the vCPU root role when allocating shadow page tables.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 9f193b1620a95..b0504d40ac8c6 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
[ ... ]
> @@ -2176,6 +2158,54 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>  	return sp;
>  }
>  
> +static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, unsigned int access)
> +{
> +	struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
> +	union kvm_mmu_page_role role;
> +
> +	role = parent_sp->role;
> +	role.level--;
> +	role.access = access;
> +	role.direct = direct;
> +
> +	/*
> +	 * If the guest has 4-byte PTEs then that means it's using 32-bit,
> +	 * 2-level, non-PAE paging. KVM shadows such guests with PAE paging
> +	 * (i.e. 8-byte PTEs). The difference in PTE size means that KVM must
> +	 * shadow each guest page table with multiple shadow page tables, which
> +	 * requires extra bookkeeping in the role.
> +	 *
> +	 * Specifically, to shadow the guest's page directory (which covers a
> +	 * 4GiB address space), KVM uses 4 PAE page directories, each mapping
> +	 * 1GiB of the address space. @role.quadrant encodes which quarter of
> +	 * the address space each maps.
> +	 *
> +	 * To shadow the guest's page tables (which each map a 4MiB region), KVM
> +	 * uses 2 PAE page tables, each mapping a 2MiB region. For these,
> +	 * @role.quadrant encodes which half of the region they map.
> +	 *
> +	 * Note, the 4 PAE page directories are pre-allocated and the quadrant
> +	 * assigned in mmu_alloc_root(). So only page tables need to be handled
> +	 * here.
> +	 */
> +	if (!role.gpte_is_8_bytes) {
> +		WARN_ON_ONCE(role.level != PG_LEVEL_4K);
> +		role.quadrant = (sptep - parent_sp->spt) % 2;
> +	}
> +
> +	return role;
> +}

[Severity: High]
Because this patch removes the earlier enforcement of gpte_is_8_bytes being
true for direct pages, will direct child pages (like NX huge page workaround
pages shadowed from a 32-bit parent) inherit gpte_is_8_bytes = 0 from the
parent here?

If so, doesn't this incorrectly assign a quadrant to these direct pages,
violating the KVM invariant that quadrants are never used for direct pages?
This broken invariant appears to cause the WARN_ON_ONCE(role.direct &&
!role.gpte_is_8_bytes) check to trigger when introduced in later patches,
which creates a potential host-side denial of service if panic_on_warn is
enabled.

[ ... ]
> @@ -3438,13 +3467,18 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn)
>  	return ret;
>  }
>  
> -static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gva,
> +static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, int quadrant,
>  			    u8 level)
>  {
> -	bool direct = vcpu->arch.mmu->mmu_role.base.direct;
> +	union kvm_mmu_page_role role = vcpu->arch.mmu->mmu_role.base;
>  	struct kvm_mmu_page *sp;
>  
> -	sp = kvm_mmu_get_page(vcpu, gfn, gva, level, direct, ACC_ALL);
> +	role.level = level;
> +
> +	if (!role.gpte_is_8_bytes)
> +		role.quadrant = quadrant;
> +
> +	sp = kvm_mmu_get_page(vcpu, gfn, role);
>  	++sp->root_count;
>  
>  	return __pa(sp->spt);

[Severity: High]
When a guest runs in non-paging mode (CR0.PG=0), mmu_role.base has
gpte_is_8_bytes set to 0. Since the code no longer forces gpte_is_8_bytes to
true for direct pages, will this block erroneously assign a quadrant to the
direct root?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913155523.7423-1-k@mgml.me?part=3