[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size



Hi Volodymyr,

On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:

Julien Grall writes:

Hi Volodymyr,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
We want to limit number of calls to lookup_and_pin_guest_ram_addr()
per one request. There are two ways to do this: either preempt
translate_noncontig() or to limit size of one shared buffer size.

It is quite hard to preempt translate_noncontig(), because it is deep
nested. So we chose second option. We will allow 512 pages per one
shared buffer. This does not interfere with GP standard, as it
requires that size limit for shared buffer should be at lest 512kB.

Do you mean "least" instead of "lest"?
Yes

If so, why 512 pages (i.e 1MB)
is plenty enough for most of the use cases? What does "xtest" consist
on?
Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
is enough for the most cases, because OP-TEE itself have a very limited
resources. But this value is chosen arbitrary.

Could we potentially reduce to let say 512KB (or maybe lower) if xtest only allocate 32KB?



Also, with this limitation OP-TEE still passes own "xtest" test suite,
so this is okay for now.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
---
   xen/arch/arm/tee/optee.c | 30 ++++++++++++++++++------------
   1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index ec5402e89b..f4fa8a7758 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -72,6 +72,17 @@
    */
   #define MAX_TOTAL_SMH_BUF_PG    16384
   +/*
+ * Arbitrary value that limits maximum shared buffer size. It is
+ * merely coincidence that it equals to both default OP-TEE SHM buffer
+ * size limit and to (1 << CONFIG_DOMU_MAX_ORDER). Please note that
+ * this define limits number of pages. But user buffer can be not
+ * aligned to a page boundary. So it is possible that user would not
+ * be able to share exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with
+ * OP-TEE.
+ */
+#define MAX_SHM_BUFFER_PG       512
+
   #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR
   #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \
                                 OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
@@ -697,15 +708,17 @@ static int translate_noncontig(struct optee_domain *ctx,
       size = ROUNDUP(param->u.tmem.size + offset, 
OPTEE_MSG_NONCONTIG_PAGE_SIZE);
         pg_count = DIV_ROUND_UP(size,
OPTEE_MSG_NONCONTIG_PAGE_SIZE);
+    if ( pg_count > MAX_SHM_BUFFER_PG )
+        return -ENOMEM;
+
       order = get_order_from_bytes(get_pages_list_size(pg_count));
         /*
-     * In the worst case we will want to allocate 33 pages, which is
-     * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at
-     * most 64 pages allocated. This buffer will be freed right after
-     * the end of the call and there can be no more than
+     * In the worst case we will want to allocate 2 pages, which is
+     * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed
+     * right after the end of the call and there can be no more than
        * max_optee_threads calls simultaneously. So in the worst case
-     * guest can trick us to allocate 64 * max_optee_threads pages in
+     * guest can trick us to allocate 2 * max_optee_threads pages in
        * total.
        */
       xen_pgs = alloc_domheap_pages(current->domain, order, 0);
@@ -747,13 +760,6 @@ static int translate_noncontig(struct optee_domain *ctx,
               xen_data = __map_domain_page(xen_pgs);
           }
   -        /*
-         * TODO: That function can pin up to 64MB of guest memory by
-         * calling lookup_and_pin_guest_ram_addr() 16384 times
-         * (assuming that PAGE_SIZE equals to 4096).
-         * This should be addressed before declaring OP-TEE security
-         * supported.
-         */
           BUILD_BUG_ON(PAGE_SIZE != 4096);

Without the comment, the BUILD_BUG_ON() looks random. So either you
want to have a different version of the comment or you want to move
the BUILD_BUG_ON() to somewhere else.

It is still before get_domain_ram_page() call. But for clarity I can add
comment like "Only 4k pages are supported right now".
           page = 
get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
           if ( !page )

That would be useful.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.