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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

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().

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
>
>