Re: [PATCH 20/20] KVM: arm64: Stage-2 huge mappings for protected VMs

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] A TOCTOU race and insufficient `folio_test_large()` validation in `pkvm_mem_abort()` allows malicious host userspace to trigger a DoS or map up to 2MB of arbitrary unowned physical memory into the guest.
- [High] Missing verification of pinned PFNs in `__pkvm_pgtable_stage2_split()` causes a THP refcount underflow (kernel panic) and a memory leak when raced with host userspace unmapping.
- [High] Unconditional suppression of `-EAGAIN` in `pkvm_mem_abort()` causes a deterministic livelock (infinite fault loop) when a PMD-sized mapping overlaps with a pre-existing 4K mapping.
- [High] Missing validation of the stage-2 block granule against the guest's requested fault size in `pkvm_mem_abort()` silently breaks pKVM isolation.
--

--- Patch [20]: [PATCH 20/20] KVM: arm64: Stage-2 huge mappings for protected VMs ---
commit 66c372ae561bfc5bac1a23e79526b597c8fa02fd
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Stage-2 huge mappings for protected VMs

Enable PMD-sized stage-2 block mappings for protected VMs. This is
possible whenever the stage-1 mapping allows it, that is if it itself
backed by THPs.

When a THP is found, an entire PMD_SIZE mapping is donated to the guest.
This mapping can only be broken down via the HVC
__pkvm_host_split_guest() which the hypervisor can request with
PKVM_HYP_REQ_SPLIT.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6c941aaa10c63..46820240d2337 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -1755,25 +1737,59 @@ static int pkvm_mem_abort(const struct kvm_s2_fault_desc *s2fd)
>  		goto unpin;
>  	}
>  
> +	pfn = page_to_pfn(page);
> +	gfn = gpa_to_gfn(s2fd->fault_ipa);
> +
> +	page_size = transparent_hugepage_adjust(kvm, s2fd->memslot, s2fd->hva, &pfn, &gfn);

[Severity: High]
Does this skip validating the stage-2 block granule against the guest's
requested fault size?

Deriving the block size solely from the host's THP state via
transparent_hugepage_adjust() without validating it against the fault
granule can install a larger block mapping than the guest requested. Is
there a missing check here to ensure the resulting page_size matches the
expected fault granule before donating host memory?

> +	if (page_size < 0) {
> +		ret = page_size;
> +		goto unpin;
> +	} else if (page_size == PMD_SIZE) {
> +		if (WARN_ON(!folio_test_large(page_folio(page)))) {
> +			ret = -EINVAL;
> +			goto unpin;
> +		}
> +	}

[Severity: Critical]
Is this folio_test_large() check sufficient to prevent a TOCTOU race
and unintended memory donation?

Since mmap_read_lock() is dropped before this check, a malicious host
userspace could unmap the faulting page and remap it as a PMD-mapped THP.
If the initially pinned page was a smaller mTHP (like a 64KB folio),
folio_test_large() will return true, bypassing the WARN_ON.

transparent_hugepage_adjust() aligns the 64KB folio's PFN down to a PMD
boundary, which is then passed to pkvm_pgtable_stage2_map(), granting
the guest RWX access to 2MB of unrelated physical memory. Could this
be prevented by stricter validation of the page size rather than just
checking if it is large?

> +
> +	ret = account_locked_vm(mm, page_size / PAGE_SIZE, true);
> +	if (ret)
> +		goto unpin;
> +
>  	write_lock(&kvm->mmu_lock);
> -	ret = pkvm_pgtable_stage2_map(pgt, s2fd->fault_ipa, PAGE_SIZE,
> -				      page_to_phys(page), KVM_PGTABLE_PROT_RWX,
> +	ret = pkvm_pgtable_stage2_map(pgt, gfn_to_gpa(gfn), page_size,
> +				      __pfn_to_phys(pfn), KVM_PGTABLE_PROT_RWX,
>  				      hyp_memcache, 0);
>  	write_unlock(&kvm->mmu_lock);
>  	if (ret) {
>  		if (ret == -EAGAIN)
>  			ret = 0;
> -		goto unpin;
> +		goto dec_account;
>  	}

[Severity: High]
Will masking -EAGAIN here cause a deterministic livelock?

If a PMD-sized mapping overlaps with a pre-existing 4K mapping,
pkvm_pgtable_stage2_map() returns -EAGAIN. Masking this error and returning
success without installing a mapping causes the guest to re-execute the
instruction and hit the exact same fault path indefinitely. Should this
fall back to a PAGE_SIZE mapping instead of suppressing the error?

[ ... ]
> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 089b77cf2f6a9..0b591c44998d0 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c
[ ... ]
> @@ -541,10 +539,8 @@ int pkvm_pgtable_stage2_map(struct kvm_pgtable *pgt, u64 addr, u64 size,
>  			return ret ? -EFAULT : -EAGAIN;
>  		}
>  
> -		ret = kvm_call_hyp_nvhe(__pkvm_host_donate_guest, pfn, gfn, 1);
> +		ret = kvm_call_hyp_nvhe(__pkvm_host_donate_guest, pfn, gfn, size / PAGE_SIZE);
>  	} else {

[Severity: High]
Does __pkvm_pgtable_stage2_split() properly verify pinned PFNs when splitting
a THP?

When the hypervisor requests a split, __pkvm_pgtable_stage2_split() calls
pin_user_pages() to pin the remaining pages. However, pkvm_mapping_split()
ignores the returned array and populates the tracking mappings by
sequentially incrementing the old THP's base PFN.

If the host userspace evicts the original THP before the split,
pin_user_pages() pins new anonymous pages that are never recorded and will
leak. Later, __pkvm_pgtable_stage2_reclaim() will call
unpin_user_pages_dirty_lock() on the old THP's subpages 512 times,
causing a refcount underflow.

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