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