virt-manager's UI connection will need socket access for openGraphicsFD to work - otherwise users will face a failed connection error when opening the UI view. Depending on the exact versions of libvirt and qemu involved this needs either a rule from qemu to libvirt or vice versa. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 3 +++ examples/apparmor/usr.sbin.libvirtd | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index df5f512487..5caf14e418 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -188,6 +188,9 @@ @{PROC}/device-tree/** r, /sys/firmware/devicetree/** r, + # allow connect with openGraphicsFD to work + unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd),

diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 3102cab382..dd37866c2a 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -69,6 +69,11 @@ unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd//qemu_bridge_helper), signal (send) set=("term") peer=/usr/sbin/libvirtd//qemu_bridge_helper, + # allow connect with openGraphicsFD, direction reversed in newer versions + unix (send, receive) type=stream addr=none peer=(label=libvirt-[0- 9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*), + # unconfined also required if guests run without security module + unix (send, receive) type=stream addr=none peer=(label=unconfined),

On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote: > Several cases were found needing /tmp, for example ceph will try to > list /tmp > and the samba feature of qemu will place things in /tmp/qemu-smb.*. > This is sort of safe because: > - While /tmp could contain anything it is not recommended to put > critical > data there anyway > - We restrict general access to only dir listing and reading of > files owned > (intentionally not the full power of user-tmp abstraction) > - While it would be hard to predict the PID as part of the string > for the > qemu smb feature (this is not exposed through XML so virt-aa- > helper > can't help) it is guarded by the "owner" statement and a pretty > clear > qemu-smb infix in the path. > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > --- > examples/apparmor/libvirt-qemu | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 5caf14e418..c4f231b328 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -180,6 +180,16 @@ > # for rbd > /etc/ceph/ceph.conf r, > > + # various functions will need /tmp (e.g. ceph), allow the base dir > and a > + # few known functions. > + # we want to avoid to give blanket read or even write to > everything under /tmp > + # so users are expected to add site specific addons for more > uncommon cases. > + # allow only dir listing and owner based file read > + /{,var/}tmp/ r, > + owner /{,var/}tmp/**/ r, > + # allow qemu smb feature specific path with write access > + owner /tmp/qemu-smb.*/{,**} rw, The actual rule additions are a compromise between security and usability. Personally I'd prefer they not be there and let admins add them or allow distros to patch them. That said, the comments and commit message don't seem quite right for what you are adding. Eg, you mention ceph, but there is no ceph rule and the profile comment doesn't mention ceph only needs to list dirs; the profile comments are formatted a bit weird too. You mention 'sort of safe', but because of the '/{,var/}tmp/ r,' rule, you are allowing guests to enumerate all the /tmp/qemu-smb.* directories and then there is a 'rw' rule for that path. While this is an 'owner' match, in practice, VMs all run under the same uid so this means a rogue vm would be allowed to access files in these directories. That said, I don't know what qemu is setting up in there-- so maybe it is designed in such a way that this doesn't matter. I'd much rather not call this 'sort of safe' but instead call out the problem, justify why the rule should be there and perhaps add a TODO that once smb is supported in domain xml that this rule will be added conditionally.

+0 for rule inclusion provided the comments and commit message are addressed. +1 if it can be demonstrated that qemu is handling those dirs safely wrt vm isolation. -- Jamie Strandboge | http://www.canonical.com

Libvirt now tries to preserve all mounts under /dev in qemu namespaces. The old rules only listed a set of known paths but those are no more enough. I found some due to containers like /dev/.lxc/* and such but also /dev/console and /dev/net/tun. Libvirt is correct to do so, but we can no more predict the names properly, so we modify the rule to allow a wildcard based pattern matching what libvirt does. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- examples/apparmor/usr.sbin.libvirtd | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 3ff43c32a2..b2e38fe0ad 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -33,17 +33,11 @@ mount options=(rw,rslave) -> /, mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/, - mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/, - mount options=(rw, move) /dev/hugepages/ -> /{var/,}run/libvirt/qemu/*.hugepages/, - mount options=(rw, move) /dev/mqueue/ -> /{var/,}run/libvirt/qemu/*.mqueue/, - mount options=(rw, move) /dev/pts/ -> /{var/,}run/libvirt/qemu/*.pts/, - mount options=(rw, move) /dev/shm/ -> /{var/,}run/libvirt/qemu/*.shm/, - - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ -> /dev/hugepages/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/ -> /dev/mqueue/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ -> /dev/pts/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ -> /dev/shm/, + # libvirt provides any mounts under /dev to qemu namespaces + mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/, + mount options=(rw, move) /dev/**{/,} -> /{var/,}run/libvirt/qemu/*{/,},

+ mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/, + mount options=(rw, move) /{var/,}run/libvirt/qemu/*{/,} -> /dev/**{/,},