Re: [PATCH v2 04/18] KVM: arm64: Add a heap allocator for the pKVM hyp

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Jul 06, 2026 at 06:12:41PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] hyp_allocator_map remaps already-mapped pages and corrupts unmapped tracker state due to unaligned next chunks.
> - [High] Fatal EL2 WARN_ON creates a dead-branch trap and panics the hypervisor on global lock contention in hyp_allocator_alloc.
> --
> 
> commit d07913766ae6b18b86e593f40b4fc4a8165e2722
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> KVM: arm64: Add a heap allocator for the pKVM hyp
> 
> This commit creates a heap allocator to manage VA-contiguous memory for the
> hypervisor, replacing donations embedded within HVCs. It supports allocating,
> freeing, topping up from a host memcache, and reclaiming memory.
> 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/alloc.c b/arch/arm64/kvm/hyp/nvhe/alloc.c
> > index 0000000000000..07ef0a13855f7
> > --- /dev/null
> > +++ b/arch/arm64/kvm/hyp/nvhe/alloc.c
> 
> [ ... ]
> 
> > +static int hyp_allocator_map(struct hyp_allocator *allocator, struct chunk_hdr *chunk,
> > +			     struct chunk_hdr *next,
> > +			     void *addr, void *end)
> > +{
> > +	void *unmapped = chunk ? chunk_unmapped(chunk) : allocator->first_unmapped;
> > +
> > +	/*
> > +	 * hyp_allocator_can_create_chunk() already validates addr/end
> > +	 * belong to the chunk.
> > +	 */
> > +	WARN_ON(end <= addr);
> > +
> > +	/* The chunk does not span an unmapped region */
> > +	if (!unmapped)
> > +		return 0;
> > +
> > +	while (unmapped < end) {
> > +		void *page = pop_hyp_memcache(&allocator->mc, hyp_phys_to_virt);
> > +		int ret;
> > +
> > +		if (!page) {
> > +			end = PTR_ALIGN(end, PAGE_SIZE);
> > +			*this_cpu_ptr(allocator->topup_needed) =
> > +				(unsigned long)(end - unmapped) >> PAGE_SHIFT;
> > +			return -ENOMEM;
> > +		}
> > +
> > +		ret = __hyp_allocator_map(unmapped, hyp_virt_to_phys(page));
> 
> [Severity: Critical]
> Does this loop correctly handle unaligned next chunks when re-mapping
> reclaimed memory?
> 
> If the allocator reclaims a chunk where the next chunk is unaligned (e.g., at
> PAGE_SIZE + 32), the unmapped hole is safely truncated to PAGE_SIZE by
> PTR_ALIGN_DOWN(next, PAGE_SIZE). However, if the chunk is later reused with
> an end address extending into the partially shared page (e.g., PAGE_SIZE + 16):
> 
> The loop in hyp_allocator_map() iterates while (unmapped < end). When unmapped
> reaches the end of the true hole (PAGE_SIZE), the loop continues because
> PAGE_SIZE < PAGE_SIZE + 16. It then pops a new physical page and maps it at
> PAGE_SIZE, overwriting the existing active PTE containing the next chunk.
> 
> Additionally, the unmapped == next reset check fails because of the unaligned
> next pointer, propagating the corrupted tracker state.

I don't believe this can happen. Chunks __always__ describe a mapped section,
followed (or not) by an unmapped one. So if we are adding mappings to a chunk,
next has to be PAGE_ALIGNED.

However, I could add a WARN_ON() here.


> 
> [ ... ]
> 
> > +static void *hyp_allocator_alloc(struct hyp_allocator *allocator, size_t size)
> > +{
> > +	struct chunk_hdr *chunk;
> > +
> > +	size = max(size, MIN_ALLOC_SIZE);
> > +
> > +	/* Ensure we do not overflow ALIGN(MIN_ALLOC_SIZE) */
> > +	if (size > U32_MAX) {
> > +		hyp_allocator_set_errno(allocator, -E2BIG);
> > +		return NULL;
> > +	}
> > +
> > +	size = ALIGN(size, MIN_ALLOC_SIZE);
> > +	if (size > (allocator->end - allocator->start - chunk_hdr_size())) {
> > +		hyp_allocator_set_errno(allocator, -E2BIG);
> > +		return NULL;
> > +	}
> > +
> > +#ifdef CONFIG_NVHE_EL2_DEBUG
> > +	/* The allocator can modify the hyp stage-1 */
> > +	if (WARN_ON(hyp_spin_is_locked(&pkvm_pgd_lock))) {
> > +		hyp_allocator_set_errno(allocator, -EINVAL);
> > +		return NULL;
> > +	}
> > +#endif
> 
> [Severity: High]
> Can this lock check trigger a hypervisor panic under normal SMP contention?
> 
> At EL2, WARN_ON() expands to a fatal BRK instruction, meaning the error
> handling path here acts as a dead-branch trap and will never execute.
> 
> Furthermore, hyp_spin_is_locked() returns true if the lock is held by any CPU,
> not just the local one. If host userspace triggers concurrent VM creations or
> VCPU initializations, driving concurrent EL2 hypercalls to hyp_alloc(), CPU A
> might hold pkvm_pgd_lock while CPU B concurrently evaluates this WARN_ON().
> 
> This converts standard SMP lock contention into a fatal host denial of service.

Duh, this hyp_spin_is_locked() is of course utterly broken! I'll add a
comment for hyp_alloc() instead.

> 
> > +	scoped_guard(hyp_spinlock, &allocator->lock) {
> > +		chunk = hyp_allocator_alloc_chunk(allocator, size);
> > +		if (IS_ERR_OR_NULL(chunk)) {
> > +			hyp_allocator_set_errno(allocator,
> > +						IS_ERR(chunk) ? PTR_ERR(chunk) : -EINVAL);
> > +			return NULL;
> > +		}
> > +	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260706175415.2604046-1-vdonnefort@google.com?part=4