Re: [PATCH 5.10.y 17/17] KVM: x86/mmu: Ensure hugepage is in by slot before checking max mapping level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch backports a fix for an out-of-bounds array access to a kernel version where the buggy function does not exist, introducing dead logic.

Pre-existing issues:
- [Critical] KVM flushes the wrong GFN when zapping SPTEs, leaving stale EPT/TLB entries for the actual guest memory.
--

commit 1fc28b2436d7c174304f3fc391cd3180566514b7
Author: Sean Christopherson <seanjc@google.com>

KVM: x86/mmu: Ensure hugepage is in by slot before checking max mapping level

This patch adds a bounds check to verify that a shadow page's base GFN is
within the target memslot when checking for collapsible SPTEs. This is intended
to prevent out-of-bounds accesses to the slot's lpage_info during hugepage
recovery.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 39186d6952696..b7e8618d3df52 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5659,13 +5659,20 @@ static bool kvm_mmu_zap_collapsible_spte(struct kvm *kvm,
>  		pfn = spte_to_pfn(*sptep);
>  
>  		/*
> -		 * We cannot do huge page mapping for indirect shadow pages,
[ ... ]
> +		 * Direct shadow page can be replaced by a hugepage if the host
> +		 * mapping level allows it and the memslot maps all of the host
> +		 * hugepage.  Note!  If the memslot maps only part of the
> +		 * hugepage, sp->gfn may be below slot->base_gfn, and querying
> +		 * the max mapping level would cause an out-of-bounds lpage_info
> +		 * access.  So the gfn bounds check *must* be done first.
> +		 *
> +		 * Indirect shadow pages are created when the guest page tables
> +		 * are using 4K pages.  Since the host mapping is always
> +		 * constrained by the page size in the guest, indirect shadow
> +		 * pages are never collapsible.
>  		 */
> -		if (sp->role.direct && !kvm_is_reserved_pfn(pfn) &&
> +		if (sp->role.direct && is_gfn_in_memslot(slot, sp->gfn) &&
> +		    !kvm_is_reserved_pfn(pfn) &&
> +		    (kvm_is_zone_device_pfn(pfn) ||
> +		     PageCompound(pfn_to_page(pfn)))) {
>  			pte_list_remove(rmap_head, sptep);

[Severity: Medium]
The commit message mentions that querying the max mapping level causes an
out-of-bounds lpage_info access, but looking at kvm_mmu_zap_collapsible_spte()
in this kernel version, neither kvm_mmu_max_mapping_level() nor lpage_info are
accessed here.

Does this added check just prevent KVM from collapsing 4KB pages into a 2MB
hugepage if the 2MB region crosses the memslot boundary, without actually fixing
an out-of-bounds access in this code path?

[Severity: Critical]
This is a pre-existing issue, but does this code flush the correct GFN?

In kvm_mmu_zap_collapsible_spte(), just below this change, the flush uses
sp->gfn:

			if (kvm_available_flush_tlb_with_range())
				kvm_flush_remote_tlbs_with_address(kvm, sp->gfn,
					KVM_PAGES_PER_HPAGE(sp->role.level));

sp->gfn is the base GFN of the shadow page, not the actual data page GFN mapped
by the zapped SPTE. If KVM_PAGES_PER_HPAGE(sp->role.level) is 1 (for a level-1
shadow page), wouldn't this flush only the base GFN and leave stale TLB entries
for the actual guest memory?

(Similar pre-existing issues appear to exist in rmap_recycle() and
FNAME(sync_page)() which also flush sp->gfn instead of the actual data page GFN.)

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