[xen master] xen/arm: ffa: Rework partinfo_get implementation

[patchbot@xxxxxxx]

commit 36dd9a7a42bd524c1587e45e1c39d9cd24f4322d
Author:     Bertrand Marquis <bertrand.marquis@xxxxxxx>
AuthorDate: Mon Aug 11 15:34:58 2025 +0200
Commit:     Julien Grall <jgrall@xxxxxxxxxx>
CommitDate: Tue Aug 12 12:04:40 2025 +0100

    xen/arm: ffa: Rework partinfo_get implementation
    
    This patch is in preparation for VM to VM support in order to do the
    changes on the SP handling part of partinfo_get before adding support
    for the VM part.
    
    This patches is doing the following changes:
    - split partinfo_get into 3 functions to have the locking handling and
      proper exit on error handled more clearly
    - add some potential overflow checks to validate the offset and sizes
      passed by the VM on partinfo call.
    - Introduce a maximum number of SPs (for now set to 64) to prevent
      holding the CPU for too long in case there would be a lot of
      partitions in the secure world. The limit currently set is thought to
      be realistic for most use cases as 64 secure partitions is a very high
      number compared to current seen usage (more 3 or 4).
    - fix include ordering in ffa_private.h to be in alphabetic order
    
    Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
    Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
    Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
---
 xen/arch/arm/tee/ffa_partinfo.c | 202 +++++++++++++++++++++++-----------------
 xen/arch/arm/tee/ffa_private.h  |  18 +++-
 2 files changed, 132 insertions(+), 88 deletions(-)

diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
index c0510ceb83..dfa0b23eaf 100644
--- a/xen/arch/arm/tee/ffa_partinfo.c
+++ b/xen/arch/arm/tee/ffa_partinfo.c
@@ -63,9 +63,96 @@ static int32_t ffa_partition_info_get(uint32_t *uuid, 
uint32_t flags,
     return ret;
 }
 
-void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
+static int32_t ffa_get_sp_count(uint32_t *uuid, uint32_t *sp_count)
+{
+    uint32_t src_size;
+
+    return ffa_partition_info_get(uuid, FFA_PARTITION_INFO_GET_COUNT_FLAG,
+                                  sp_count, &src_size);
+}
+
+static int32_t ffa_get_sp_partinfo(uint32_t *uuid, uint32_t *sp_count,
+                                   void *dst_buf, void *end_buf,
+                                   uint32_t dst_size)
 {
     int32_t ret;
+    uint32_t src_size, real_sp_count;
+    void *src_buf = ffa_rx;
+    uint32_t count = 0;
+
+    /* Do we have a RX buffer with the SPMC */
+    if ( !ffa_rx )
+        return FFA_RET_DENIED;
+
+    /* We need to use the RX buffer to receive the list */
+    spin_lock(&ffa_rx_buffer_lock);
+
+    ret = ffa_partition_info_get(uuid, 0, &real_sp_count, &src_size);
+    if ( ret )
+        goto out;
+
+    /* We now own the RX buffer */
+
+    /* Validate the src_size we got */
+    if ( src_size < sizeof(struct ffa_partition_info_1_0) ||
+         src_size >= FFA_PAGE_SIZE )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_release;
+    }
+
+    /*
+     * Limit the maximum time we hold the CPU by limiting the number of SPs.
+     * We just ignore the extra ones as this is tested during init in
+     * ffa_partinfo_init so the only possible reason is SP have been added
+     * since boot.
+     */
+    if ( real_sp_count > FFA_MAX_NUM_SP )
+        real_sp_count = FFA_MAX_NUM_SP;
+
+    /* Make sure the data fits in our buffer */
+    if ( real_sp_count > (FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE) / src_size )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_release;
+    }
+
+    for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ )
+    {
+        struct ffa_partition_info_1_1 *fpi = src_buf;
+
+        /* filter out SP not following bit 15 convention if any */
+        if ( FFA_ID_IS_SECURE(fpi->id) )
+        {
+            if ( dst_buf > (end_buf - dst_size) )
+            {
+                ret = FFA_RET_NO_MEMORY;
+                goto out_release;
+            }
+
+            memcpy(dst_buf, src_buf, MIN(src_size, dst_size));
+            if ( dst_size > src_size )
+                memset(dst_buf + src_size, 0, dst_size - src_size);
+
+            dst_buf += dst_size;
+            count++;
+        }
+
+        src_buf += src_size;
+    }
+
+    *sp_count = count;
+
+out_release:
+    ffa_hyp_rx_release();
+out:
+    spin_unlock(&ffa_rx_buffer_lock);
+    return ret;
+}
+
+void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
+{
+    int32_t ret = FFA_RET_OK;
     struct domain *d = current->domain;
     struct ffa_ctx *ctx = d->arch.tee;
     uint32_t flags = get_user_reg(regs, 5);
@@ -75,8 +162,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs 
*regs)
         get_user_reg(regs, 3),
         get_user_reg(regs, 4),
     };
-    uint32_t src_size, dst_size;
-    void *dst_buf;
+    uint32_t dst_size = 0;
+    void *dst_buf, *end_buf;
     uint32_t ffa_sp_count = 0;
 
     /*
@@ -89,31 +176,26 @@ void ffa_handle_partition_info_get(struct cpu_user_regs 
*regs)
     else
         dst_size = sizeof(struct ffa_partition_info_1_1);
 
-    /*
-     * FF-A v1.0 has w5 MBZ while v1.1 allows
-     * FFA_PARTITION_INFO_GET_COUNT_FLAG to be non-zero.
-     *
-     * FFA_PARTITION_INFO_GET_COUNT is only using registers and not the
-     * rxtx buffer so do the partition_info_get directly.
-     */
-    if ( flags == FFA_PARTITION_INFO_GET_COUNT_FLAG &&
-         ctx->guest_vers == FFA_VERSION_1_1 )
+    /* Only count requested */
+    if ( flags )
     {
+        /*
+         * FF-A v1.0 has w5 MBZ while v1.1 allows
+         * FFA_PARTITION_INFO_GET_COUNT_FLAG to be non-zero.
+         */
+        if ( ctx->guest_vers == FFA_VERSION_1_0 ||
+                flags != FFA_PARTITION_INFO_GET_COUNT_FLAG )
+        {
+            ret = FFA_RET_INVALID_PARAMETERS;
+            goto out;
+        }
+
         if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
-            ret = ffa_partition_info_get(uuid, flags, &ffa_sp_count,
-                                        &src_size);
-        else
-            ret = FFA_RET_OK;
+            ret = ffa_get_sp_count(uuid, &ffa_sp_count);
 
         goto out;
     }
 
-    if ( flags )
-    {
-        ret = FFA_RET_INVALID_PARAMETERS;
-        goto out;
-    }
-
     if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
     {
         /* Just give an empty partition list to the caller */
@@ -121,80 +203,33 @@ void ffa_handle_partition_info_get(struct cpu_user_regs 
*regs)
         goto out;
     }
 
+    /* Get the RX buffer to write the list of partitions */
     ret = ffa_rx_acquire(d);
     if ( ret != FFA_RET_OK )
         goto out;
 
     dst_buf = ctx->rx;
+    end_buf = ctx->rx + ctx->page_count * FFA_PAGE_SIZE;
 
-    if ( !ffa_rx )
-    {
-        ret = FFA_RET_DENIED;
-        goto out_rx_release;
-    }
-
-    spin_lock(&ffa_rx_buffer_lock);
-
-    ret = ffa_partition_info_get(uuid, 0, &ffa_sp_count, &src_size);
-
-    if ( ret )
-        goto out_rx_hyp_unlock;
+    /* An entry should be smaller than a page */
+    BUILD_BUG_ON(sizeof(struct ffa_partition_info_1_1) > FFA_PAGE_SIZE);
 
     /*
-     * ffa_partition_info_get() succeeded so we now own the RX buffer we
-     * share with the SPMC. We must give it back using ffa_hyp_rx_release()
-     * once we've copied the content.
+     * Check for overflow and that we can at least store one entry.
+     * page_count cannot be 0 so we have at least one page.
      */
-
-    /* we cannot have a size smaller than 1.0 structure */
-    if ( src_size < sizeof(struct ffa_partition_info_1_0) )
-    {
-        ret = FFA_RET_NOT_SUPPORTED;
-        goto out_rx_hyp_release;
-    }
-
-    if ( ctx->page_count * FFA_PAGE_SIZE < ffa_sp_count * dst_size )
+    if ( dst_buf >= end_buf || dst_buf > (end_buf - dst_size) )
     {
-        ret = FFA_RET_NO_MEMORY;
-        goto out_rx_hyp_release;
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out_rx_release;
     }
 
-    if ( ffa_sp_count > 0 )
-    {
-        uint32_t n, n_limit = ffa_sp_count;
-        void *src_buf = ffa_rx;
-
-        /* copy the secure partitions info */
-        for ( n = 0; n < n_limit; n++ )
-        {
-            struct ffa_partition_info_1_1 *fpi = src_buf;
-
-            /* filter out SP not following bit 15 convention if any */
-            if ( FFA_ID_IS_SECURE(fpi->id) )
-            {
-                memcpy(dst_buf, src_buf, dst_size);
-                dst_buf += dst_size;
-            }
-            else
-                ffa_sp_count--;
+    ret = ffa_get_sp_partinfo(uuid, &ffa_sp_count, dst_buf, end_buf,
+                              dst_size);
 
-            src_buf += src_size;
-        }
-    }
 
-out_rx_hyp_release:
-    ffa_hyp_rx_release();
-out_rx_hyp_unlock:
-    spin_unlock(&ffa_rx_buffer_lock);
 out_rx_release:
-    /*
-     * The calling VM RX buffer only contains data to be used by the VM if the
-     * call was successful, in which case the VM has to release the buffer
-     * once it has used the data.
-     * If something went wrong during the call, we have to release the RX
-     * buffer back to the SPMC as the VM will not do it.
-     */
-    if ( ret != FFA_RET_OK )
+    if ( ret )
         ffa_rx_release(d);
 out:
     if ( ret )
@@ -353,9 +388,10 @@ bool ffa_partinfo_init(void)
         goto out;
     }
 
-    if ( count >= UINT16_MAX )
+    if ( count >= FFA_MAX_NUM_SP )
     {
-        printk(XENLOG_ERR "ffa: Impossible number of SPs: %u\n", count);
+        printk(XENLOG_ERR "ffa: More SPs than the maximum supported: %u - 
%u\n",
+               count, FFA_MAX_NUM_SP);
         goto out;
     }
 
diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
index c4cd655389..0a9c1082db 100644
--- a/xen/arch/arm/tee/ffa_private.h
+++ b/xen/arch/arm/tee/ffa_private.h
@@ -6,15 +6,15 @@
 #ifndef __FFA_PRIVATE_H__
 #define __FFA_PRIVATE_H__
 
+#include <xen/bitmap.h>
 #include <xen/const.h>
-#include <xen/sizes.h>
-#include <xen/types.h>
-#include <xen/mm.h>
 #include <xen/list.h>
-#include <xen/spinlock.h>
+#include <xen/mm.h>
 #include <xen/sched.h>
+#include <xen/sizes.h>
+#include <xen/spinlock.h>
 #include <xen/time.h>
-#include <xen/bitmap.h>
+#include <xen/types.h>
 
 /* Error codes */
 #define FFA_RET_OK                      0
@@ -108,6 +108,14 @@
  */
 #define FFA_CTX_TEARDOWN_DELAY          SECONDS(1)
 
+/*
+ * The maximum number of Secure partitions we support for partinfo_get.
+ * This prevents holding the CPU during potentially to long time during
+ * a partinfo_get call. Value choosen seems realistic for any configuration
+ * but can be incremented here if needed.
+ */
+#define FFA_MAX_NUM_SP                  64
+
 /*
  * We rely on the convention suggested but not mandated by the FF-A
  * specification that secure world endpoint identifiers have the bit 15
--
generated by git-patchbot for /home/xen/git/xen.git#master