Hi Amir,
On Wed, Jul 9, 2025 at 6:46 AM Amirreza Zarrabi via OP-TEE op-tee@lists.trustedfirmware.org wrote:
Hi Jens,
Sorry for multiple replies.
On 7/9/2025 10:40 AM, Amirreza Zarrabi via OP-TEE wrote:
Hi Jens,
On 7/7/2025 9:22 PM, Jens Wiklander wrote:
Hi Amir,
On Mon, Jul 7, 2025 at 4:22 AM Amirreza Zarrabi via OP-TEE op-tee@lists.trustedfirmware.org wrote:
Hi Jens,
On 6/10/2025 11:13 PM, Jens Wiklander wrote:
Implement DMA heap for protected DMA-buf allocation in the TEE subsystem.
Protected memory refers to memory buffers behind a hardware enforced firewall. It is not accessible to the kernel during normal circumstances but rather only accessible to certain hardware IPs or CPUs executing in higher or differently privileged mode than the kernel itself. This interface allows to allocate and manage such protected memory buffers via interaction with a TEE implementation.
The protected memory is allocated for a specific use-case, like Secure Video Playback, Trusted UI, or Secure Video Recording where certain hardware devices can access the memory.
The DMA-heaps are enabled explicitly by the TEE backend driver. The TEE backend drivers needs to implement protected memory pool to manage the protected memory.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
drivers/tee/Kconfig | 5 + drivers/tee/Makefile | 1 + drivers/tee/tee_heap.c | 472 ++++++++++++++++++++++++++++++++++++++ drivers/tee/tee_private.h | 6 + include/linux/tee_core.h | 65 ++++++ 5 files changed, 549 insertions(+) create mode 100644 drivers/tee/tee_heap.c
[snip]
I'm having a bit of trouble understanding the rationale behind supporting tee_device_unregister_all_dma_heaps(). Considering that the heap remains accessible from userspace, wouldn't this lead to undefined behavior? For example, what happens if a user is in the middle of a tee_dma_heap_alloc() -- specifically before tee_device_get() -- while the backend calls tee_device_unregister_all_dma_heaps()?
That can't happen since tee_device_unregister() has been called before, guaranteeing that no further calls to tee_device_get() can succeed.
The issue lies with tee_device_unregister(). After it returns, teedev is released (I don't see get_device() anywhere), yet the dma heap is still holding a reference to it. I believe it could cause null deref.
After reviewing it again, you're right -- I didn’t notice that teedev is being read from h->teedev. However, since h->teedev could be null, we should still check for null before calling tee_device_get().
Yes, you're right.
I understand that you want to use teedev refcount to protect against the TEE driver unbinding if there is a buffer allocated. But what about the heap device?
Additionally, what if the user decides to unbind the TEE backend driver? Would the dma_heap device still persist without any alloc function?
Yes, the device would still be there, but alloc would return -EINVAL until a new heap has been registered.
That’s only valid if the back-end is built-in. What happens if it’s a module and gets removed? In that case, the alloc function isn’t present to return -EINVAL. Perhaps using module_get() for each dma heap could be a workaround for now?
This point remains valid: if we're using the dma heap, we should either use module_get() or make the driver built-in.
Correct, I prefer module_get() since we can then keep it as a module.
Regards, Amir
But I think you're on to something, we should perhaps increase the TEE module refcount when calling dma_heap_add(). Do you agree?
Since the lifetime of the dma heaps affects teedev, it seems more appropriate to maintain a reference count per dma heap and release it during unregister. This also addresses the first issue.
OK, I'll try to propose something in the next version of the patchset.
Cheers, Jens
Regards, Amir
In the case of qcomtee, my original idea was to have two separate drivers loaded alongside each other. This setup would allow the TEE backend driver to support unbinding, while the protected memory backend could remain loaded. This separation is particularly useful for FFA, or scenarios where the protected buffer does not need to be transfered to TEE.
Or
A reference to teedev could be obtained when the heap is registered, rather than for each buffer allocation. In other words, once the heap is registered, the backend must remain active and cannot be unloaded.
Or
Find a way to have something like dma_heap_rm()?
That would be helpful, but I'd prefer to keep it out of the scope of the patchset if possible.
Thanks, Jens
Please let me know if I mis-understood something? or missing something :)
Regards, Amir
+void tee_device_unregister_all_dma_heaps(struct tee_device *teedev) +{
struct tee_protmem_pool *pool;struct tee_dma_heap *h;u_long i;xa_for_each(&tee_dma_heap, i, h) {if (h) {pool = NULL;mutex_lock(&h->mu);if (h->teedev == teedev) {pool = h->pool;h->teedev = NULL;h->pool = NULL;}mutex_unlock(&h->mu);if (pool)pool->ops->destroy_pool(pool);}}+} +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps);