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

[Jamie Strandboge]

On Wed, 2017-12-20 at 14:43 +0100, Christian Ehrhardt wrote:
> On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie at canonical.co
> m> wrote:
> > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> > > From: Serge Hallyn <serge.hallyn at ubuntu.com>
> > > 
> > > 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 at canonical.com>
> > > ---
> > >  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171220/83bc5118/attachment-0001.sig>