[libvirt] [PATCH 2/2] security: aa-helper: generate more rules for gl devices

[Christian Ehrhardt]

On Fri, Feb 15, 2019 at 11:29 PM Jamie Strandboge <jamie at canonical.com> wrote:
>
> On Tue, 12 Feb 2019, Christian Ehrhardt wrote:
>
> > Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
> > graphics devices" implemented the detection for gl enabled
> > devices in virt-aa-helper. But further testing showed
> > that it will need much more access for the full gl stack
> > to work.
> >
> > Upstream apparmor just recently split those things out and now
> > has two related abstractions at
> > https://gitlab.com/apparmor/apparmor/blob/master:
> > - dri-common at /profiles/apparmor.d/abstractions/dri-common
> > - mesa: at /profiles/apparmor.d/abstractions/mesa
> >
> > If would be great to just include that for the majority of
> > rules, but they are not yet in any distribution so we need
> > to add rules inspired by them based on the testing that we
> > can do.
> >
> > Furthermore qemu with opengl will also probe the backing device
> > of the rendernode for attributes which should be safe as
> > read-only wildcard rules.
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> > ---
> >  src/security/virt-aa-helper.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index 8e22e9978a..46c1914f58 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -937,7 +937,7 @@ get_files(vahControl * ctl)
> >      size_t i;
> >      char *uuid;
> >      char uuidstr[VIR_UUID_STRING_BUFLEN];
> > -    bool needsVfio = false, needsvhost = false;
> > +    bool needsVfio = false, needsvhost = false, needsgl = false;
> >
> >      /* verify uuid is same as what we were given on the command line */
> >      virUUIDFormat(ctl->def->uuid, uuidstr);
> > @@ -1065,9 +1065,11 @@ get_files(vahControl * ctl)
> >
> >          if (rendernode) {
> >              vah_add_file(&buf, rendernode, "rw");
> > +            needsgl = true;
> >          } else {
> >              if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> >                  char *defaultRenderNode = virHostGetDRMRenderNode();
> > +                needsgl = true;
> >
> >                  if (defaultRenderNode) {
> >                      vah_add_file(&buf, defaultRenderNode, "rw");
> > @@ -1267,6 +1269,22 @@ get_files(vahControl * ctl)
> >          virBufferAddLit(&buf, "  \"/dev/vfio/vfio\" rw,\n");
> >          virBufferAddLit(&buf, "  \"/dev/vfio/[0-9]*\" rw,\n");
> >      }
> > +    if (needsgl) {
> > +        /* if using gl all sorts of further dri related paths will be needed */
> > +        virBufferAddLit(&buf, "  # DRI/Mesa/(e)GL config and driver paths\n");
> > +        virBufferAddLit(&buf, "  \"/usr/lib{,32,64}/dri/**\" mr,\n");
> > +        virBufferAddLit(&buf, "  \"/usr/lib/@{multiarch}/dri/**\" mr,\n");
> > +        virBufferAddLit(&buf, "  \"/usr/lib/fglrx/dri/**\" mr,\n");
> > +        virBufferAddLit(&buf, "  \"/etc/drirc\" r,\n");
> > +        virBufferAddLit(&buf, "  \"/usr/share/drirc.d/{,*.conf}\" r,\n");
> > +        virBufferAddLit(&buf, "  \"/etc/glvnd/egl_vendor.d/{,*}\" r,\n");
> > +        virBufferAddLit(&buf, "  \"/usr/share/glvnd/egl_vendor.d/{,*}\" r,\n");
>
> The above rules are fine. It would be nice to me slightly more specific in the
> mmap rules to mmap only .so files, but not a blocker.

Ok, will do *.so for the mmaps in a V2 thanks for the hint

> > +        virBufferAddLit(&buf, "  owner \"/var/lib/libvirt/.cache/\" w,\n");
>
> This doesn't seem to belong here and DAC is usually going to prevent it since
> VMs run as non-root and /var/lib/libvirt is 755. Perhaps get rid of owner and
> make this an explicit denial rule to silence the denial (with a code comment)?

I was just following the denials that I saw, I also couldn't fully see
the reason.
I can follow your DAC argument which is correct, and in addition every
instance would use the same cache which I'm not sure would be right.
I'll remove the line in a v2 and replace it with the suggested denial.


> > +        virBufferAddLit(&buf, "  # Probe DRI device attributes\n");
> > +        virBufferAddLit(&buf, "  \"/dev/dri/\" r,\n");
> > +        virBufferAddLit(&buf, "  \"/sys/devices/*/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\" r,\n");
> > +        virBufferAddLit(&buf, "  \"/sys/devices/*/*/drm/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\" r,\n");
>
> These are fine.
>
> --
> Jamie Strandboge             | http://www.canonical.com



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd