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

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(a)canonical.com&gt; > --- > 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.

> + 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)?

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