Add support for FFA_MSG_SEND2 to send indirect messages from a VM to a
secure partition.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
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;
+    }
+
+    ret = ffa_simple_call(FFA_MSG_SEND2, ((uint32_t)src_id) << 16, 0, 0, 0);
+
+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

Hi Bertrand,

On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@arm.com> 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@arm.com>
> ---
> 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?

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

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
>

Hi Jens,

> On 24 Oct 2024, at 10:50, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <bertrand.marquis@arm.com> 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@arm.com>
>> ---
>> 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

Hi Bertrand,

On Thu, Oct 24, 2024 at 12:05 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 24 Oct 2024, at 10:50, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> > <bertrand.marquis@arm.com> 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@arm.com>
> >> ---
> >> 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
>
>

Hi Jens,

> On 24 Oct 2024, at 15:43, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi Bertrand,
> 
> On Thu, Oct 24, 2024 at 12:05 PM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>> 
>> Hi Jens,
>> 
>>> On 24 Oct 2024, at 10:50, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
>>> <bertrand.marquis@arm.com> 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@arm.com>
>>>> ---
>>>> 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