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

[Christian Ehrhardt]

On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie at canonical.com> 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=ce937d37105c2fc95638f4dc0d2993ae5275b464
[2]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=f55afd83b1338e17eae7ec83b792a7fc962edbc3
[3]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=eff2b2edb147572b6d8f701f4b4f4a69580e17c8
[4]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/virt-aa-helper.c;h=07ece730e27d2be4be92adcabb54d788825659f0;hb=HEAD#l1356

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd