Re: [PATCH v2 10/10] xen/arm: ffa: Add indirect message support

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 24 Oct 2024, at 10:50, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> wrote:
>> 
>> Add support for FFA_MSG_SEND2 to send indirect messages from a VM to a
>> secure partition.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> 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