Hi Jens,
> On 24 Oct 2024, at 10:50, Jens Wiklander <[email protected]> wrote:
>
> Hi Bertrand,
>
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <[email protected]> wrote:
>>
>> Add support for FFA_MSG_SEND2 to send indirect messages from a VM to a
>> secure partition.
>>
>> Signed-off-by: Bertrand Marquis <[email protected]>
>> ---
>> Changes in v2:
>> - rebase
>> ---
>> xen/arch/arm/tee/ffa.c | 5 ++++
>> xen/arch/arm/tee/ffa_msg.c | 49 ++++++++++++++++++++++++++++++++++
>> xen/arch/arm/tee/ffa_private.h | 1 +
>> 3 files changed, 55 insertions(+)
>>
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 3a9525aa4598..21d41b452dc9 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -101,6 +101,7 @@ static const struct ffa_fw_abi ffa_fw_abi_needed[] = {
>> FW_ABI(FFA_MEM_RECLAIM),
>> FW_ABI(FFA_MSG_SEND_DIRECT_REQ_32),
>> FW_ABI(FFA_MSG_SEND_DIRECT_REQ_64),
>> + FW_ABI(FFA_MSG_SEND2),
>> };
>>
>> /*
>> @@ -195,6 +196,7 @@ static void handle_features(struct cpu_user_regs *regs)
>> case FFA_PARTITION_INFO_GET:
>> case FFA_MSG_SEND_DIRECT_REQ_32:
>> case FFA_MSG_SEND_DIRECT_REQ_64:
>> + case FFA_MSG_SEND2:
>> ffa_set_regs_success(regs, 0, 0);
>> break;
>> case FFA_MEM_SHARE_64:
>> @@ -275,6 +277,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>> case FFA_MSG_SEND_DIRECT_REQ_64:
>> ffa_handle_msg_send_direct_req(regs, fid);
>> return true;
>> + case FFA_MSG_SEND2:
>> + e = ffa_handle_msg_send2(regs);
>> + break;
>> case FFA_MEM_SHARE_32:
>> case FFA_MEM_SHARE_64:
>> ffa_handle_mem_share(regs);
>> diff --git a/xen/arch/arm/tee/ffa_msg.c b/xen/arch/arm/tee/ffa_msg.c
>> index ae263e54890e..335f246ba657 100644
>> --- a/xen/arch/arm/tee/ffa_msg.c
>> +++ b/xen/arch/arm/tee/ffa_msg.c
>> @@ -12,6 +12,15 @@
>>
>> #include "ffa_private.h"
>>
>> +/* Encoding of partition message in RX/TX buffer */
>> +struct ffa_part_msg_rxtx {
>> + uint32_t flags;
>> + uint32_t reserved;
>> + uint32_t msg_offset;
>> + uint32_t send_recv_id;
>> + uint32_t msg_size;
>> +};
>> +
>> void ffa_handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
>> {
>> struct arm_smccc_1_2_regs arg = { .a0 = fid, };
>> @@ -78,3 +87,43 @@ out:
>> resp.a4 & mask, resp.a5 & mask, resp.a6 & mask,
>> resp.a7 & mask);
>> }
>> +
>> +int32_t ffa_handle_msg_send2(struct cpu_user_regs *regs)
>> +{
>> + struct domain *src_d = current->domain;
>> + struct ffa_ctx *src_ctx = src_d->arch.tee;
>> + const struct ffa_part_msg_rxtx *src_msg;
>> + uint16_t dst_id, src_id;
>> + int32_t ret;
>> +
>> + if ( !ffa_fw_supports_fid(FFA_MSG_SEND2) )
>> + return FFA_RET_NOT_SUPPORTED;
>> +
>> + if ( !spin_trylock(&src_ctx->tx_lock) )
>> + return FFA_RET_BUSY;
>> +
>> + src_msg = src_ctx->tx;
>> + src_id = src_msg->send_recv_id >> 16;
>> + dst_id = src_msg->send_recv_id & GENMASK(15,0);
>> +
>> + if ( src_id != ffa_get_vm_id(src_d) || !FFA_ID_IS_SECURE(dst_id) )
>> + {
>> + ret = FFA_RET_INVALID_PARAMETERS;
>> + goto out_unlock_tx;
>> + }
>> +
>> + /* check source message fits in buffer */
>> + if ( src_ctx->page_count * FFA_PAGE_SIZE <
>> + src_msg->msg_offset + src_msg->msg_size ||
>> + src_msg->msg_offset < sizeof(struct ffa_part_msg_rxtx) )
>> + {
>> + ret = FFA_RET_INVALID_PARAMETERS;
>> + goto out_unlock_tx;
>> + }
>
> The guest can change src_mst at any moment with another CPU so these
> tests are only sanity checks. The SPMC will also have to lock and do
> the same tests again. So the tests here will only in the best case (in
> case the guest is misbehaving) save us from entering the SPMC only to
> get an error back. The lock makes sense since we could have concurrent
> calls to FFA_MEM_SHARE. How about removing the tests?
I think we should still prevent to forward invalid requests to the SPMC as
much as we can to prevent a malicious guest from stilling CPU cycles by
doing invalid calls to the secure world.
I could put a comment in there saying that this is just protection but to be
fare the SPMC in secure will have the same issues: this can be changed
at any time by the caller on another core.
>
>> +
>> + ret = ffa_simple_call(FFA_MSG_SEND2, ((uint32_t)src_id) << 16, 0, 0, 0);
>
> I'd rather use ffa_get_vm_id(src_d) instead of src_id.
src_id is a local variable and was checked to be equal to ffa_get_vm_id(src_d)
upper so those 2 values are the same.
Why would you rather recall ffa_get_vm_id here ?
Cheers
Bertrand
>
> Cheers,
> Jens
>
>> +
>> +out_unlock_tx:
>> + spin_unlock(&src_ctx->tx_lock);
>> + return ret;
>> +}
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index 973ee55be09b..d441c0ca5598 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -359,6 +359,7 @@ void ffa_handle_notification_get(struct cpu_user_regs
>> *regs);
>> int ffa_handle_notification_set(struct cpu_user_regs *regs);
>>
>> void ffa_handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t
>> fid);
>> +int32_t ffa_handle_msg_send2(struct cpu_user_regs *regs);
>>
>> static inline uint16_t ffa_get_vm_id(const struct domain *d)
>> {
>> --
>> 2.47.0
