Re: [PATCH v6 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 3 potential issue(s) to consider:

New issues:
- [Medium] `ffa_check_unused_args_sbz` incorrectly enforces 64-bit zeroing on unused registers for SMC32 calls, violating the SMCCC calling convention.
- [Medium] `FFA_FEATURES` returns `FFA_RET_INVALID_PARAMETERS` instead of `FFA_RET_NOT_SUPPORTED` when MBZ parameters are non-zero.

Pre-existing issues:
- [High] The early return paths in `do_ffa_version` leave `res->a1` through `res->a7` uninitialized, resulting in EL2 stack garbage being leaked to the untrusted EL1 host.
--

commit 307381420348c3a7f498a4b56cd09448fc71cde0
Author: Sebastian Ene <sebastianene@google.com>
Subject: 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 calls to this helper across various FF-A handlers.

> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 712811e894352..bd50ddc5b61cc 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 (FFA_MINOR_VERSION(hyp_ffa_version) >= 2)
> +		end_reg = ARM_SMCCC_IS_64(func_id) ? 17 : 7;
> +	for (reg = first_reg; reg <= end_reg; reg++) {
> +		if (cpu_reg(ctxt, reg))
> +			return true;
> +	}
> +
> +	return false;
> +}

[Severity: Medium]
For SMC32 calls, the SMCCC specification states that the upper 32 bits of
parameter registers are UNKNOWN and must be ignored. Does cpu_reg() evaluate
the full 64-bit register here?

If so, this could erroneously reject valid SMC32 calls from the host if their
upper 32 bits contain garbage.

[ ... ]

> @@ -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;
> +	}
> +
>  	if (FFA_MAJOR_VERSION(ffa_req_version) != 1) {
>  		res->a0 = FFA_RET_NOT_SUPPORTED;
>  		return;

[Severity: High]
This is a pre-existing issue, but by just setting res->a0 and returning, do we
leak EL2 stack contents to the host?

kvm_host_ffa_handler() allocates the res structure on the stack uninitialized,
and ffa_set_retval() blindly copies a0-a7 back to the host's context.

Since this early return doesn't initialize res->a1 through res->a7, it appears
to leak 56 bytes of uninitialized stack memory back to the untrusted EL1 host.

[ ... ]

> @@ -984,6 +1069,11 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
>  
>  	switch (func_id) {
>  	case FFA_FEATURES:
> +		if (ffa_check_unused_args_sbz(host_ctxt, 3)) {
> +			ffa_to_smccc_res(&res, FFA_RET_INVALID_PARAMETERS);
> +			goto out_handled;
> +		}
> +
>  		if (!do_ffa_features(&res, host_ctxt))
>  			return false;
>  		goto out_handled;

[Severity: Medium]
According to the FF-A v1.2 specification (Section 13.1.1 FFA_FEATURES), if any
parameter specified as MBZ is non-zero, the ABI must return FFA_ERROR with the
error code FFA_RET_NOT_SUPPORTED.

Should this return FFA_RET_NOT_SUPPORTED instead of FFA_RET_INVALID_PARAMETERS
to match the specification?

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