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

[Ayan Halder]

On Thu, Sep 19, 2019 at 04:10:42PM +0200, Daniel Vetter wrote:
> On Thu, Sep 19, 2019 at 4:03 PM Ayan Halder <Ayan.Halder@xxxxxxx> wrote:
> >
> > 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.
> 
> Allocating buffers from a special heap is what I expected the
> interface to be. The issue is that if we specify the secure mode any
> time later on, then it could be changed. E.g. with Daniel Stone's idea
> of a handle2fd flag, you could export the buffer twice, once secure,
> once non-secure. That sounds like a silly thing to me, and better to
> prevent that - or is this actually possible/wanted, i.e. do we want to
> change the secure mode for a buffer later on?
> 
> > 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).
> 
> If we allocate a secure buffer there's no need for flags anymore I
> think - it would be a property of the underlying buffer (like a
> contiguous buffer). All we need are two things:
> - make sure secure buffers can only be imported by secure-buffer aware drivers
> - some way for such drivers to figure out whether they deal with a
> secure buffer or not.

I am with you on this. Yes, we need to communicate the above two
things.

> 
> There's no need for any flags anywhere else with the ion/secure
> dma-buf heap solution. E.g. for contig buffer we also dont pass around
> a DRM_FORMAT_MOD_PHYSICALLY_CONTIG for addfb2.

Sorry, I could not follow you here. For the driver to know if it is importing
a secure buffer or using a secure buffer, it needs some flags, right?

Or on a second thought, are you suggesting that we should stick with
a dma_buf flag (IS_PROTECTED) only ?

> 
> > 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.
> 
> dumb is dumb, this definitely feels like the wrong interface to me.
> 
> > 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.
> 
> Why does userspace need to communicate this again? What's the issue
> with using an ARM specific allocator for this?

We never felt the need to create an Arm specific allocator. Either
Dumb or Ion allocator would always suffice our purpose.

To upstream 'protected mode' feature of our komeda driver, if we need to
write our own allocator, it will be a big overhead. :)

Also to answer your earlier question

"But if it's a generic flag, how do you combine that with other
modifiers? Like if you have a tiled buffer, but also encrypted? Or
afbc compressed, or whatever else. I'd expect for your hw encryption
is orthogonal to the buffer/tiling/compression format used?"

Yes, hw encryption(protected mode) is orthogonal/independent to AFBC compression.
Thus, any/all AFBC buffers can be supported with/without protected
mode.

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch