From: Serge Hallyn <serge.hallyn(a)ubuntu.com&gt; Bug-Ubuntu: https://bugs.launchpad.net/bugs/1393548 Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index d4fad85..77c72a5 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -81,6 +81,7 @@ /usr/share/proll/** r, /usr/share/vgabios/** r, /usr/share/seabios/** r, + /usr/share/misc/sgabios.bin r, /usr/share/ovmf/** r, /usr/share/OVMF/** r, /usr/share/AAVMF/** r,

--- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -191,3 +191,7 @@ /sys/devices/system/node/ r, /sys/devices/system/node/node[0-9]*/meminfo r, /sys/module/vhost/parameters/max_mem_regions r, + + # silence refusals to open lttng files (see LP: #1432644) + deny /dev/shm/lttng-ust-wait-* r, + deny /run/shm/lttng-ust-wait-* r,

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 651d841..b9e45bd 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -34,6 +34,10 @@ owner @{PROC}/@{pid}/task/@{tid}/comm rw, @{PROC}/sys/kernel/cap_last_cap r,

+ /sys/devices/system/node/ r, + /sys/devices/system/node/node[0-9]*/meminfo r, + /sys/devices/system/cpu/ r, + # For hostdev access. The actual devices will be added dynamically /sys/bus/usb/devices/ r, /sys/devices/**/usb[0-9]*/** r,

On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Jamie Strandboge <jamie(a)ubuntu.com&gt; > > Allows (multi-arch enabled) access to libraries under the > /usr/lib/(a){multiarch}/qemu/*.so path in the Debian/Ubuntu > qemu-block-extra package. > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1554761 > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > --- > examples/apparmor/libvirt-qemu | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 91d0e02..912b4ac 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -161,6 +161,9 @@ > /usr/{lib,lib64}/qemu/block-curl.so mr, > /usr/{lib,lib64}/qemu/block-rbd.so mr, > > + # for Debian/Ubuntu qemu-block-extra (LP: #1554761) > + /usr/lib/(a){multiarch}/qemu/*.so rm, > + +1 as is (though s/rm/mr/ for consistency),

but on my system I see block-curl.so, block-isci.so and block-rdb.so. I think it probably makes to adjust this rule block to simply be:

/usr/{lib,lib64}/qemu/*.so mr, /usr/lib/(a){multiarch}/qemu/*.so mr,

From: Jamie Strandboge <jamie(a)ubuntu.com&gt;

Allows (multi-arch enabled) access to libraries under the /usr/lib/(a){multiarch}/qemu/*.so path in the Debian/Ubuntu qemu-block-extra package and all such libs for the paths of rpm qemu-block-* packages.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1554761

Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 91d0e02..34a564f 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -158,8 +158,9 @@ /usr/bin/qemu-sparc64 rmix, /usr/bin/qemu-unicore32 rmix, /usr/bin/qemu-x86_64 rmix, - /usr/{lib,lib64}/qemu/block-curl.so mr, - /usr/{lib,lib64}/qemu/block-rbd.so mr, + # for Debian/Ubuntu qemu-block-extra / RPMs qemu-block-* (LP: #1554761) + /usr/{lib,lib64}/qemu/*.so mr, + /usr/lib/(a){multiarch}/qemu/*.so mr,

On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Serge Hallyn <serge.hallyn(a)ubuntu.com&gt; > > Allows owner access to hugepage mounts (both, the old and > new systemd variant). > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216 > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737 > > Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; > --- > examples/apparmor/libvirt-qemu | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 912b4ac..bb30530 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -184,6 +184,10 @@ > /sys/bus/ r, > /sys/class/ r, > > + # for access to hugepages (LP: #1250216 and LP: #1524737) > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw, > + owner "/dev/hugepages/libvirt/qemu/**" rw, > + These rules are not vm-specific. I'm not familiar with the hugepages feature in libvirt, but the rule suggests that libvirtd will create files in these directories per-vm, and then the vm uses what libvirtd created for it. I see virSecurityManagerSetHugepages() in security_manager.c, is it possible that these rules can be removed and vm-specific ones added dynamically with virt-aa-helper?

On Wed, 2017-12-20 at 14:43 +0100, Christian Ehrhardt wrote: > On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie(a)canonical.co > m> wrote: > > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > > > From: Serge Hallyn <serge.hallyn(a)ubuntu.com&gt; > > > > > > Allows owner access to hugepage mounts (both, the old and > > > new systemd variant). > > > > > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216 > > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737 > > > > > > Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; > > > --- > > > examples/apparmor/libvirt-qemu | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/examples/apparmor/libvirt-qemu > > > b/examples/apparmor/libvirt-qemu > > > index 912b4ac..bb30530 100644 > > > --- a/examples/apparmor/libvirt-qemu > > > +++ b/examples/apparmor/libvirt-qemu > > > @@ -184,6 +184,10 @@ > > > /sys/bus/ r, > > > /sys/class/ r, > > > > > > + # for access to hugepages (LP: #1250216 and LP: #1524737) > > > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw, > > > + owner "/dev/hugepages/libvirt/qemu/**" rw, > > > + > > > > These rules are not vm-specific. I'm not familiar with the > > hugepages > > feature in libvirt, but the rule suggests that libvirtd will create > > files in these directories per-vm, and then the vm uses what > > libvirtd > > created for it. I see virSecurityManagerSetHugepages() in > > security_manager.c, > > > > is it possible that these rules can be removed and > > vm-specific ones added dynamically with virt-aa-helper? > > I agree - and in general it would also be nice to do a per guest rule > with a full path to keep the guests away from each other. > I see different ways to go thou: > > #1 - per security_apparmor.c > The particular function you mention was generalized by [1] and > further > reused in [3]. > The creation of the per guest paths is in [2]. > > So essentially it comes down to a call to domainSetPathLabel with the > path as an argument. > But then security_apparmor.c so far does not implement > domainSetPathLabel at all. > > If we add a function for domainSetPathLabel it could call > reload_profile with the path (as others in security_apparmor.c do). > But this path is in use by other places as well: > - qemuDomainWriteMasterKeyFile > - qemuProcessMakeDir > > OTOH domainSetPathLabel is used in those places for the reson of it's > meaning "add this to the scope of the security label right?" > So we might after all want to add all these anyway? > > If that is so, an (untested) change could be like: > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr > mgr, > return reload_profile(mgr, def, savefile, true); > } > > +static int > +AppArmorSetPathLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > + const char *path) > +{ > + return reload_profile(mgr, def, path, true); > +} > > static int > AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, > @@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = { > .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, > .domainRestoreSavedStateLabel = > AppArmorRestoreSavedStateLabel, > > + .domainSetPathLabel = AppArmorSetPathLabel, > + > .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, > .domainSetSecurityTapFDLabel = AppArmorSetFDLabel, > > > #2 in virt-aa-helper with XML parsing > The next option would to try to detect if hugepages are used from the > xml description in virt-aa-helper. > > > #3 unconditional per domain rule > And finally we could avoid too much (?over?)engineering and > unconditionally add this in virt-aa-helper (at the cost of breaking > if > paths generated [2] ever change): > owner "/dev/hugepages/libvirt/qemu/<domainName>" rw, > > > IMHO - [1] or [3] - please let me know your opinions before I follow > any of those paths. > > [1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=ce937d37105c2f > Or a combination of 2 and 3. Ie, if hugepages is enabled, add the rule in 3. To me, 1 feels most correct cause while the other two fix hugepages, there seem to be lurking bugs since we aren't implementing domainSetPathLabel.

-- Jamie Strandboge | http://www.canonical.com

Adding the PKI path that is used as default suggestion in src/qemu/qemu.conf If people use non-default paths they should use local overrides but the suggested defaults we should open up. This is the default path as referenced by src/qemu/qemu.conf in libvirt. While doing so merge the several places we have to cover PKI access into one. Bug-Ubuntu: https://bugs.launchpad.net/bugs/1690140 Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index fa2b753..f206f6c 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -88,8 +88,11 @@ /usr/share/qemu-efi/** r, /usr/share/slof/** r, - # access PKI infrastructure - /etc/pki/libvirt-vnc/** r, + # pki for libvirt-vnc and libvirt-spice (LP: #901272, #1690140) + /etc/pki/CA/ r, + /etc/pki/CA/* r, + /etc/pki/libvirt{,-spice,-vnc}/ r, + /etc/pki/libvirt{,-spice,-vnc}/** r, # the various binaries /usr/bin/kvm rmix, @@ -156,12 +159,6 @@ /usr/{lib,lib64}/qemu/*.so mr, /usr/lib/(a){multiarch}/qemu/*.so mr, - # for use by libvirt-vnc (LP: #901272) - /etc/pki/CA/ r, - /etc/pki/CA/* r, - /etc/pki/libvirt/ r, - /etc/pki/libvirt/** r, - # for save and resume /{usr/,}bin/dash rmix, /{usr/,}bin/dd rmix,

vfio devices are generated on the fly, but the generic base is missing. The base vfio has not much functionality but to provide a custom container by opening this path. See https://www.kernel.org/doc/Documentation/vfio.txt for more. Current access by qemu is "wr": [ 2652.756712] audit: type=1400 audit(1491303691.719:25): apparmor="DENIED" operation="open" profile="libvirt-17a61b87-5132-497c-b928-421ac2ee0c8a" name="/dev/vfio/vfio" pid=8486 comm="qemu-system-x86" requested_mask="wr" denied_mask="wr" fsuid=64055 ouid=0 Bug-Ubuntu: https://bugs.launchpad.net/bugs/1678322 Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 3 +++ 1 file changed, 3 insertions(+) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 5d811f9..eb4d58c 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -212,3 +212,6 @@ # silence refusals to open lttng files (see LP: #1432644) deny /dev/shm/lttng-ust-wait-* r, deny /run/shm/lttng-ust-wait-* r, + + # for vfio (LP: #1678322) + /dev/vfio/vfio rw,

On Tue, Dec 19, 2017 at 5:26 PM, Jamie Strandboge <jamie(a)canonical.com&gt; wrote: > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: >> vfio devices are generated on the fly, but the generic base is >> missing. >> >> The base vfio has not much functionality but to provide a custom >> container by opening this path. >> See https://www.kernel.org/doc/Documentation/vfio.txt for more. >> >> Current access by qemu is "wr": >> [ 2652.756712] audit: type=1400 audit(1491303691.719:25): >> apparmor="DENIED" operation="open" >> profile="libvirt-17a61b87-5132-497c-b928-421ac2ee0c8a" >> name="/dev/vfio/vfio" pid=8486 comm="qemu-system-x86" >> requested_mask="wr" denied_mask="wr" fsuid=64055 ouid=0 >> >> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1678322 >> >> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; >> Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; >> --- >> examples/apparmor/libvirt-qemu | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/examples/apparmor/libvirt-qemu >> b/examples/apparmor/libvirt-qemu >> index 5d811f9..eb4d58c 100644 >> --- a/examples/apparmor/libvirt-qemu >> +++ b/examples/apparmor/libvirt-qemu >> @@ -212,3 +212,6 @@ >> # silence refusals to open lttng files (see LP: #1432644) >> deny /dev/shm/lttng-ust-wait-* r, >> deny /run/shm/lttng-ust-wait-* r, >> + >> + # for vfio (LP: #1678322) >> + /dev/vfio/vfio rw, > > Why not just also add this rule iff there is a vfio-specific device > rule? Ie, just add this along with the vfio device rule in virt-aa- > helper instead of giving all VMs access when it isn't needed. Good suggestion, but I realized that - other than the per-device rules that I knew - we already do so for the base dev as well [1]. Thanks for making me aware. One thing that puzzles me still is that I hit this in 2017 on a rather new libvirt. Maybe on that case the detection was broken and thereby the rule missing - I'll go back and take a look again and might come up with a fix for that logic then. TL;DR - this is just old garbage, I'm dropping this on my end - yay cleanup !

[1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h= 74e86b6b2521881808bb93290bcebcb469ab7820 > -- > Jamie Strandboge | http://www.canonical.com

While libvirtd might do so, qemu itself as a guest will not need to call qemu-nbd so remove it from the profile. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/libvirt-qemu | 1 - 1 file changed, 1 deletion(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index eb4d58c..2bffb04 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -144,7 +144,6 @@ /usr/bin/qemu-mipsel rmix, /usr/bin/qemu-mipsn32 rmix, /usr/bin/qemu-mipsn32el rmix, - /usr/bin/qemu-nbd rmix, /usr/bin/qemu-or32 rmix, /usr/bin/qemu-ppc rmix, /usr/bin/qemu-ppc64 rmix,

From: Jamie Strandboge <jamie(a)ubuntu.com&gt; This is required for the ebtables functionality added in libvirt 0.8.0. Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/usr.sbin.libvirtd | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 8d61d15..2b6b33a 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -76,6 +76,10 @@ /usr/{lib,lib64}/xen/bin/* Ux, /usr/lib/xen-*/bin/libxl-save-helper PUx, + # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to + # write and run an ebtables script. + /var/lib/libvirt/virtd* ixr, +

From: Jamie Strandboge <jamie(a)ubuntu.com&gt; Bug-Ubuntu: https://bugs.launchpad.net/bugs/591769 Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index bd6181d..d63c844 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -47,6 +47,10 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { audit deny @{HOME}/bin/** mrwkl, @{HOME}/ r, @{HOME}/** r, + # Alow access to ecryptfs files (LP: #591769) + @{HOME}/.Private/** mrwlk, + @{HOMEDIRS}/.ecryptfs/*/.Private/** mrwlk, +

From: Jamie Strandboge <jamie(a)ubuntu.com&gt; Required to generate correct profiles when using usb passthrough. Bug-Ubuntu: https://bugs.launchpad.net/bugs/565691 Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index d63c844..de92291 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -22,6 +22,8 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { # for hostdev /sys/devices/ r, /sys/devices/** r, + /sys/bus/usb/devices/ r, + /sys/bus/usb/devices/** r, deny /dev/sd* r, deny /dev/vd* r, deny /dev/dm-* r,