[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


  • To: Julien Grall <julien.grall@xxxxxxx>
  • From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Date: Wed, 11 Sep 2019 18:48:39 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=DCNp7nmmnTGkbvBfGGeLqYbDTghMkRrAfai0syFg6c8=; b=ZRLlLfpVm9tC8cvEy8agUuSMIhGJN4ui61wTNiYXYaLXm4bxkFfpbZRKWtySXpgHxbNztZDqsnKjrkdxcEbt0gJEhzqIGisjHn9B6GELK2K+xpK16LV00zxBBJn2ulAubKDL5ZrRf+2SW5ZPo3Y8hxxiOpVUsy8vBGowCyacCmmakABWdzcGZOc4zMpkwBzlfoMoFCvtBgMbOEPdyp1G5nAw9cXjDNMJ9GN7ugyLW4Jwx4vklkQbSj05oCvrHU5liC2Zr8MBs3tzPIAvgObXUt4qryh6F9S8mGsXiwHkVB0ipKvC1MyFtOh4V0dJ2/O2NOiqjj1wO8ZK1pai+XcYhg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jBHbAzUHf1b+HlBcqqrpxoj7NEXrc1md8WA3c6hwrm1w0N6OwFwIrChc5M7lFgU5J7vXmWQEfcXUCsbnx6JMgfWrZbWCOACKvOyFY8Uw9xS70lszQUcqDbK1+3LvE8eaobs9uYDRStUq6QqJ9+UD0vA2DOsITNSe8SBTTFxOwbWLm4egluAC5WgXLvc3AvR1nwRmf5y4gF/CEzEv0RQTM5SJe7qlLH8mmVHay8Z3Ssoy5Uy9WJ53Rs+a0N3dWn6x1JwdqmKs6ONtS8UcGuz8nyUZYPREOswZpjzOiSv9AZBfY59Tl60eKoveEYMlUDZsyS9vE01Beb8DVbq9LSnjSw==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Volodymyr_Babchuk@xxxxxxxx;
  • Cc: "tee-dev@xxxxxxxxxxxxxxxx" <tee-dev@xxxxxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 11 Sep 2019 18:48:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVWeNlCY0CqDi7zE2giEuaPhvOCqckA0sAgALr+QA=
  • Thread-topic: [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

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.

>
>> 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 )
>>
>
> Cheers,


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
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®.