Re: [libvirt] [PATCH 06/12] apparmor, libvirt-qemu: Allow access to hugepage mounts

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