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

[Daniel Vetter]

On Thu, Sep 19, 2019 at 5:13 PM Ayan Halder <Ayan.Halder@xxxxxxx> wrote:
>
> 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?

It needs that information somehow. It doesn't necessarily need to be a
flag in the uapi, it could be simply a bit of information attached to
the dma-buf.

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

Not sure whether you mean some kind of uapi dma-buf flag or something
internal, so not clear whether I should say yes or no.

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

There has been patches floating around to add a secure buffer heap (to
ion, or as a new stand-alone allocator). I'm assuming that secure
buffer mode is something that's at least somewhat standardized in the
arm world (so that all the gfx ip can share such a buffer). Hence a
standalone allocator that all drivers which want to support these
secure buffers on arm platforms can interface with sounds like a good
idea to me.

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

Ok, so definitely not a modifier then.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch