Messages in this thread | | | Date | Wed, 16 Nov 2022 17:48:42 +0000 | From | Oliver Upton <> | Subject | Re: [PATCH 03/12] KVM: arm64: Block unsafe FF-A calls from the host |
| |
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.
> +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?
> + > + 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.
-- Thanks, Oliver
| |