Re: [PATCH v10 3/9] tee: implement protected DMA-heap

[Sumit Garg via OP-TEE <op-tee@lists.trustedfirmware.org>]

On Mon, Jul 07, 2025 at 01:22:19PM +0200, 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.
> 
> >
> > 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.
> But I think you're on to something, we should perhaps increase the TEE
> module refcount when calling dma_heap_add(). Do you agree?

But that won't allow to cleanly un-bind/bind OP-TEE module. It looks
like we rather need to enforce the OP-TEE driver to be built-in in case
it is the provider of DMA heaps.

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

Yeah lets keep that for next feature patch-set.

-Sumit

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