Re: [PATCH 5/9] dma-buf: heaps: mtk_sec_heap: Initialise tee session

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 28/09/2023 à 19:48, Jeffrey Kardatzke a écrit :
> On Thu, Sep 28, 2023 at 1:30 AM Benjamin Gaignard
> <benjamin.gaignard@collabora.com> wrote:
>>
>> Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit :
>>> On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard
>>> <benjamin.gaignard@collabora.com> wrote:
>>>> Le 27/09/2023 à 15:46, Joakim Bech a écrit :
>>>>> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
>>>>>> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
>>>>>>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
>>>>>>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
>>>>>>>> wrote:
>>>>>>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
>>>>>>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
>>>>>>>>>> work
>>>>>>>>>> here since this is not a platform driver, therefore initialise
>>>>>>>>>> the
>>>>>>>>>> TEE
>>>>>>>>>> context/session while we allocate the first secure buffer.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/dma-buf/heaps/mtk_secure_heap.c | 61
>>>>>>>>>> +++++++++++++++++++++++++
>>>>>>>>>>       1 file changed, 61 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>>>> b/drivers/dma-
>>>>>>>>>> buf/heaps/mtk_secure_heap.c
>>>>>>>>>> index bbf1c8dce23e..e3da33a3d083 100644
>>>>>>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>>>> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>>>> @@ -10,6 +10,12 @@
>>>>>>>>>>       #include <linux/err.h>
>>>>>>>>>>       #include <linux/module.h>
>>>>>>>>>>       #include <linux/slab.h>
>>>>>>>>>> +#include <linux/tee_drv.h>
>>>>>>>>>> +#include <linux/uuid.h>
>>>>>>>>>> +
>>>>>>>>>> +#define TZ_TA_MEM_UUID          "4477588a-8476-11e2-ad15-
>>>>>>>>>> e41f1390d676"
>>>>>>>>>> +
>>>>>>>>> Is this UUID the same for all SoCs and all TZ versions?
>>>>>>>> Yes. It is the same for all SoCs and all TZ versions currently.
>>>>>>>>
>>>>>>> That's good news!
>>>>>>>
>>>>>>> Is this UUID used in any userspace component? (example: Android
>>>>>>> HALs?)
>>>>>> No. Userspace never use it. If userspace would like to allocate this
>>>>>> secure buffer, it can achieve through the existing dmabuf IOCTL via
>>>>>> /dev/dma_heap/mtk_svp node.
>>>>>>
>>>>> In general I think as mentioned elsewhere in comments, that there isn't
>>>>> that much here that seems to be unique for MediaTek in this patch
>>>>> series, so I think it worth to see whether this whole patch set can be
>>>>> made more generic. Having said that, the UUID is always unique for a
>>>>> certain Trusted Application. So, it's not entirely true saying that the
>>>>> UUID is the same for all SoCs and all TrustZone versions. It might be
>>>>> true for a family of MediaTek devices and the TEE in use, but not
>>>>> generically.
>>>>>
>>>>> So, if we need to differentiate between different TA implementations,
>>>>> then we need different UUIDs. If it would be possible to make this patch
>>>>> set generic, then it sounds like a single UUID would be sufficient, but
>>>>> that would imply that all TA's supporting such a generic UUID would be
>>>>> implemented the same from an API point of view. Which also means that
>>>>> for example Trusted Application function ID's needs to be the same etc.
>>>>> Not impossible to achieve, but still not easy (different TEE follows
>>>>> different specifications) and it's not typically something we've done in
>>>>> the past.
>>>>>
>>>>> Unfortunately there is no standardized database of TA's describing what
>>>>> they implement and support.
>>>>>
>>>>> As an alternative, we could implement a query call in the TEE answering,
>>>>> "What UUID does your TA have that implements secure unmapped heap?".
>>>>> I.e., something that reminds of a lookup table. Then we wouldn't have to
>>>>> carry this in UAPI, DT or anywhere else.
>>>> Joakim does a TA could offer a generic API and hide the hardware specific
>>>> details (like kernel uAPI does for drivers) ?
>>> It would have to go through another layer (like the tee driver) to be
>>> a generic API. The main issue with TAs is that they have UUIDs you
>>> need to connect to and specific codes for each function; so we should
>>> abstract at a layer above where those exist in the dma-heap code.
>>>> Aside that question I wonder what are the needs to perform a 'secure' playback.
>>>> I have in mind 2 requirements:
>>>> - secure memory regions, which means configure the hardware to ensure that only
>>>> dedicated hardware blocks and read or write into it.
>>>> - set hardware blocks in secure modes so they access to secure memory.
>>>> Do you see something else ?
>>> This is more or less what is required, but this is out of scope for
>>> the Linux kernel since it can't be trusted to do these things...this
>>> is all done in firmware or the TEE itself.
>> Yes kernel can't be trusted to do these things but know what we need could help
>> to define a API for a generic TA.
>>
>> Just to brainstorm on mailing list:
>> What about a TA API like
>> TA_secure_memory_region() and TA_unsecure_memory_region() with parameters like:
>> - device identifier (an ID or compatible string maybe)
>> - memory region (physical address, size, offset)
>> - requested access rights (read, write)
>>
>> and on kernel side a IOMMU driver because it basically have all this information already
>> (device attachment, kernel map/unmap).
>>
>> In my mind it sound like a solution to limit the impact (new controls, new memory type)
>> inside v4l2. Probably we won't need new heap either.
>> All hardware dedicated implementations could live inside the TA which can offer a generic
>> API.
> The main problem with that type of design is the limitations of
> TrustZone memory protection. Usually there is a limit to the number of
> regions you can define for memory protection (and there is on
> Mediatek). So you can't pass an arbitrary memory region and mark it
> protected/unprotected at a given time. You need to establish these
> regions in the firmware instead and then configure those regions for
> protection in the firmware or the TEE.

The TEE iommu could be aware of these limitations and merge the regions when possible
plus we can define a CMA region for each device to limit the secured memory fragmentation.

>
>>>> Regards,
>>>> Benjamin
>>>>