On Wed, Dec 20, 2017 at 3:34 PM, Jamie Strandboge <jamie@xxxxxxxxxxxxx> wrote: > On Wed, 2017-12-20 at 14:43 +0100, Christian Ehrhardt wrote: >> On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie@xxxxxxxxxxxx >> m> wrote: >> > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: >> > > From: Serge Hallyn <serge.hallyn@xxxxxxxxxx> >> > > >> > > 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@xxxxxxxxxxxxx> >> > > --- >> > > 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. > I work on #1 a while and I think we can do a lot good here. Yet while I'm convinced at the changes this is currently a debugging nightmare. I guess it wants to become a 2018 submission. So for now keep this hugepage patch out of consideration when looking at applying all those with many +1's. I'll hopefully come back somewhen in 2018 with a dynamic handling of hugepages (and several more actually). > -- > Jamie Strandboge | http://www.canonical.com -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
