Re: [PATCH v7 7/7] KVM: arm64: Enforce strict SBZ checks in the FF-A proxy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Lockless read of `hyp_ffa_version` in `ffa_check_unused_args_sbz()` races with concurrent writes.

Pre-existing issues:
- [High] Uninitialized fields in the `res` structure leak EL2 stack memory to the host during FF-A version negotiation.
--

commit a19572356269d847d64eebb97bb025807ca073f2
Author: Sebastian Ene <sebastianene@google.com>
KVM: arm64: Enforce strict SBZ checks in the FF-A proxy

This patch introduces a helper function to enforce strict argument checking
when the hypervisor acts as a relayer between the host and Trustzone. It
adds calls to this new helper across various FF-A hypercall handlers to
ensure unused arguments are zeroed.

> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 712811e894352..334f8a28d942d 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -74,6 +74,21 @@ static u32 hyp_ffa_version;
>  static bool has_version_negotiated;
>  static hyp_spinlock_t version_lock;
>  
> +static bool ffa_check_unused_args_sbz(struct kvm_cpu_context *ctxt, int first_reg)
> +{
> +	DECLARE_REG(u32, func_id, ctxt, 0);
> +	int reg, end_reg = 7;
> +
> +	if (hyp_ffa_version >= FFA_VERSION_1_2 && ARM_SMCCC_IS_64(func_id))

[Severity: Medium]
Can this lockless read of hyp_ffa_version cause a data race?

During host initialization, multiple CPUs could issue the FFA_VERSION
hypercall concurrently. If CPU A acquires version_lock in do_ffa_version()
and writes to hyp_ffa_version, CPU B entering do_ffa_version() and calling
ffa_check_unused_args_sbz() will evaluate this condition. 

Since logical AND evaluates left-to-right, CPU B could locklessly read
hyp_ffa_version while CPU A is writing it. Swapping the conditions to check
ARM_SMCCC_IS_64(func_id) first would short-circuit the evaluation for
FFA_VERSION (which is an SMC32 call) and avoid the unprotected read entirely.

> +		end_reg = 17;
> +	for (reg = first_reg; reg <= end_reg; reg++) {
> +		if (cpu_reg(ctxt, reg))
> +			return true;
> +	}
> +
> +	return false;
> +}

[ ... ]

> @@ -764,6 +804,11 @@ static void do_ffa_version(struct arm_smccc_1_2_regs *res,
>  {
>  	DECLARE_REG(u32, ffa_req_version, ctxt, 1);
>  
> +	if (ffa_check_unused_args_sbz(ctxt, 2)) {
> +		res->a0 = FFA_RET_NOT_SUPPORTED;
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but does this code leak uninitialized EL2
stack memory to the host?

In kvm_host_ffa_handler(), an uninitialized struct arm_smccc_1_2_regs res
is declared on the stack. When do_ffa_version() is called (both on this
newly added failure path and the existing paths), it only manually assigns
res->a0 before returning.

Back in kvm_host_ffa_handler(), ffa_set_retval(host_ctxt, &res) then
blindly copies the entire structure into the host context, which exposes
the uninitialized a1-a17 fields to the untrusted host.

> +
>  	if (FFA_MAJOR_VERSION(ffa_req_version) != 1) {
>  		res->a0 = FFA_RET_NOT_SUPPORTED;
>  		return;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629093558.2425257-1-sebastianene@google.com?part=7