Re: [PATCH 5.15.y 3/8] 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] Missing `gpte_is_8_bytes = true` override for direct pages causes KVM invariant violations, memory bloat, and guest-triggerable host panics (DoS).
--

commit 9e9bc7152f4bea9d4c30ffc781d2f28f82a2c951
Author: Paolo Bonzini <pbonzini@redhat.com>
Subject: KVM: x86/mmu: Derive shadow MMU page role from parent

This commit refactors how the shadow MMU page role is computed by
deriving it from the parent shadow page. This removes the reliance 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
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2070,33 +2070,15 @@ static void clear_sp_write_flooding_count(u64 *spte)
[ ... ]
> -	role.direct = direct;
> -	if (role.direct)
> -		role.gpte_is_8_bytes = true;

[ ... ]

> @@ -2171,6 +2153,54 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> +static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, unsigned int access)
> +{
[ ... ]
> +	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]
Does this missing gpte_is_8_bytes override cause issues for direct maps
when a guest has paging disabled?

Since we no longer set gpte_is_8_bytes = true when role.direct is true, 
if a guest runs with CR0.PG=0, mmu_role.base.gpte_is_8_bytes will be false.

In kvm_mmu_child_role(), child direct pages inherit this false value
from their parent. When building non-leaf shadow pages for direct maps,
this will enter the if (!role.gpte_is_8_bytes) block.

Won't this trigger the WARN_ON_ONCE(role.level != PG_LEVEL_4K) assertion
for non-leaf direct maps, and also calculate a bogus quadrant? This
could fragment the cache keys and cause duplicate shadow page allocations
for the same GFN.

[ ... ]

> @@ -3408,13 +3437,18 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn)
> -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;
[ ... ]
> +	if (!role.gpte_is_8_bytes)
> +		role.quadrant = quadrant;
> +
> +	sp = kvm_mmu_get_page(vcpu, gfn, role);

[Severity: High]
Could the lack of the direct override here cause the root page to have
invalid role properties?

When mmu_alloc_root() is called by mmu_alloc_direct_roots() for a guest
with paging disabled, the role is extracted from mmu_role.base. If we
don't override gpte_is_8_bytes to true for direct pages, the root page
will have gpte_is_8_bytes = false. 

Because of this, sequentially all children will inherit these invalid
properties, leading to the problems mentioned above in
kvm_mmu_child_role(). Should we ensure gpte_is_8_bytes is set correctly
for direct pages during root allocation?

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