Re: [PATCH v4 09/17] KVM: arm64: Add selftests for the pKVM heap allocator

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The pKVM hyp allocator fails to map backing memory for chunks appended to a previous chunk that does not explicitly track the unmapped boundary, leading to a fatal EL2 data abort.
- [Medium] The `__pkvm_hyp_alloc_selftest` hypercall is placed in the always-on band instead of the init-only band, violating hypervisor lifecycle rules.
- [Medium] Physical pages temporarily held in the local `host_mc` memcache are permanently leaked if an assertion fails during the selftest.
--

--- Patch [9]: [PATCH v4 09/17] KVM: arm64: Add selftests for the pKVM heap allocator ---
commit 68a0bf1c1fb29e6fed5c0a59e15840d4a948bd3e
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Add selftests for the pKVM heap allocator

Introduce a comprehensive runtime selftest for the pKVM hypervisor heap
allocator, executed during init when CONFIG_NVHE_EL2_DEBUG is enabled.

> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 0c3126179704f..08466f608d1f5 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -92,6 +92,7 @@ enum __kvm_host_smccc_func {
>  	__KVM_HOST_SMCCC_FUNC___pkvm_hyp_topup,
>  	__KVM_HOST_SMCCC_FUNC___pkvm_hyp_reclaim,
>  	__KVM_HOST_SMCCC_FUNC___pkvm_hyp_reclaimable,
> +	__KVM_HOST_SMCCC_FUNC___pkvm_hyp_alloc_selftest,
>  
>  	MARKER(__KVM_HOST_SMCCC_FUNC_PKVM_ONLY),

[Severity: Medium]
Is __pkvm_hyp_alloc_selftest placed in the correct hypercall ID band here?

Placing it before the PKVM_ONLY marker puts the hypercall in the always-on
band rather than the init-only band. Since this is an initialization test
meant to run before pKVM finalization, should it be placed in the init-only
band to ensure it is not exposed to the untrusted host kernel
post-deprivilege?

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/nvhe/alloc.c b/arch/arm64/kvm/hyp/nvhe/alloc.c
> index af4ab53784f7f..de636fdf976bf 100644
> --- a/arch/arm64/kvm/hyp/nvhe/alloc.c
> +++ b/arch/arm64/kvm/hyp/nvhe/alloc.c
[ ... ]
> +	/* Test reclaiming */
> +	if (addr1 != selftest_alloc(0))
> +		return -EINVAL;
> +	if (addr2 != selftest_alloc(PAGE_SIZE * 2))
> +		return -EINVAL;
> +	addr3 = selftest_alloc(0);
> +	addr4 = selftest_alloc(PAGE_SIZE);

[Severity: High]
Does allocating addr4 here trigger a data abort due to unmapped memory?

When appending a new chunk, hyp_allocator_map() uses chunk_unmapped(prev) to
find the unmapped boundary. If the previous chunk (addr3) does not track it
(which occurs when the boundary is held by an earlier chunk), it returns 0
and skips mapping.

Because addr4 is allocated after addr3, it crosses the actual unmapped
boundary without mapping new pages. This leaves the new region unmapped,
which leads to a fatal EL2 data abort when the hypervisor initializes and
accesses the memory.

[ ... ]
> +
> +	/* Test punching a hole in the middle of a free chunk ... */
> +	selftest_free(addr2);
> +	hyp_allocator_reclaim(allocator, &host_mc, SELFTEST_MAX_PAGES);
> +	if (host_mc.nr_pages != SELFTEST_MAX_PAGES - 2)
> +		return -EINVAL;
> +
> +	if (selftest_alloc(PAGE_SIZE))
> +		return -EINVAL;
> +	if (selftest_errno() != -ENOMEM)
> +		return -EINVAL;

[Severity: Medium]
Do these error paths leak the physical pages currently held in host_mc?

The hyp_allocator_selftest() function temporarily stores reclaimed physical
pages in the local host_mc memcache stack variable. If any of these
subsequent assertions fail, the function returns -EINVAL directly without
draining host_mc or returning the pages to the host, resulting in a permanent
memory leak.

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