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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] pkvm_pin_user_pages() returns 0 on -EHWPOISON without initializing output parameters, leading to undefined behavior and potential system compromise in pkvm_mem_abort().
- [Medium] Unprivileged host userspace can trigger a WARN_ON_ONCE in pkvm_pin_user_pages() via TOCTOU, leading to a host-side denial of service.
--

commit 3d6ed4192aebc28eca33a0d8ed20d5a587ffa2cd
Author: Vincent Donnefort <vdonnefort@google.com>

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

This commit enables PMD-sized stage-2 block mappings for protected VMs when
the stage-1 mapping is backed by THPs. An entire PMD_SIZE mapping is donated
to the guest upon finding a THP.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 9ba86450fe4af..218df096c72ed 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> +static int pkvm_pin_user_pages(const struct kvm_s2_fault_desc *s2fd, struct page **__page,
> +			       unsigned long *__size, kvm_pfn_t *__pfn, gfn_t *__gfn)
>  {
>  	unsigned int flags = FOLL_HWPOISON | FOLL_LONGTERM | FOLL_WRITE;
>  	struct kvm_vcpu *vcpu = s2fd->vcpu;
>  	struct mm_struct *mm = current->mm;
>  	struct kvm *kvm = vcpu->kvm;
>  	struct page *page;
> +	kvm_pfn_t pfn;
> +	gfn_t gfn;
> +	long ret;
>  
> -	mmap_read_lock(mm);
> +	guard(mmap_read_lock)(mm);
>  
>  	ret = pin_user_pages(s2fd->hva, 1, flags, &page);
> -	mmap_read_unlock(mm);
> -
>  	if (ret == -EHWPOISON) {
>  		kvm_send_hwpoison_signal(s2fd->hva, PAGE_SHIFT);
> -		ret = 0;
> -		goto dec_account;
> +		return 0;

[Severity: Critical]
If pin_user_pages() returns -EHWPOISON, does this return 0 without
initializing the output parameters (__page, __size, __pfn, __gfn)?

>  	} else if (ret != 1) {
> -		ret = -EFAULT;
> -		goto dec_account;
> +		return -EFAULT;
>  	} else if (!folio_test_swapbacked(page_folio(page))) {
[ ... ]
> -		ret = -EIO;
> -		goto unpin;
> +		unpin_user_page(page);
> +		return -EIO;
> +	}
> +
> +	pfn = page_to_pfn(page);
> +	gfn = gpa_to_gfn(s2fd->fault_ipa);
> +
> +	ret = transparent_hugepage_adjust(kvm, s2fd->memslot, s2fd->hva, &pfn, &gfn);
> +	if (ret < 0) {
> +		unpin_user_page(page);
> +		return ret;
> +	} else if (ret == PMD_SIZE && WARN_ON_ONCE(folio_size(page_folio(page)) < PMD_SIZE)) {
> +		unpin_user_page(page);
> +		return -EINVAL;
>  	}

[Severity: Medium]
Could a concurrent page table modification trigger this warning? Since
mmap_read_lock() is held here, if a concurrent thread holding the per-VMA
lock (like madvise MADV_DONTNEED) zaps the page table and faults in a
PMD-mapped THP at the same address, transparent_hugepage_adjust() might
observe the new PMD mapping while the initially pinned page remains 4K.

As this warning is reachable from unprivileged host userspace, could this
be used for a host-side denial of service?

> +
> +	*__page = page;
> +	*__size = ret;
> +	*__pfn = pfn;
> +	*__gfn = gfn;
> +
> +	return 0;
> +}
> +
> +static int pkvm_mem_abort(const struct kvm_s2_fault_desc *s2fd)
> +{
> +	struct kvm_vcpu *vcpu = s2fd->vcpu;
> +	struct kvm_pgtable *pgt = vcpu->arch.hw_mmu->pgt;
> +	struct mm_struct *mm = current->mm;
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned long size;
> +	void *hyp_memcache;
> +	struct page *page;
> +	kvm_pfn_t pfn;
> +	gfn_t gfn;
> +	int ret;
> +
> +	hyp_memcache = get_mmu_memcache(vcpu);
> +	ret = topup_mmu_memcache(vcpu, hyp_memcache);
> +	if (ret)
> +		return -ENOMEM;
> +
> +	ret = pkvm_pin_user_pages(s2fd, &page, &size, &pfn, &gfn);
> +	if (ret)
> +		return ret;
> +
> +	ret = account_locked_vm(mm, size / PAGE_SIZE, true);

[Severity: Critical]
When pkvm_pin_user_pages() encounters a poisoned page, it returns 0. Will
this caller then proceed to use the uninitialized size, page, pfn, and gfn
variables?

It looks like this could result in mapping arbitrary, uninitialized
physical memory frames into the guest's stage-2 page tables. Furthermore,
if the mapping fails, the error path appears to call unpin_user_page()
with a garbage page pointer, which might cause a host kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911135053.146435-1-vdonnefort@google.com?part=22