On Fri, Feb 15, 2019 at 11:29 PM Jamie Strandboge <jamie@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > --- > > 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
