Re: [PATCH v1 4/6] optee: add OP-TEE mediator

From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
To: Julien Grall <julien.grall@arm.com>,
	xen-devel@lists.xen.org,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Tim Deegan <tim@xen.org>, Wei Liu <wei.liu2@citrix.com>,
	Jens Wiklander <jens.wiklander@linaro.org>
Cc: tee-dev@lists.linaro.org
Subject: Re: [PATCH v1 4/6] optee: add OP-TEE mediator
Date: Thu, 23 Aug 2018 17:27:52 +0300	[thread overview]
Message-ID: <926a6612-d514-0f27-4710-d8c6191ab672@epam.com> (raw)
In-Reply-To: <15ce9921-3541-a89a-8257-1bc4a2c83386@arm.com>

Hi,

On 22.08.18 20:28, Julien Grall wrote:
> Hi,
> 
> Please only CC relevant people to the patches. This could be done easily 
> using the new script add_maintainers.pl.
> 
Oh, I'm sorry. I used get_maintainers.pl.

> On 22/08/18 15:11, Volodymyr Babchuk wrote:
>> Add OP-TEE mediator, so guests can access OP-TEE services.
>>
>> OP-TEE mediator support address translation for DomUs.
>> It tracks execution of STD calls, correctly handles memory-related RPC
>> requests, tracks buffer allocated for RPCs.
>>
>> With this patch OP-TEE sucessfully passes own tests, while client is
>> running in DomU.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>
>>   Changes from "RFC":
>>   - Removed special case for Dom0/HwDOM
>>   - No more support for plain OP-TEE (only OP-TEE with virtualization
>>     config enabled is supported)
>>   - Multiple domains is now supported
>>   - Pages that are shared between OP-TEE and domain are now pinned
>>   - Renamed CONFIG_ARM_OPTEE to CONFIG_OPTEE
>>   - Command buffers from domain are now shadowed by XEN
>>   - Mediator now filters out unknown capabilities and requests
>>   - call contexts, shared memory object now stored per-domain
>>
>>   xen/arch/arm/tee/Kconfig            |   4 +
>>   xen/arch/arm/tee/Makefile           |   1 +
>>   xen/arch/arm/tee/optee.c            | 972 
>> ++++++++++++++++++++++++++++++++++++
> 
> This patch is far to big to get a proper review with understanding of 
> the code. Can you split it in smaller ones with appropriate commit message?
> 
Yes, it is a quite big. But this is a complete feature. I can't remove 
anything from it, because it will not work.
I can split it into series of patches, that will add various pieces of 
code... But this will lead to patches with not-working code until the 
final one. Is this okay?

>  From a quick look at it, I would like to understand how the memory 
> allocated in Xen is bounded for a given guest? Same question for the time.

I store references to allocated pages in per-domain context. But they 
are not accounted as a domain memory. This pages are needed by XEN to 
conceal real PAs from guest. I'm not sure it they should be accounted as 
a memory allocated by domain.

And what about a time? Did you mean time accounting?

> I am interested in a normal case but also in the case where someone 
> malicious is using that API. How much damage can it do to the hypervisor?

Every standard (long-lasting) call requires small amount of memory to 
store context. Every shared buffer requires enough memory to store 
references to shared pages.
OP-TEE has limited resources, so it will not allow you to create, say, 
100 calls and couple of GBs of shared memory. I expect that it will 
limit caller in memory overuse.

Apart from that I can't imagine how malicious user can damage the 
hypervisor.

-- 
Volodymyr Babchuk

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