Re: [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

[Ayan Halder]

On Wed, Sep 18, 2019 at 10:30:12PM +0100, Daniel Stone wrote:

Hi All,
Thanks for your suggestions.

> Hi Liviu,
> 
> On Wed, 18 Sep 2019 at 13:04, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
> > On Wed, Sep 18, 2019 at 09:49:40AM +0100, Daniel Stone wrote:
> > > I totally agree. Framebuffers aren't about the underlying memory they
> > > point to, but about how to _interpret_ that memory: it decorates a
> > > pointer with width, height, stride, format, etc, to allow you to make
> > > sense of that memory. I see content protection as being the same as
> > > physical contiguity, which is a property of the underlying memory
> > > itself. Regardless of the width, height, or format, you just cannot
> > > access that memory unless it's under the appropriate ('secure enough')
> > > conditions.
> > >
> > > So I think a dmabuf attribute would be most appropriate, since that's
> > > where you have to do all your MMU handling, and everything else you
> > > need to do to allow access to that buffer, anyway.
> >
> > Isn't it how AMD currently implements protected buffers as well?
> 
> No idea to be honest, I haven't seen anything upstream.
> 
> > > There's a lot of complexity beyond just 'it's protected'; for
> > > instance, some CP providers mandate that their content can only be
> > > streamed over HDCP 2.2 Type-1 when going through an external
> > > connection. One way you could do that is to use a secure world
> > > (external controller like Intel's ME, or CPU-internal enclave like SGX
> > > or TEE) to examine the display pipe configuration, and only then allow
> > > access to the buffers and/or keys. Stuff like that is always going to
> > > be out in the realm of vendor & product-policy-specific add-ons, but
> > > it should be possible to agree on at least the basic mechanics and
> > > expectations of a secure path without things like that.
> >
> > I also expect that going through the secure world will be pretty much transparent for
> > the kernel driver, as the most likely hardware implementations would enable
> > additional signaling that will get trapped and handled by the secure OS. I'm not
> > trying to simplify things, just taking the stance that it is userspace that is
> > coordinating all this, we're trying only to find a common ground on how to handle
> > this in the kernel.
> 
> Yeah, makes sense.
> 
> As a strawman, how about a new flag to drmPrimeHandleToFD() which sets
> the 'protected' flag on the resulting dmabuf?

To be honest, during our internal discussion james.qian.wang@xxxxxxx had a
similar suggestion of adding a new flag to dma_buf but I decided
against it.

As per my understanding, adding custom dma buf flags (like
AMDGPU_GEM_CREATE_XXX, etc) is possible if we(Arm) had our own allocator. We
rely on the dumb allocator and ion allocator for framebuffer creation.

I was looking at an allocator independent way of userspace
communicating to the drm framework that the framebuffer is protected.

Thus, it looked to me that framebuffer modifier is the best (or the least
corrupt) way of going forth.

We use ion and dumb allocator for framebuffer object creation. The way
I see it is as follows :-

1. For ion allocator :-
Userspace can specify that it wants the buffer from a secure heap or any other
special region of heap. The ion driver will either fault into the secure os to
create the buffers or it will do some other magic. Honestly, I have still not
figured that out. But it will be agnostic to the drm core.

The userspace gets a handle to the buffer and then it calls addFB2 with
DRM_FORMAT_MOD_ARM_PROTECTED, so that the driver can configure the
dpu's protection bits (to access the memory with special signals).

2. For dumb allocator :-
I am curious to know if we can add 'IS_PROTECTED' flag to
drm_mode_create_dumb.flags. This can/might be used to set dma_buf
flags. Let me know if this is an incorrect/forbidden path.

In a nutshell, my objective is to figure out if the userspace is able
to communicate to the drm core about the 'protection' status of the
buffer without introducing Arm specific buffer allocator.

Thanks,
Ayan

> Cheers,
> Daniel