Re: [EXT] Re: [PATCH 1/3] dma-buf: heaps: add Linaro secure dmabuf heap support

[Olivier Masse]

Hi Brian,


On ven., 2022-08-12 at 17:39 +0100, Brian Starkey wrote:
> Caution: EXT Email
> 
> Hi,
> 
> On Mon, Aug 08, 2022 at 02:39:53PM +0000, Olivier Masse wrote:
> > Hi Brian,
> > 
> > On ven., 2022-08-05 at 16:41 +0100, Brian Starkey wrote:
> > > Caution: EXT Email
> > > 
> > > Hi Olivier,
> > > 
> > > Thanks, I think this is looking much better.
> > > 
> > > I'd like to know how others feel about landing this heap; there's
> > > been
> > > push-back in the past about heaps in device-tree and discussions
> > > around how "custom" heaps should be treated (though IMO this is
> > > quite
> > > a generic one).
> > > 
> > > On Fri, Aug 05, 2022 at 03:53:28PM +0200, Olivier Masse wrote:
> > > > add Linaro secure heap bindings: linaro,secure-heap
> > > > use genalloc to allocate/free buffer from buffer pool.
> > > > buffer pool info is from dts.
> > > > use sg_table instore the allocated memory info, the length of
> > > > sg_table is 1.
> > > > implement secure_heap_buf_ops to implement buffer share in
> > > > difference device:
> > > > 1. Userspace passes this fd to all drivers it wants this buffer
> > > > to share with: First the filedescriptor is converted to a
> > > > &dma_buf
> > > > using
> > > > dma_buf_get(). Then the buffer is attached to the device using
> > > > dma_buf_attach().
> > > > 2. Once the buffer is attached to all devices userspace can
> > > > initiate DMA
> > > > access to the shared buffer. In the kernel this is done by
> > > > calling
> > > > dma_buf_map_attachment()
> > > > 3. get sg_table with dma_buf_map_attachment in difference
> > > > device.
> > > > 
> > > 
> > > I think this commit message could use a little rework. A few
> > > thoughts:
> > > 
> > > * The bindings are in a separate commit, so seems strange to
> > > mention
> > >   here.
> > 
> > what about:
> > "add Linaro secure heap compatible reserved memory: linaro,secure-
> > heap"
> > 
> 
> I'd say something like:
> 
> Add a dma-buf heap to allocate secure buffers from a reserved-memory
> region.
> 
> ..snip

ok right.

> 
> > > > +
> > > > +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&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OKZhaNevD5dj7Wjm6zbZlij0mPA9XYyio1NAN3VjTVM%3D&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.


>  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&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XVpI93dXYu%2BGswLE8dcYboq%2FAWzSJn9j9LMlngpr238%3D&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&data=05%7C01%7Colivier.masse%40nxp.com%7C6b3d47f1e15c41a8cf7108da7c813ef6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637959191795668899%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5Ec61NC1f0UQU%2F3BEURZQhEBrZ%2FuvJ1vaoSN4ChMn%2Bw%3D&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.
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.

> 
> 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

> 
> Cheers,
> -Brian
> 
> > 
> > > 
> > > Thanks,
> > > -Brian
> > > 
> > > > +
> > > > +module_init(secure_heap_create);
> > > > +MODULE_LICENSE("GPL v2");
> > > > --
> > > > 2.25.0
> > > >