Re: [PATCH v2 18/18] KVM: arm64: Alloc simple_buffer_page using pKVM hyp allocator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing validation of host-supplied `nr_page_va` causes EL2 memory leak and host infinite loop.
--

commit 20e38c941fd5614b7d02825f2350aeac0276e149
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Alloc simple_buffer_page using pKVM hyp allocator
    
This patch transitions the allocation of the simple_ring_buffer structures
from the host to the hypervisor in protected mode using the new pKVM heap
allocator. It uses pkvm_call_hyp_req() to handle potential top-up requests
during allocation.

> diff --git a/arch/arm64/kvm/hyp/nvhe/trace.c b/arch/arm64/kvm/hyp/nvhe/trace.c
> index 6e716295247ad..bcfe06bba29bb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/trace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/trace.c

[ ... ]

> @@ -62,18 +63,34 @@ static void __release_host_mem(void *start, u64 size)
>  	WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(start), size >> PAGE_SHIFT));
>  }
>  
> -static int hyp_trace_buffer_load_bpage_backing(struct hyp_trace_buffer *trace_buffer,
> -					       struct hyp_trace_desc *desc)
> +static int hyp_trace_buffer_alloc_bpages(struct hyp_trace_buffer *trace_buffer,
> +					 struct hyp_trace_desc *desc)
>  {
> -	void *start = (void *)kern_hyp_va(desc->bpages_backing_start);
> -	size_t size = desc->bpages_backing_size;
> +	void *start;
> +	size_t size;
>  	int ret;
>  
> -	ret = __admit_host_mem(start, size);
> -	if (ret)
> -		return ret;
> +	if (is_protected_kvm_enabled()) {
> +		struct ring_buffer_desc *rb_desc;
> +		int cpu;
> +
> +		size = 0;
> +		for_each_ring_buffer_desc(rb_desc, cpu, &desc->trace_buffer_desc)
> +			size += rb_desc->nr_page_va * sizeof(struct simple_buffer_page);
> +
> +		start = hyp_alloc(size);

[Severity: High]
Is there a missing bounds check on the host-supplied nr_page_va here?

If the host sets nr_page_va to 0 for all CPUs, size will be 0, but
hyp_alloc(0) will still succeed by allocating MIN_ALLOC_SIZE.

> +		if (!start)
> +			return hyp_alloc_errno();
> +	} else {
> +		start = (void *)kern_hyp_va(desc->bpages_backing_start);
> +		size = desc->bpages_backing_size;
>  
> -	memset(start, 0, size);
> +		ret = __admit_host_mem(start, size);
> +		if (ret)
> +			return ret;
> +
> +		memset(start, 0, size);
> +	}

[ ... ]

> @@ -81,7 +98,7 @@ static int hyp_trace_buffer_load_bpage_backing(struct hyp_trace_buffer *trace_bu
>  	return 0;
>  }
>  
> -static void hyp_trace_buffer_unload_bpage_backing(struct hyp_trace_buffer *trace_buffer)
> +static void hyp_trace_buffer_free_bpages(struct hyp_trace_buffer *trace_buffer)
>  {
>  	void *start = trace_buffer->bpages_backing_start;
>  	size_t size = trace_buffer->bpages_backing_size;
> @@ -89,9 +106,12 @@ static void hyp_trace_buffer_unload_bpage_backing(struct hyp_trace_buffer *trace
>  	if (!size)
>  		return;

[Severity: High]
Will this early return permanently leak the memory allocated above?

If size evaluated to 0 during allocation, simple_ring_buffer_init_mm()
fails with -EINVAL and triggers this cleanup path. Since size is 0, this
returns early without calling hyp_free(), leaking the MIN_ALLOC_SIZE chunk
in the EL2 heap.

Could exhausting the EL2 heap this way cause a subsequent 0-page topup
request back to the host, resulting in an infinite loop in the host CPU's
pkvm_call_hyp_req() macro?

> -	memset(start, 0, size);
> -
> -	__release_host_mem(start, size);
> +	if (is_protected_kvm_enabled()) {
> +		hyp_free(start);
> +	} else {
> +		memset(start, 0, size);
> +		__release_host_mem(start, size);
> +	}
>  
>  	trace_buffer->bpages_backing_start = 0;
>  	trace_buffer->bpages_backing_size = 0;

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