Message-ID: <20220817135723.rf567ocaba2k5smf@000377403353>
Date:   Wed, 17 Aug 2022 14:57:23 +0100
From:   Brian Starkey <brian.starkey@....com>
To:     Olivier Masse <olivier.masse@....com>
Cc:     "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
        "christian.koenig@....com" <christian.koenig@....com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "nd@....com" <nd@....com>,
        Clément Faure <clement.faure@....com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "benjamin.gaignard@...labora.com" <benjamin.gaignard@...labora.com>
Subject: Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf
 heap support

On Tue, Aug 16, 2022 at 11:20:50AM +0000, Olivier Masse wrote:
> On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote:
> > On Mon, Aug 08, 2022 at 02:39:53PM +0000, Olivier Masse wrote:
> > > On ven., 2022-08-05 at 16:41 +0100, Brian Starkey wrote:
> > > > On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:

.. snip

> > > > > +
> > > > > +static struct sg_table *secure_heap_map_dma_buf(struct
> > > > > dma_buf_attachment *attachment,
> > > > > +                                             enum
> > > > > dma_data_direction direction)
> > > > > +{
> > > > > +     struct secure_heap_attachment *a = attachment->priv;
> > > > > +
> > > > > +     return a->table;
> > > > 
> > > > I think you still need to implement mapping and unmapping using
> > > > the
> > > > DMA APIs. For example devices might be behind IOMMUs and the
> > > > buffer
> > > > will need mapping into the IOMMU.
> > > 
> > > Devices that will need access to the buffer must be in secure.
> > > The tee driver will only need the scatter-list table to get dma
> > > address
> > > and len. Mapping will be done in the TEE.
> > > Please find tee_shm_register_fd in the following commit
> > > 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux%2Fcommit%2F41e21e5c405530590dc2dd10b2a8dbe64589840f&amp;data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=OKZhaNevD5dj7Wjm6zbZlij0mPA9XYyio1NAN3VjTVM%3D&amp;reserved=0
> > > 
> > > This patch need to be up-streamed as well.
> > > 
> > 
> > Interesting, that's not how the devices I've worked on operated.
> > 
> > Are you saying that you have to have a display controller driver
> > running in the TEE to display one of these buffers?
> 
> In fact the display controller is managing 3 plans : UI, PiP and
> video. The video plan is protected in secure as you can see on slide
> 11:
> https://static.linaro.org/connect/san19/presentations/san19-107.pdf
> 
> The DCSS (display controller) is able to read from the protected secure
> heap and composition result is send directly to the HDMI/HDCP port.

But it sounds like the DCSS driver is running in non-trusted Linux?

> 
> 
> >  If everything
> > needs to be in the TEE, then why even have these buffers allocated
> > by non-secure Linux at all?
> 
> The TEE is only doing decryption using the HW Crypto Accelerator
> (CAAM).
> The CAAM will read from a non protected encrypted buffer to write clear
> content to a secure buffer allocated with DMABUF and mapped in secure
> by OPTEE OS.
> 
> > 
> > I would have expected there to be HW enforcement of buffer access,
> > but for the display driver to be in non-secure Linux. That's how
> > TZMP1 works: 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fhkg18%2Fpresentations%2Fhkg18-408.pdf&amp;data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=XVpI93dXYu%2BGswLE8dcYboq%2FAWzSJn9j9LMlngpr238%3D&amp;reserved=0
> > 
> > Looking at this SDP presentation, that also seems to be the case
> > there: 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstatic.linaro.org%2Fconnect%2Fsan19%2Fpresentations%2Fsan19-107.pdf&amp;data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=5Ec61NC1f0UQU%2F3BEURZQhEBrZ%2FuvJ1vaoSN4ChMn%2Bw%3D&amp;reserved=0
> > 
> 
> Indeed, TZMP1 is similar to our implementation.
> 
> > Based on those presentations, I think this heap should be
> > implementing
> > map_dma_buf in the "normal" way, using the DMA API to map the buffer
> > to the device. It's up to the TEE and HW firewall to prevent access
> > to those mappings from non-secure devices.
> 
> In fact, our devices (VPU and DCSS) do not need any mapping, but only
> the physical address of buffers which need to be contiguous.

That's not how dma-buf or the DMA APIs work though - you should use
dma_map_sgtable and let the DMA API take care of whether a mapping
is needed or not.

> The VPU decoder, run by the CPU, read video meta data from a non
> protected buffer and send physical memory address of encoded buffer to
> the VPU HW.
> As well, the DCSS get physical address of contiguous decoded video
> buffer to do the composition.
> 

Can you share the DCSS side of this? Maybe that will help to clear it
up.

Thanks,
-Brian

> > 
> > My understanding is:
> > 
> > * The memory region should never be mapped or accessed from the host
> >   CPU. This is not a security requirement - the CPU will be denied
> >   access by whatever hardware is enforcing security - but any CPU
> >   accesses will fail, so there is no point in ever having a CPU
> >   mapping.
> 
> agree with that.
> 
> > * The allocated buffers _should_ be mapped to devices via
> > map_dma_buf.
> >   Again the HW enforcement will prevent access from devices which
> >   aren't permitted access, but for example a display controller
> >   may be allowed to read the secure buffer, composite it with other
> >   buffers, and display it on the screen.
> 
> yes, in could be done for a more generic implementation.
> 
> > 
> > Am I wrong? Even if SDP doesn't work this way, I think we should make
> > the heap as generic as possible so that it can work with different
> > secure video implementations.
> > 
> > > 
> > > > 
> > 
> > .. snip
> 
> alright, I get your point
> 
> > 
> > > > > +
> > > > > +RESERVEDMEM_OF_DECLARE(secure_heap, "linaro,secure-heap",
> > > > > rmem_secure_heap_setup);
> > > > 
> > > > Is there anything linaro-specific about this? Could it be
> > > > linux,secure-heap?
> > > 
> > > for now, it's specific to Linaro OPTEE OS.
> > > but in a more generic way, it could be
> > > linux,unmapped-heap ?
> > 
> > If these buffers can never be mapped, not to the CPU nor to devices,
> > then actually I don't see why it should be a dma-buf heap at all.
> > 
> > If this is just an interface to associate some identifier (in this
> > case an fd) with a region of physical address space, then why is it
> > useful to pretend that it's a dma-buf, if none of the dma-buf
> > operations actually do anything?
> 
> in our previous implementation, we were using unmapped ION buffer to be
> able to send an opaque fd to the TEE driver which could then be mapped
> in secure by OPTEE.
> Transitioning from ION to DMABUF heaps, our retaining option was to
> create a new heap type.
> 
> 
> Best regards,
> Olivier
> 

Powered by blists - more mailing lists