Re: [PATCH v9 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:
- [High] ffa_check_unused_args_sbz() incorrectly performs a full 64-bit check on unused arguments for SMC32 calls, violating the SMCCC and spuriously rejecting valid hypercalls.
- [Medium] Lockless concurrent read of hyp_ffa_version inside ffa_check_unused_args_sbz creates a data race when evaluated during concurrent FFA_VERSION calls.
--

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

This commit introduces a helper method ffa_check_unused_args_sbz to enforce
strict arguments checking when the hypervisor acts as a relayer between the
host and Trustzone. It adds checks across various FF-A handlers to ensure
unused arguments in SMC calls are strictly zeroed out.

> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 2281ac2e42fcb..5ad74ab265b57 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]
Could this lockless read of hyp_ffa_version create a data race?

When the host kernel executes concurrent FFA_VERSION calls, one CPU might 
evaluate the hyp_ffa_version >= FFA_VERSION_1_2 condition here in 
ffa_check_unused_args_sbz(), while another CPU mutates hyp_ffa_version under 
version_lock in do_ffa_version().

Although the 32-bit read compiles to a single atomic load on ARM64 and the 
result is ultimately discarded for FFA_VERSION because 
ARM_SMCCC_IS_64(func_id) evaluates to false, it is still an unprotected 
concurrent access to shared state.

Would reversing the condition to check ARM_SMCCC_IS_64(func_id) first safely 
short-circuit the read for SMC32 calls like FFA_VERSION?

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

[Severity: High]
Does ffa_check_unused_args_sbz() incorrectly perform a full 64-bit check on 
unused arguments for SMC32 calls?

Under the SMCCC 1.2 specification, for SMC32 calls, only the lower 32 bits 
of the registers (W0-W7) are used, and the upper 32 bits are UNPREDICTABLE. 
However, cpu_reg(ctxt, reg) returns the full 64-bit register value.

If a compliant host issues any SMC32 FF-A hypercall (such as FFA_VERSION) 
and leaves non-zero residual data in the upper 32 bits of the unused 
registers, will this strictly reject valid SMC32 hypercalls with 
FFA_RET_INVALID_PARAMETERS? 

Should the register value be masked to 32 bits when ARM_SMCCC_IS_64(func_id) 
is false?

> +	}
> +
> +	return false;
> +}
> +

[ ... ]

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