Re: [RFC PATCH 0/5] drm/panthor: Protected mode support for Mali CSF GPUs

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Maxime,

On Thu, 20 Feb 2025 14:32:14 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> > > > This approach has two downsides though:
> > > > 
> > > > 1. We have no way of checking that the memory we're passed is actually
> > > > suitable for FW execution in a protected context. If we're passed
> > > > random memory, this will likely hang the platform as soon as we enter
> > > > protected mode.    
> > > 
> > > It's a current limitation of dma-buf in general, and you'd have the same
> > > issue right now if someone imports a buffer, or misconfigure the heap
> > > for a !protected heap.
> > > 
> > > I'd really like to have some way to store some metadata in dma_buf, if
> > > only to tell that the buffer is protected.  
> > 
> > The dma_buf has a pointer to its ops, so it should be relatively easy
> > to add an is_dma_buf_coming_from_this_heap() helper. Of course this
> > implies linking the consumer driver to the heap it's supposed to take
> > protected buffers from, which is basically the thing being discussed
> > here :-).  
> 
> I'm not sure looking at the ops would be enough. Like, you can compare
> that the buffer you allocated come from the heap you got from the DT,
> but if that heap doesn't allocate protected buffers, you're screwed and
> you have no way to tell.

If heap names are unique, the name of the heap should somehow guarantee
the protected/restricted nature of buffers allocated from this heap
though. So, from a user perspective, all you have to do is check that
the buffers you import come from this particular heap you've been
pointed to. Where we get the heap name from (DT or module param
passed through a whitelist of protected heap names?) is an
implementation detail.

> 
> It also falls apart if we have a heap driver with multiple instances,
> which is pretty likely if we ever merge the carveout heap driver.

What I meant here is that checking that a buffer comes from a
particular heap is something the heap driver itself can easily do. It
can be a mix of ops+name check (or ops+property check) if there's
multiple heaps instantiated by a single driver, of course.

I guess the other option would be to have a protected property at the
dma_buf level so we don't have to go all the way back to the dma_heap
to figure it out.

> 
> > > 
> > > I suspect you'd also need that if you do things like do protected video
> > > playback through a codec, get a protected frame, and want to import that
> > > into the GPU. Depending on how you allocate it, either the codec or the
> > > GPU or both will want to make sure it's protected.  
> > 
> > If it's all allocated from a central "protected" heap (even if that
> > goes through the driver calling the dma_heap_alloc_buffer()), it
> > shouldn't be an issue.  
> 
> Right, assuming we have a way to identify the heap the buffer was
> allocated from somehow. This kind of assumes that you only ever get one
> source of protected memory, and you'd never allocate a protected buffer
> from a different one in the codec driver for example.

Yes, and that's why having the ability to check that a buffer comes
from a particular heap is key. I mean, we don't necessarily have to
restrict things to a single heap, it can be a whitelist of heaps we know
provide protected buffers if we see a value in having multiple
protected heaps coexisting on a single platform.

Regards,

Boris