Re: [PATCH 1/4] KVM: arm64: Trap FFA_VERSION host call in pKVM

[Will Deacon]

Hi Seb,

On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote:
> The pKVM hypervisor initializes with FF-A version 1.0. Keep the
> supported version inside the host structure and prevent the host
> drivers from overwriting the FF-A version with an increased version.
> Without trapping the call, the host drivers can negotiate a higher
> version number with TEE which can result in a different memory layout
> described during the memory sharing calls.
> 
> Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> ---
>  arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 320f2eaa14a9..023712e8beeb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
>  	hyp_spinlock_t lock;
>  	void *tx;
>  	void *rx;
> +	u32 ffa_version;
>  };

Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and
Secure side will end up using the same version, so a simple global
variable would suffice, no?

>  /*
> @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
>  	return true;
>  }
>  
> +static void do_ffa_version(struct arm_smccc_res *res,
> +			   struct kvm_cpu_context *ctxt)
> +{
> +	DECLARE_REG(u32, ffa_req_version, ctxt, 1);
> +	u32 current_version;
> +
> +	hyp_spin_lock(&host_buffers.lock);

Why do you need to take the lock for this?

> +	current_version = host_buffers.ffa_version;
> +	if (FFA_MAJOR_VERSION(ffa_req_version) != FFA_MAJOR_VERSION(current_version)) {
> +		res->a0 = FFA_RET_NOT_SUPPORTED;
> +		goto unlock;
> +	}

We won't have probed the proxy if the Secure side doesn't support 1.x
so I think you should just do:

  if (FFA_MAJOR_VERSION(ffa_req_version) != 1)
	...

> +	/*
> +	 * If the client driver tries to downgrade the version, we need to ask
> +	 * first if TEE supports it.
> +	 */
> +	if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {

Similarly here, I don't think 'current_version' is what we should expose.
Rather, we should be returning the highest version that the proxy
supports in the host, which is 1.0 at this point in the patch series.

> +		arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
> +				  0, 0, 0, 0, 0,
> +				  res);

Hmm, I'm struggling to see how this is supposed to work per the spec.
The FF-A spec says:

  | ... negotiation of the version must happen before an invocation of
  | any other FF-A ABI.

and:

  | Once the caller invokes any FF-A ABI other than FFA_VERSION, the
  | version negotiation phase is complete.
  |
  | Once an FF-A version has been negotiated between a caller and a
  | callee, the version may not be changed for the lifetime of the
  | calling component. The callee must treat the negotiated version as
  | the only supported version for any subsequent interactions with the
  | caller.

So by the time we get here, we've already settled on our version with
the Secure side and the host cannot downgrade.

That's a bit rubbish if you ask me, but I think it means we'll have to
defer some of the proxy initialisation until the host calls FFA_VERSION,
at which point we'll need to negotiate a common version between the host,
the proxy and Secure. Once we've done that, our FFA_VERSION handler will
just return that negotiated version.

Will