Hi Jens,
> On 24 Oct 2024, at 15:43, Jens Wiklander <[email protected]> wrote:
>
> Hi Bertrand,
>
> On Thu, Oct 24, 2024 at 12:05 PM Bertrand Marquis
> <[email protected]> wrote:
>>
>> 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.
>
> 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
