Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled graphics devices" implemented the detection for gl enabled devices in virt-aa-helper. But it will in certain cases e.g. if no rendernode was explicitly specified need to read /dev/dri which it currently isn't allowed. Add a rule to the apparmor profile of virt-aa-helper itself to be able to do that. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper index de9436872c..78994bcda6 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper @@ -19,6 +19,9 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { /etc/libnl-3/classid r, + # for gl enabled graphics + /dev/dri/{,*} r, + # for hostdev /sys/devices/ r, /sys/devices/** r, -- 2.17.1

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"); + virBufferAddLit(&buf, " owner \"/var/lib/libvirt/.cache/\" w,\n"); + 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"); + } if (ctl->newfile) if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) -- 2.17.1

On Fri, Feb 15, 2019 at 11:29 PM Jamie Strandboge <jamie(a)canonical.com&gt; wrote: Ok, will do *.so for the mmaps in a V2 thanks for the hint 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. -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd

Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled graphics devices" implemented the detection for gl enabled devices in virt-aa-helper. But it will in certain cases e.g. if no rendernode was explicitly specified need to read /dev/dri which it currently isn't allowed. Add a rule to the apparmor profile of virt-aa-helper itself to be able to do that. Acked-by: Jamie Strandboge <jamie(a)canonical.com&gt; Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper index de9436872c..78994bcda6 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper @@ -19,6 +19,9 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { /etc/libnl-3/classid r, + # for gl enabled graphics + /dev/dri/{,*} r, + # for hostdev /sys/devices/ r, /sys/devices/** r, -- 2.17.1

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 | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 8e22e9978a..784ee49e61 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,23 @@ 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/**.so\" mr,\n"); + virBufferAddLit(&buf, " \&quot;/usr/lib/(a){multiarch}/dri/**.so\&quot; mr,\n"); + virBufferAddLit(&buf, " \"/usr/lib/fglrx/dri/**.so\" 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"); + 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"); + virBufferAddLit(&buf, " # dri libs will trigger that, but t is not requited and DAC would deny it anyway\n"); + virBufferAddLit(&buf, " deny \"/var/lib/libvirt/.cache/\" w,\n"); + } if (ctl->newfile) if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) -- 2.17.1