Re: [PATCH 03/12] KVM: arm64: Block unsafe FF-A calls from the host

[Will Deacon]

On Wed, Nov 16, 2022 at 05:48:42PM +0000, Oliver Upton wrote:
> Sorry, hit send a bit too early. Reviewing the patch itself:
> 
> On Wed, Nov 16, 2022 at 05:03:26PM +0000, Quentin Perret wrote:
> 
> [...]
> 
> > +static bool ffa_call_unsupported(u64 func_id)
> > +{
> > +	switch (func_id) {
> > +	/* Unsupported memory management calls */
> > +	case FFA_FN64_MEM_RETRIEVE_REQ:
> > +	case FFA_MEM_RETRIEVE_RESP:
> > +	case FFA_MEM_RELINQUISH:
> > +	case FFA_MEM_OP_PAUSE:
> > +	case FFA_MEM_OP_RESUME:
> > +	case FFA_MEM_FRAG_RX:
> > +	case FFA_FN64_MEM_DONATE:
> > +	/* Indirect message passing via RX/TX buffers */
> > +	case FFA_MSG_SEND:
> > +	case FFA_MSG_POLL:
> > +	case FFA_MSG_WAIT:
> > +	/* 32-bit variants of 64-bit calls */
> > +	case FFA_MSG_SEND_DIRECT_REQ:
> > +	case FFA_MSG_SEND_DIRECT_RESP:
> > +	case FFA_RXTX_MAP:
> > +	case FFA_MEM_DONATE:
> > +	case FFA_MEM_RETRIEVE_REQ:
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> 
> Wouldn't an allowlist behave better in this case? While unlikely, you
> wouldn't want EL3 implementing some FFA_BACKDOOR_PVM SMC that falls
> outside of the denylist and is passed through.

Given that we're not intercepting all SMCs (rather, only those in the
FF-A service range), I think the denylist works much better because the
default action is "allow". We _have_ to trust EL3 regardless, as it
could just use any allowed SMC for a backdoor if it wanted. Ultimately,
EL3 runs the show in the hierarchical security model of the architecture
and we sadly can't do much about that.

> > +bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt)
> > +{
> > +	DECLARE_REG(u64, func_id, host_ctxt, 0);
> > +	struct arm_smccc_res res;
> > +
> > +	if (!is_ffa_call(func_id))
> > +		return false;
> > +
> > +	switch (func_id) {
> > +	/* Memory management */
> > +	case FFA_FN64_RXTX_MAP:
> > +	case FFA_RXTX_UNMAP:
> > +	case FFA_MEM_SHARE:
> > +	case FFA_FN64_MEM_SHARE:
> > +	case FFA_MEM_LEND:
> > +	case FFA_FN64_MEM_LEND:
> > +	case FFA_MEM_RECLAIM:
> > +	case FFA_MEM_FRAG_TX:
> > +		break;
> > +	}
> 
> What is the purpose of this switch?

As of this patch, it's not serving any functional purpose. The idea is
that later patches hook in here to provide handling at EL2. I'll remove
it and introduce it bit by bit.

> 
> > +
> > +	if (!ffa_call_unsupported(func_id))
> > +		return false; /* Pass through */
> 
> Another (tiny) benefit of implementing an allowlist is that it avoids
> the use of double-negative logic like this.

I should just rework this to be ffa_call_supported().

Will