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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Improper Trust Boundary: Selftest hypercall is exposed in the always-on SMCCC band.
- [Medium] Data race and missing mutual exclusion in `hyp_allocator_selftest()` against concurrent reclaim.
--

commit 75aa2a47b5c2aac4b561278263e6a43bfd0479e2
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Add selftests for the pKVM heap allocator

This commit introduces 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 fc3c52b031541..0a8df74a7b4e6 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]
Does placing the selftest hypercall here expose it to the host kernel
after deprivilege? 

By placing it before the PKVM_ONLY marker instead of before the MIN_PKVM
marker, this hypercall becomes always available rather than init-only.

This appears to expose boot-only test code and its memory management
operations to the untrusted host kernel at runtime, which expands the
attack surface and violates the pKVM least-privilege security model.

> diff --git a/arch/arm64/kvm/hyp/nvhe/alloc.c b/arch/arm64/kvm/hyp/nvhe/alloc.c
> index 32435782030bc..3b83de90472a8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/alloc.c
> +++ b/arch/arm64/kvm/hyp/nvhe/alloc.c

[ ... ]

> +int hyp_allocator_selftest(void)
> +{
> +	struct hyp_allocator *allocator = &selftest_allocator;
> +	static DEFINE_HYP_SPINLOCK(selftest_lock);
> +	struct kvm_hyp_memcache host_mc = { };
> +	void *addr1, *addr2, *addr3, *addr4;
> +	int ret;
> +
> +	guard(hyp_spinlock)(&selftest_lock);
> +
> +	if (allocator->mc.nr_pages < SELFTEST_MAX_PAGES) {

[Severity: Medium]
Could this read of mc.nr_pages race with concurrent reclaim or topup
operations? 

It looks like this accesses the allocator state locklessly because
allocator->lock is not held here, only the local selftest_lock.

> +		*this_cpu_ptr(allocator->topup_needed) = SELFTEST_MAX_PAGES -
> +							 allocator->mc.nr_pages;
> +		return -ENOMEM;
> +	}
> +
> +	selftest_alloc(SELFTEST_MAX_SIZE);
> +	if (selftest_errno() != -E2BIG)
> +		return -EINVAL;
> +
> +	selftest_alloc(SIZE_MAX);
> +	if (selftest_errno() != -E2BIG)
> +		return -EINVAL;
> +
> +	/* Test first chunk */
> +	addr1 = selftest_alloc(0);
> +	if (!addr1 || addr1 != (void *)allocator->start + chunk_hdr_size())
> +		return -EINVAL;
> +
> +	/* Test second contiguous chunk with unaligned size */
> +	addr2 = selftest_alloc(MIN_ALLOC_SIZE + 1);

[Severity: Medium]
Because the selftest hypercall is exposed post-deprivilege, could a
concurrent host hypercall steal chunks from the selftest allocator while
this test is running?

Since allocator->lock is not held across this sequence of allocations and
frees, a concurrent reclaim operation might mutate the allocator state. This
would break the selftest's deterministic layout sequence and result in
spurious failures.

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