Message-ID: <CA+ddPcMFM40S0sH70-StdjdJ_YpX6vfwTeXAYBJLtm911gPU9Q@mail.gmail.com>
Date:   Wed, 27 Sep 2023 11:30:39 -0700
From:   Jeffrey Kardatzke <jkardatzke@...gle.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     Yunfei Dong (董云飞) 
        <Yunfei.Dong@...iatek.com>,
        "nicolas.dufresne@...labora.com" <nicolas.dufresne@...labora.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "frkoenig@...omium.org" <frkoenig@...omium.org>,
        "stevecho@...omium.org" <stevecho@...omium.org>,
        "wenst@...omium.org" <wenst@...omium.org>,
        "nhebert@...omium.org" <nhebert@...omium.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "benjamin.gaignard@...labora.com" <benjamin.gaignard@...labora.com>,
        "hsinyi@...omium.org" <hsinyi@...omium.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "nfraprado@...labora.com" <nfraprado@...labora.com>
Subject: Re: [PATCH 12/14] media: medkatek: vcodec: set secure mode to decoder driver

Sounds great Hans! I'll work with Mediatek to update their code for that.

On Wed, Sep 27, 2023 at 12:26 AM Hans Verkuil <hverkuil-cisco@...all.nl> wrote:
>
> On 26/09/2023 22:59, Jeffrey Kardatzke wrote:
> > Hans,
> >
> > I've been looking through the v4l2/vbuf2 code to get an idea of the
> > details for implementing a new memory type for secure buffers.  What
> > it comes down to essentially is that it would behave just like
> > V4L2_MEMORY_DMABUF, but then there would be an extra check in
> > __prepare_dmabuf (in videobuf2-core.c) when the memory type is SECURE
> > to ensure that it is actually from a secure dma-buf allocation.  So
> > I'm thinking an alternate solution might be cleaner so we don't have
> > two memory types that are handled nearly identically in most of the
> > code. What do you think about a new memory flag like
> > V4L2_MEMORY_FLAG_SECURE?  This would be set in vb2_queue struct like
> > the other existing memory flag. Then when it gets into
> > __prepare_dmabuf and invokes attach_dmabuf on each buffer...that call
> > could then check for the existence of that flag, and if it's there it
> > could validate it is actually secure memory.  Then in various other
> > dmabuf vb2_mem_ops (maybe alloc, get_userptr, vaddr and mmap) those
> > could also check for the secure flag, and if present return an
> > error/null.  Then also in the driver specific vb2_ops for queue_setup,
> > the MTK driver could recognize the flag there and then configure
> > itself for secure mode.
> >
> > How does that sound as an overall strategy?
>
> Yes, I actually had the same thought.
>
> You would also need a new capability: V4L2_BUF_CAP_SUPPORTS_SECURE_MEMORY
>
> It makes more sense than creating a new V4L2_MEMORY_ type, and it still
> is handled at the right place (creating the buffers).
>
> Regards,
>
>         Hans
>
> >
> > Cheers,
> > Jeff
> >
> > On Mon, Sep 25, 2023 at 9:51 AM Jeffrey Kardatzke <jkardatzke@...gle.com> wrote:
> >>
> >> On Mon, Sep 25, 2023 at 2:00 AM Hans Verkuil <hverkuil-cisco@...all.nl> wrote:
> >>>
> >>> On 22/09/2023 21:17, Jeffrey Kardatzke wrote:
> >>>> On Fri, Sep 22, 2023 at 1:44 AM Hans Verkuil <hverkuil-cisco@...all.nl> wrote:
> >>>>>
> >>>>> On 22/09/2023 05:28, Yunfei Dong (董云飞) wrote:
> >>>>>> Hi Hans,
> >>>>>>
> >>>>>> Thanks for your help to give some good advice.
> >>>>>> On Wed, 2023-09-20 at 09:20 +0200, Hans Verkuil wrote:
> >>>>>>>
> >>>>>>>>>>> In any case, using a control to switch to secure mode and using
> >>>>>>> a control
> >>>>>>>>>>> to convert a dmabuf fd to a secure handle seems a poor choice to
> >>>>>>> me.
> >>>>>>>>>>>
> >>>>>>>>>>> I was wondering if it wouldn't be better to create a new
> >>>>>>> V4L2_MEMORY_ type,
> >>>>>>>>>>> e.g. V4L2_MEMORY_DMABUF_SECURE (or perhaps _DMABUF_OPTEE). That
> >>>>>>> ensures that
> >>>>>>>>>>> once you create buffers for the first time, the driver can
> >>>>>>> switch into secure
> >>>>>>>>>>> mode, and until all buffers are released again you know that the
> >>>>>>> driver will
> >>>>>>>>>>> stay in secure mode.
> >>>>>>>>>>
> >>>>>>>>>> Why do you think the control for setting secure mode is a poor
> >>>>>>> choice?
> >>>>>>>>>> There's various places in the driver code where functionality
> >>>>>>> changes
> >>>>>>>>>> based on being secure/non-secure mode, so this is very much a
> >>>>>>> 'global'
> >>>>>>>>>> setting for the driver. It could be inferred based off a new
> >>>>>>> memory
> >>>>>>>>>> type for the queues...which then sets that flag in the driver;
> >>>>>>> but
> >>>>>>>>>> that seems like it would be more fragile and would require
> >>>>>>> checking
> >>>>>>>>>> for incompatible output/capture memory types. I'm not against
> >>>>>>> another
> >>>>>>>>>> way of doing this; but didn't see why you think the proposed
> >>>>>>> method is
> >>>>>>>>>> a poor choice.
> >>>>>>>>>
> >>>>>>>>> I assume you are either decoding to secure memory all the time, or
> >>>>>>> not
> >>>>>>>>> at all. That's something you would want to select the moment you
> >>>>>>> allocate
> >>>>>>>>> the first buffer. Using the V4L2_MEMORY_ value would be the
> >>>>>>> natural place
> >>>>>>>>> for that. A control can typically be toggled at any time, and it
> >>>>>>> makes
> >>>>>>>>> no sense to do that for secure streaming.
> >>>>>>>>>
> >>>>>>>>> Related to that: if you pass a dmabuf fd you will need to check
> >>>>>>> somewhere
> >>>>>>>>> if the fd points to secure memory or not. You don't want to mix
> >>>>>>> the two
> >>>>>>>>> but you want to check that at VIDIOC_QBUF time.
> >>>>>>>>>
> >>>>>>>>> Note that the V4L2_MEMORY_ value is already checked in the v4l2
> >>>>>>> core,
> >>>>>>>>> drivers do not need to do that.
> >>>>>>>>
> >>>>>>>> Just to clarify a bit, and make sure I understand this too. You are
> >>>>>>> proposing to
> >>>>>>>> introduce something like:
> >>>>>>>>
> >>>>>>>>    V4L2_MEMORY_SECURE_DMABUF
> >>>>>>>>
> >>>>>>>> Which like V4L2_MEMORY_DMABUF is meant to import dmabuf, while
> >>>>>>> telling the
> >>>>>>>> driver that the memory is secure according to the definition of
> >>>>>>> "secure" for the
> >>>>>>>> platform its running on.
> >>>>>>>>
> >>>>>>>> This drivers also allocate secure SHM (a standard tee concept) and
> >>>>>>> have internal
> >>>>>>>> allocation for reconstruction buffer and some hw specific reference
> >>>>>>> metadata. So
> >>>>>>>> the idea would be that it would keep allocation using the dmabuf
> >>>>>>> heap internal
> >>>>>>>> APIs ? And decide which type of memory based on the memory type
> >>>>>>> found in the
> >>>>>>>> queue?
> >>>>>>>
> >>>>>>> Yes. Once you request the first buffer you basically tell the driver
> >>>>>>> whether it
> >>>>>>> will operate in secure or non-secure mode, and that stays that way
> >>>>>>> until all
> >>>>>>> buffers are freed. I think that makes sense.
> >>>>>>>
> >>>>>>
> >>>>>> According to iommu's information, the dma operation for secure and non-
> >>>>>> secure are the same, whether just need to add one memory type in v4l2
> >>>>>> framework the same as V4L2_MEMORY_DMABUF? The dma operation in
> >>>>>> videobuf2-dma-contig.c can use the same functions.
> >>>>>
> >>>>> So if I pass a non-secure dma fd to the capture queue of the codec, who
> >>>>> will check that it can't write the data to that fd? Since doing so would
> >>>>> expose the video. Presumably at some point the tee code will prevent that?
> >>>>> (I sincerely hope so!)
> >>>>
> >>>> It is entirely the job of the TEE to prevent this. Nothing in the
> >>>> kernel should allow exploitation of what happens in the TEE no matter
> >>>> what goes on in the kernel
> >>>>
> >>>>>
> >>>>> Having a separate V4L2_MEMORY_DMABUF_SECURE type is to indicate to the
> >>>>> driver that 1) it can expect secure dmabuf fds, 2) it can configure itself
> >>>>> for that (that avoids using a control to toggle between normal and secure mode),
> >>>>> and at VIDIOC_QBUF time it is easy for the V4L2 core to verify that the
> >>>>> fd that is passed in is for secure memory. This means that mistakes by
> >>>>> userspace are caught at QBUF time.
> >>>>>
> >>>>> Of course, this will not protect you (people can disable this check by
> >>>>> recompiling the kernel), that still has to be done by the firmware, but
> >>>>> it catches userspace errors early on.
> >>>>>
> >>>>> Also, while for this hardware the DMA operation is the same, that might
> >>>>> not be the case for other hardware.
> >>>>
> >>>> That's a really good point. So one of the other models that is used
> >>>> for secure video decoding is to send the encrypted buffer into the
> >>>> video decoder directly (i.e. V4L2_MEMORY_MMAP) and then also send in
> >>>> all the corresponding crypto parameters (i.e. algorithm, IV,
> >>>> encryption pattern, etc.). Then the video driver internally does the
> >>>> decryption and decode in one operation.  That's not what we want to
> >>>> use here for Mediatek; but I've done other integrations that work that
> >>>> way (that was for VAAPI [1], not V4L2...but there are other ARM
> >>>> implementations that do operate that way).  So if we end up requiring
> >>>> V4L2_MEMORY_DMABUF_SECURE to indicate secure mode and enforce it on
> >>>> output+capture, that'll close off other potential solutions in the
> >>>> future.
> >>>>
> >>>> Expanding on your point about DMA operations being different on
> >>>> various hardware, that also makes me think a general check for this in
> >>>> v4l2 code may also be limiting. There are various ways secure video
> >>>> pipelines are done, so leaving these checks up to the individual
> >>>> drivers that implement secure video decode may be more pragmatic. If
> >>>> there's a generic V4L2 _CID_SECURE_MODE control, that makes it more
> >>>> general for how drivers can handle secure video decode.
> >>>
> >>> No, using a control for this is really wrong.
> >>>
> >>> The reason why I want it as a separate memory type is that that is
> >>> what you use when you call VIDIOC_REQBUFS, and that ioctl is also
> >>> when things are locked down in a driver. As long as no buffers have
> >>> been allocated, you can still change formats, parameters, etc. But
> >>> once buffers are allocated, most of that can't be changed, since
> >>> changing e.g. the format would also change the buffer sizes.
> >>>
> >>> It also locks down who owns the buffers by storing the file descriptor.
> >>> This prevents other processes from hijacking the I/O streaming, only
> >>> the owner can stream buffers.
> >>>
> >>> So it is a natural point in the sequence for selecting secure
> >>> buffers.
> >>>
> >>> If you request V4L2_MEMORY_DMABUF_SECURE for the output, then the
> >>> capture side must also use DMABUF_SECURE. Whether or not you can
> >>> use regular DMABUF for the output side and select DMABUF_SECURE
> >>> on the capture side is a driver decision. It can be useful to
> >>> support this for testing the secure capture using regular video
> >>> streams (something Nicolas discussed as well), but it depends on
> >>> the hardware whether you can use that technique.
> >>
> >> OK, that does work for the additional cases I mentioned.  And for
> >> testing...we would still want to use DMABUF_SECURE on both ends for
> >> Mediatek at least (that's the only way they support it). But rather
> >> than having to bother with a clearkey implementation...we can just do
> >> something that directly copies compressed video into the secure
> >> dmabufs and then exercises the whole pipeline from there. This same
> >> thing happens with the 'clear lead' that is sometimes there with
> >> encrypted video (where the first X seconds are unencrypted and then it
> >> switches to encrypted...but you're still using the secure video
> >> pipeline on the unencrypted frames in that case).
> >>
> >>
> >>>
> >>> Regards,
> >>>
> >>>         Hans
> >>>
> >>>>
> >>>> [1] - https://github.com/intel/libva/blob/master/va/va.h#L2177
> >>>>
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>>         Hans
> >>>>>
> >>>>>>
> >>>>>> Best Regards,
> >>>>>> Yunfei Dong
> >>>>>>
> >>>>>
> >>>
>

Powered by blists - more mailing lists