Re: [PATCH] KVM: arm64: Add support for FFA_PARTITION_INFO_GET

[Vincent Donnefort]

Hi Seb,

On Tue, Apr 09, 2024 at 03:19:08PM +0000, Sebastian Ene wrote:
> Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor
> and copy the response message back to the host buffers. Save the
> returned FF-A version as we will need it later to interpret the response
> from the TEE.
> 
> Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> ---
>  arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 320f2eaa14a9..72fc365bc7a8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -67,6 +67,7 @@ struct kvm_ffa_buffers {
>   */
>  static struct kvm_ffa_buffers hyp_buffers;
>  static struct kvm_ffa_buffers host_buffers;
> +static u32 ffa_version;
>  
>  static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno)
>  {
> @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res,
>  	return true;
>  }
>  
> +static void do_ffa_part_get(struct arm_smccc_res *res,
> +			    struct kvm_cpu_context *ctxt)
> +{
> +	DECLARE_REG(u32, uuid0, ctxt, 1);
> +	DECLARE_REG(u32, uuid1, ctxt, 2);
> +	DECLARE_REG(u32, uuid2, ctxt, 3);
> +	DECLARE_REG(u32, uuid3, ctxt, 4);
> +	DECLARE_REG(u32, flags, ctxt, 5);
> +	u32 off, count, sz, buf_sz;
> +
> +	hyp_spin_lock(&host_buffers.lock);
> +	if (!host_buffers.rx) {
> +		ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
> +		goto out_unlock;
> +	}
> +
> +	arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1,
> +			  uuid2, uuid3, flags, 0, 0,
> +			  res);
> +
> +	if (res->a0 != FFA_SUCCESS)
> +		goto out_unlock;
> +
> +	count = res->a2;
> +	if (!count)
> +		goto out_unlock;

Looking at the table 13.34, it seems what's in "count" depends on the flag.
Shouldn't we check its value, and only memcpy into the host buffers if the flag
is 0?

> +
> +	if (ffa_version > FFA_VERSION_1_0) {
> +		buf_sz = sz = res->a3;
> +		if (sz > sizeof(struct ffa_partition_info))
> +			buf_sz = sizeof(struct ffa_partition_info);

What are you trying to protect against here? We have to trust EL3 anyway, (as
other functions do).

The WARN() could be kept though to make sure we won't overflow our buffer. But
it could be transformed into an error? FFA_RET_ABORTED?


> +	} else {
> +		/* FFA_VERSION_1_0 lacks the size in the response */
> +		buf_sz = sz = 8;
> +	}
> +
> +	WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE);
> +	for (off = 0; off < count * sz; off += sz)
> +		memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz);
> +out_unlock:
> +	hyp_spin_unlock(&host_buffers.lock);
> +}
> +
>  bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
>  {
>  	struct arm_smccc_res res;
> @@ -686,6 +730,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
>  	case FFA_MEM_FRAG_TX:
>  		do_ffa_mem_frag_tx(&res, host_ctxt);
>  		goto out_handled;
> +	case FFA_PARTITION_INFO_GET:
> +		do_ffa_part_get(&res, host_ctxt);
> +		break;
>  	}
>  
>  	if (ffa_call_supported(func_id))
> @@ -726,6 +773,8 @@ int hyp_ffa_init(void *pages)
>  	if (FFA_MAJOR_VERSION(res.a0) != 1)
>  		return -EOPNOTSUPP;
>  
> +	ffa_version = res.a0;
> +
>  	arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res);
>  	if (res.a0 != FFA_SUCCESS)
>  		return -EOPNOTSUPP;
> -- 
> 2.44.0.478.gd926399ef9-goog
>