Re: [virtio-dev] Re: [RFC PATCH v6] virtio-video: Add virtio video device specification

[Alexander Gordeev <alexander.gordeev@opensynergy.com>]

[-- Attachment #1: Type: text/plain, Size: 6313 bytes --]

On 21.04.23 18:01, Alexander Gordeev wrote:
Hi Alexandre,

On 21.04.23 06:02, Alexandre Courbot wrote:
* I am still not convinced that V4L2 is lacking from a security
perspective. It would take just one valid example to change my mind
(and no, the way the queues are named is not valid). And btw, if it
really introduces security issues, then this makes it invalid for
inclusion in virtio entirely, just not OpSy's hypervisor.

I'd like to start with this and then answer everything else later.

Let's compare VIRTIO_VIDEO_CMD_RESOURCE_QUEUE with
VIDIOC_QBUF+VIDIOC_DQBUF. Including the parameters, of course. First,
let's compare the word count to get a very rough estimate of complexity.
I counted 585 words for VIRTIO_VIDEO_CMD_RESOURCE_QUEUE, including the
parameters. VIDIOC_QBUF+VIDIOC_DQBUF are defined together and take 1206
words, they both use struct v4l2_buffer as a parameter. The struct takes
2716 words to be described. So the whole thing takes 3922 words. This is
6.7 times more, than VIRTIO_VIDEO_CMD_RESOURCE_QUEUE. If we check the
definitions of the structs, it is also very obvious, that V4L2 UAPI is
almost like an order of magnitude more complex.


I think, it is best to add all the steps necessary to reproduce my calculations just in case.

VIRTIO_VIDEO_CMD_RESOURCE_QUEUE is doing essentially the same thing as VIDIOC_QBUF+VIDIOC_DQBUF, so we're comparing apples to apples (if we don't forget to compare their parameters too).

To get the word count for the VIRTIO_VIDEO_CMD_RESOURCE_QUEUE I opened the rendered PDF of video section only from the first email in this thread. Here is the link: https://drive.google.com/file/d/1Sm6LSqvKqQiwYmDE9BXZ0po3XTKnKYlD/view?usp=sharing . Then I scrolled to page 11 and copied everything related a text file. This is around two pages in the PDF. Then I removed page numbers from the copied text and used 'wc -w' to count words.

To get the word count for VIDIOC_QBUF+VIDIOC_DQBUF I opened this link: https://docs.kernel.org/userspace-api/media/v4l/vidioc-qbuf.html . Then I selected all the text except table of contents and did followed the same procedure.

To get the word count for struct v4l2_buffer and other types, that are referenced from it, I opened this link: https://docs.kernel.org/userspace-api/media/v4l/buffer.html#struct-v4l2-buffer . Then I selected all the text except the table of contents and the text above struct v4l2_buffer definition. The rest is the same.

Also it's quite obvious if you look at them how much bigger struct v4l2_buffer (including the referenced types) is compared to struct virtio_video_resource_queue.

Do we agree now, that V4L2 UAPI is not only marginally more complex?


Also please read:

https://medium.com/starting-up-security/evidence-of-absence-8148958da092


This reference probably needs a clarification. You argued, that V4L2 has a good track record so far. Here is the quote:

FWIW V4L2 has been in use for a looong time (including in Chromebooks
that do secure playback) and I am not aware of fundamental security
issues with it.

But absence of found major security issues doesn't tell us much about the number of not found ones. Absence of evidence is not an evidence of absence. At the link above a former Director of Security at Facebook shares his thoughts about what could be a good evidence of absence of major security problems.

So a bug bounty program with high premiums that covers V4L2 would be a better argument in favor of *already written code* in my opinion. Not for new code. Also probably it is also an argument in favor of the spec, that is the V4L2 UAPI. Like that it is polished enough. Not so sure about that though.

There actually are several bug bounty programs, that cover the kernel. These are Google's kctf, ZDI's pwn2own, and zerodium AFAIK. However the premiums are not even close to the ones mentioned in my reference. Anyway this means, that using *the existing V4L2 code in the kernel* is probably OK. But this creates some limitations if we want the actual code to still be covered with these bug bounties, right? This means, that the host OS has to be Linux and the actual hardware has to be exposed through a stable V4L2 driver, that is in mainline for some time, and there has to be no or little processing on top. For us this is not possible unfortunately. In the end both things could be secure:

1. V4L2 pass through can be secure because of the bug bounty programs and a lot of attention to the kernel in general.
2. For the new code this doesn't work, so the spec should be as simple and device-centric as possible. Because, all other things being equal, there are fewer errors in simpler programs. So defining a subset of V4L2 UAPI including the data types looks like a good idea to me. The stateful decoder interface, that you point to, does not define a subset in the data types.

This is basically my reasoning.

Also these two specs don't need to compete with each other. They have different limitations and they are for different audiences. If you check the XKCD's comic, it is about competing standards.


https://www.schneier.com/essays/archives/1999/11/a_plea_for_simplicit.html


A quote from this article:

The worst enemy of security is complexity.

I hope I've provided above some evidence, that V4L2 UAPI is significantly more complex. You asked for one example, I provided it. For us this is already something to care about.


--
Alexander Gordeev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone: +49 30 60 98 54 0 - 88
Fax: +49 (30) 60 98 54 0 - 99
EMail: alexander.gordeev@opensynergy.com<mailto:alexander.gordeev@opensynergy.com>

www.opensynergy.com<http://www.opensynergy.com>

Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Régis Adjamah

Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>

[-- Attachment #2: Type: text/html, Size: 8813 bytes --]