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

Christian Ehrhardt christian.ehrhardt at canonical.com
Thu Dec 21 16:49:10 UTC 2017


On Wed, Dec 20, 2017 at 3:34 PM, Jamie Strandboge <jamie at canonical.com> 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 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.
>

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




More information about the libvir-list mailing list