Re: [PATCH 2/2] KVM: arm64: Support FFA_FN64_MEM_RECLAIM2 in host handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: arm64: incorrect buffer bounds in do_ffa_mem_reclaim2()
--

commit 9e4ba2a6fb44613f009459976518be9371118818
Author: Snehal Koukuntla <snehalreddy@google.com>

KVM: arm64: Support FFA_FN64_MEM_RECLAIM2 in host handler

This commit adds support for the FFA_MEM_RECLAIM2 interface in the host
FF-A proxy handler. It forwards the SMC call to the SPMD and unshares the
reclaimed page ranges in the host stage-2 page tables.

Link: https://support.arm.com/documentation/den0140/j

> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 5ad74ab265b57..e0fba07a67bc9 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
[ ... ]
> @@ -611,6 +624,107 @@ static void __do_ffa_mem_xfer(const u64 func_id,
[ ... ]
> +static void do_ffa_mem_reclaim2(struct arm_smccc_1_2_regs *res,
> +				struct kvm_cpu_context *ctxt)
> +{
> +	DECLARE_REG(u64, handle, ctxt, 1);
> +	DECLARE_REG(u64, flags, ctxt, 2);
> +	DECLARE_REG(u64, x3, ctxt, 3);
> +	DECLARE_REG(u64, x4, ctxt, 4);
> +	DECLARE_REG(u64, x5, ctxt, 5);
> +	u32 req_desc_cnt = (u32)(x3 >> 32);
> +	u32 total_pages = (u32)x3;
> +	u32 desc_size = (u32)x4;
> +	u32 offset = (u32)x5;
> +	u32 page_index = (u32)(x5 >> 32);
[ ... ]
> +	if (offset >= (u64)req_desc_cnt * desc_size) {
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		return;
> +	}

[Severity: High]
Could this check incorrectly reject valid offsets? By checking offset against
the total length of the requested descriptors (req_desc_cnt * desc_size), it
seems to conflate the descriptor count with the total buffer capacity.

> +
> +	hyp_spin_lock(&host_buffers.lock);
> +	if (!host_buffers.tx) {
> +		ret = FFA_RET_INVALID_PARAMETERS;
> +		goto out_unlock;
> +	}
> +
> +	memcpy(hyp_buffers.tx, host_buffers.tx, (u64)req_desc_cnt * desc_size);

[Severity: High]
Is there a risk that this copies too few bytes when offset is greater than
zero? Since the start of the required descriptors in the buffer is shifted by
offset bytes, copying only req_desc_cnt * desc_size bytes from the beginning
of the host buffer misses the trailing descriptors.

If the host EL1 intentionally issues a hypercall with an offset greater than
zero, this might lead to the SPMD reading uninitialized or stale memory from
the hypervisor TX buffer.

> +
> +	range = (void *)hyp_buffers.tx + offset;
> +	if (page_index >= range->pg_cnt) {
> +		ret = FFA_RET_INVALID_PARAMETERS;
> +		goto out_unlock;
> +	}
> +
> +	ffa_mem_reclaim2(res, handle, flags, x3, x4, x5);
> +	if (res->a0 != FFA_SUCCESS)
> +		goto out_unlock;
> +
> +	reclaimed_pages = res->a3;
> +
> +	for (in_idx = offset / desc_size; in_idx < req_desc_cnt; in_idx++) {

[Severity: High]
Does this loop iterate fewer times than the number of descriptors reclaimed
by the SPMD?

Starting the loop index at offset / desc_size and ending it at req_desc_cnt
means the loop only executes (req_desc_cnt - offset / desc_size) times.
This appears to unshare fewer descriptors than the SPMD actually processed.

If the untrusted host EL1 triggers this path, it could leave the hypervisor
stage-2 ownership tracking permanently desynchronized from the SPMD state.

> +		struct ffa_mem_region_addr_range temp;
> +		u32 pg_cnt, to_reclaim;
> +
> +		if (reclaimed_count >= reclaimed_pages)
> +			break;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921164748.2958578-1-snehalreddy@google.com?part=2