[MIR] virglrenderer

Bug #1657409 reported by Christian Ehrhardt  on 2017-01-18
34
This bug affects 5 people
Affects Status Importance Assigned to Milestone
virglrenderer (Ubuntu)
Medium
Unassigned

Bug Description

Currently not a hard requirement yet, but I believe it could pretty fast become one.

So I wanted to start the MIR discussion at a lower prio to get a security evaluation to know the current status when any hard request comes up.

It was just enabled in Debian, but so far qemu drops this dependency due to the component mismatch.

[Availability]
- Currently in Universe
- Upstream is at https://virgil3d.github.io/

[Rationale]
- Dependency for qemu's virgl support
- Allow users KVM Guest 3d acceleration independent to GPU passthrough

[Security]
- I could not find any CVEs, but http://seclists.org/oss-sec/2016/q4/711
- Only a lib, no direct callable binaries

[Quality assurance]
- no open bugs at the moment in Ubuntu

[Dependencies]
- All further dependencies from there are in main already (as of today)

[Standards compliance]
- FHS and Debian rules are met

[Maintenance]
- no delta sync from Debian atm
- no owning Team yet (can be decided when other blockers are out of the way)

[Background information]
- So far virgl considers itself still a "research project" so please as I specified handle this evaluation at a lower prio for now.

Changed in virglrenderer (Ubuntu):
importance: Undecided → Low
Michael Terry (mterry) on 2017-01-18
Changed in virglrenderer (Ubuntu):
assignee: nobody → Mathieu Trudel-Lapierre (cyphermox)
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Move to the security team as per the description; there should be a security review of this package.

Changed in virglrenderer (Ubuntu):
assignee: Mathieu Trudel-Lapierre (cyphermox) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Emily Ratliff (emilyr) wrote :

virglrenderer is currently undergoing a security review by Li Qiang and there are now 11 open CVEs against the package.

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Please assign back to me once the security review is done so I can do the rest of the MIR review, assuming it passes your review.

Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in virglrenderer (Ubuntu):
status: New → Confirmed
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi, there was no update/result of the security Team review.

Since Debian might close in on this soon (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=813658) it would be great to have this to be able to follow.

Therefore, what was the result of this evaluation?

Revision history for this message
Matthias Klose (doko) wrote :

Li, are you still reviewing that?

Revision history for this message
Emily Ratliff (emilyr) wrote :

Li is a security researcher. There haven't been new CVEs this year, so it looks like that review is done with 13 CVEs identified.

This is on the list for us to review, but behind a some high priority reviews. Is there a need to increase the priority for this review or are you just cleaning up the queue?

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi,
I think the time is right (going from 18.10 towards 20.04 some day) to enable it now meaning 18.10 or latest 19.04 to have a few releases in between the LTSes to see how it works.
Also as assumed for some time Debian now has enabled it [1].

@Emily - I think thereby the priority increased a bit, would be great to have the security POV to this package.

[1]: https://salsa.debian.org/qemu-team/qemu/commit/fc18833073661f760eb153f055f9b711beb6f49d

Changed in virglrenderer (Ubuntu):
importance: Low → Medium
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

This is near to completion in Debian, so on next merge would be nice to be able to pick this up as well.
I'll bump the trello card for the review as well.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Just FYI: Please note that the overall feature to make it usable for a user also needs "libepoxy-dev, libdrm-dev, libgbm-dev" but those are all already in main. So it is just this MIR here.

Revision history for this message
Timo Jyrinki (timo-jyrinki) wrote :

FWIW I'm happily using VirGL based qemu also in my backport for 18.04 LTS. Thanks to bug #1804766 landing the needed changes to disco's 1:2.12+dfsg-3ubuntu9 are very small.

https://launchpad.net/~timo-jyrinki/+archive/ubuntu/qemu-virgl for whoever happens to need it on LTS.

Hopefully virgl support will make it to 19.04 officially too.

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I reviewed virglrenderer version 0.7.0-1 as checked into disco; this
shouldn't be considered a full security audit but rather a quick gauge of
maintainability.

virgl is an attempt to provide GPUs to guests that leverage the host GPUs
but may not be direct passthroughs of devices.

There's 14 CVEs in our database; most were the result of one fairly active
researcher who appeared to be doing a fairly thorough job. I had the
impression the fixes were handled well.

- Build-Depends: autoconf-archive, debhelper, libdrm-dev,
  libegl1-mesa-dev, libepoxy-dev, libgbm-dev, pkg-config, python

Is the python build dependency cause for concern?

- Does not daemonize
- Does not appear to listen to network
- No pre/post inst/rm scripts
- No init scripts
- No systemd services
- No dbus services
- No setuid files
- No binaries in PATH
- No sudo fragments
- No udev rules
- There's 6KLOC in tests/ but nothing appears to run during the build
- No cron jobs
- Build logs are extremely messy. Addressing the issues would be an
  immense undertaking. (To the point that switching to C++ to use
  std::string or Rust to use String would be easier than trying to
  address these issues in C.)

- No subprocesses spawned
- Memory management is pretty rough. C string operations without bounds
  checking, snprintfs without error return validations, etc. Fixed length
  stack-allocated buffers are used extensively and it's far from clear
  that all inputs will properly fit into the buffers.

  The generated GLGL inputs are fed into other tools and errors would
  likely cause cascading failures.

  Addressing this in C could be a disaster. I suspect trying to "fix" the
  existing code would introduce more errors than it would solve. A rewrite
  into a better language would be a lot more plausible to me.

- Opens /dev/dri/renderD* files
- Logging looked fine
- Uses many of its own environment variables:
  GALLIUM_LOG_FILE, GALLIUM_PRINT_OPTIONS, TGSI_PRINT_SANITY,
  GALLIUM_DUMP_CPU, VIRGL_DISABLE_MT, VTEST_SAVE, VTEST_USE_GLX,
  VTEST_USE_EGL_SURFACELESS, VTEST_USE_GLES, LIBGL_ALWAYS_SOFTWARE,
- No privileged operations
- No cryptography
- I'm unsure about networking -- the test infrastructure uses unix domain
  sockets, but I don't obviously spot how virglrenderer interacts outside
  its process.
- No privileged portions of code
- No temp files
- No webkit
- No JavaScript
- Some cppcheck errors have been fixed upstream since our package

I don't love the GLGL code generation. Running this code may bring the
guest operating system video drivers into the trusted codebase.

I discussed some of the issues I found with upstream authors and found
them very responsive and understanding. (To be clear I found nothing of
importance.)

Security team ACK for promoting virglrenderer to main.

Thanks

Changed in virglrenderer (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thank you so much for the check Seth.
Per process I set in-progress on this and continue to prep it that way on the merge.

Changed in virglrenderer (Ubuntu):
status: Confirmed → In Progress
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

qemu now built in Disco and triggers the expected mismatch
Doko pinged me on IRC about it:
<doko> cpaelzer: qemu-system-arm/amd64 unsatisfiable Depends: libvirglrenderer0 (>= 0.7.0)

Revision history for this message
Matthias Klose (doko) wrote :

Override component to main
virglrenderer 0.7.0-2 in disco: universe/misc -> main
libvirglrenderer-dev 0.7.0-2 in disco amd64: universe/libdevel/extra/100% -> main
libvirglrenderer-dev 0.7.0-2 in disco arm64: universe/libdevel/extra/100% -> main
libvirglrenderer-dev 0.7.0-2 in disco armhf: universe/libdevel/extra/100% -> main
libvirglrenderer-dev 0.7.0-2 in disco i386: universe/libdevel/extra/100% -> main
libvirglrenderer-dev 0.7.0-2 in disco ppc64el: universe/libdevel/extra/100% -> main
libvirglrenderer-dev 0.7.0-2 in disco s390x: universe/libdevel/extra/100% -> main
libvirglrenderer0 0.7.0-2 in disco amd64: universe/libs/extra/100% -> main
libvirglrenderer0 0.7.0-2 in disco arm64: universe/libs/extra/100% -> main
libvirglrenderer0 0.7.0-2 in disco armhf: universe/libs/extra/100% -> main
libvirglrenderer0 0.7.0-2 in disco i386: universe/libs/extra/100% -> main
libvirglrenderer0 0.7.0-2 in disco ppc64el: universe/libs/extra/100% -> main
libvirglrenderer0 0.7.0-2 in disco s390x: universe/libs/extra/100% -> main
13 publications overridden.

Changed in virglrenderer (Ubuntu):
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.