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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 24 Oct 2024, at 15:43, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Thu, Oct 24, 2024 at 12:05 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> 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.
> 
> Fair enough.
> 
>> 
>>> 
>>>> +
>>>> +    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 ?
> 
> I don't think that check is enough to prevent the compiler from
> loading that value from memory again, potentially opening a
> time-of-check to time-of-use window. Using ACCESS_ONCE() when reading
> send_recv_id above should also take care of that, but it seems more
> direct to use ffa_get_vm_id().

Ok I will use ffa_get_vm_id in v3.

Thanks a lot for the review.

Cheers
Bertrand

> 
> Cheers,
> Jens
> 
>> 
>> 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