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

[Jamie Strandboge]

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.

> +        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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190215/cf983a57/attachment-0001.sig>