Re: [PATCH 12/14] media: medkatek: vcodec: set secure mode to decoder driver

quoted

On Fri, Sep 22, 2023 at 1:44 AM Hans Verkuil [off-list ref] 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.

[1] - https://github.com/intel/libva/blob/master/va/va.h#L2177

Regards,

        Hans

Best Regards,
Yunfei Dong
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help