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@xxxxxxxxx> 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@xxxxxxxxxx> wrote: > >> > >> On Mon, Sep 25, 2023 at 2:00 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > >>> > >>> On 22/09/2023 21:17, Jeffrey Kardatzke wrote: > >>>> On Fri, Sep 22, 2023 at 1:44 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> 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 > >>>>>> > >>>>> > >>> >
