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