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