Re: [PATCH 18/20] KVM: arm64: Add PKVM_HYP_REQ_SPLIT

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) race in `__pkvm_pgtable_stage2_split` due to missing MMU notifier retry protocol.
- [Critical] Using `pin_user_pages()` introduces a `FOLL_PIN` counting bias that causes massive memory leaks during KVM teardown.
- [High] Memory cgroup accounting bypass when allocating spare `pkvm_mapping` structures.
- [Medium] Splitting read-only guest mappings is incorrectly forbidden by requiring write access.
- [Low] `__pkvm_pgtable_stage2_split()` mixes `goto`-based error handling with scope-based `__free(kfree)` cleanup.
--

--- Patch [18]: [PATCH 18/20] KVM: arm64: Add PKVM_HYP_REQ_SPLIT ---
commit eb20478193ba08806cd19923ba4b65163db37c40
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Add PKVM_HYP_REQ_SPLIT

This commit introduces a new pKVM hypervisor request, PKVM_HYP_REQ_SPLIT,
enabling the hypervisor to request the host to split stage-2 huge mappings.
This ensures synchronous splitting of both the guest stage-2 page tables
and the host's pkvm_mappings red-black tree, preventing concurrent
teardowns from observing inconsistent page sizes.

> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 379bdc2b258a6..089b77cf2f6a9 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c

[ ... ]

> +static int pkvm_mapping_alloc_spares(struct list_head *head, u64 nr_spares)
> +{
> +	struct pkvm_mapping *m;
> +
> +	while (nr_spares--) {
> +		m = kzalloc_obj(*m);

[Severity: High]
Does this bypass memory cgroup limits?

This allocation uses kzalloc_obj() which defaults to GFP_KERNEL without
the __GFP_ACCOUNT flag.

Because each PMD split allocates 511 structures, a guest could repeatedly
trigger splits across its memory footprint.

Can this allow the guest to consume large amounts of uncharged host memory,
potentially leading to host memory exhaustion?

> +		if (!m) {
> +			pkvm_mapping_free_spares(head);
> +			return -ENOMEM;
> +		}
> +
> +		list_add(&m->list, head);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int __pkvm_pgtable_stage2_split(struct kvm_vcpu *vcpu, phys_addr_t ipa, u64 size)
> +{
> +	struct kvm_hyp_memcache *mc = &vcpu->arch.pkvm_memcache;
> +	struct kvm_pgtable *pgt = vcpu->arch.hw_mmu->pgt;
> +	struct page **pages __free(kfree) = NULL;

[Severity: Low]
Is it safe to mix scope-based cleanup with goto error handling in this
function?

The pages array is declared with __free(kfree), but the function uses goto
labels like goto unlock_srcu and goto unlock_mmu for unwinding locks and
freeing the spares list.

The kernel cleanup guidelines note that functions should use either
scope-based cleanup for all resources or goto labels for all resources,
but not mix them, as it creates confusing ownership semantics.

> +	struct mm_struct *mm = current->mm;
> +	struct kvm_memory_slot *memslot;

[ ... ]

> +	idx = srcu_read_lock(&kvm->srcu);
> +	memslot = gfn_to_memslot(kvm, gfn);
> +	hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
> +	if (kvm_is_error_hva(hva) || !writable) {

[Severity: Medium]
Should read-only mappings be allowed to split?

This explicitly checks that the memslot is writable and fails with -EFAULT
otherwise. It also passes FOLL_WRITE to pin_user_pages() later on.

However, splitting a huge page block into smaller PTEs changes the mapping
granularity without modifying the underlying memory contents.

Does requiring write access prevent the hypervisor from splitting read-only
stage-2 block mappings, such as guest ROMs?

> +		ret = -EFAULT;
> +		goto unlock_srcu;
> +	}
> +
> +	mmap_read_lock(mm);
> +	ret = pin_user_pages(hva, nr_pages, FOLL_HWPOISON | FOLL_LONGTERM | FOLL_WRITE, pages);

[Severity: Critical]
Does using pin_user_pages() here cause a permanent memory leak during KVM
teardown?

Calling pin_user_pages() adds a GUP_PIN_COUNTING_BIAS to the page refcount.
However, normal KVM stage-2 teardown drops references using standard
put_page() semantics which only subtracts one from the refcount.

When the VM is destroyed, will these tail pages retain phantom references
and be permanently leaked?

> +	mmap_read_unlock(mm);
> +	if (ret != nr_pages) {
> +		if (ret > 0)
> +			unpin_user_pages(pages, ret);
> +		ret = -EFAULT;
> +		goto unlock_srcu;
> +	}
> +
> +	write_lock(&kvm->mmu_lock);
> +	mapping = pkvm_mapping_iter_first(&pgt->pkvm_mappings, ipa, ipa + size - 1);

[Severity: Critical]
Is there a Time-of-Check to Time-of-Use race here since the MMU
notifier retry protocol is missing?

The resolution of the HVA to PFNs via pin_user_pages() happens outside
kvm->mmu_lock. A concurrent host MMU invalidation could unmap the region
and remove the pkvm_mapping, and a subsequent guest fault could establish
a new huge page mapping at the same IPA.

When this thread acquires kvm->mmu_lock, it finds the new mapping in the
tree and proceeds to split it using the PFNs derived from the old mapping.

Without capturing kvm->mmu_invalidate_seq before GUP and checking
mmu_invalidate_retry() under the lock, can this lead to a refcount
underflow on the new huge page and a permanent leak of the old pages?

> +	if (!pkvm_mapping_can_split(mapping)) {
> +		ret = -EINVAL;
> +		goto unlock_mmu;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18