Comment 12 for bug 1657409

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