Message-ID: <3e053387-4ba6-49bc-a59a-46854e0a7c26@xs4all.nl>
Date:   Fri, 22 Sep 2023 10:44:08 +0200
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     Yunfei Dong (董云飞) 
        <Yunfei.Dong@...iatek.com>,
        "jkardatzke@...gle.com" <jkardatzke@...gle.com>,
        "nicolas.dufresne@...labora.com" <nicolas.dufresne@...labora.com>
Cc:     "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

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!)

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.

Regards,

	Hans

> 
> Best Regards,
> Yunfei Dong
> 

Powered by blists - more mailing lists