[PATCH] apparmor: let image label setting loop over backing files

[Jim Fehlig]

On 1/20/21 12:33 AM, Christian Ehrhardt wrote:
> On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa at redhat.com> wrote:
>>
>> On Tue, Jan 19, 2021 at 11:23:16 +0100, Christian Ehrhardt wrote:
>>> When adding a rule for an image file and that image file has a chain
>>> of backing files then we need to add a rule for each of those files.
>>>
>>> To get that iterate over the backing file chain the same way as
>>> dac/selinux already do and add a label for each.
>>>
>>> Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118
>>>
>>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
>>> ---
>>>   src/security/security_apparmor.c | 39 ++++++++++++++++++++++----------
>>>   1 file changed, 27 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
>>> index 29f0956d22..1f309c0c9f 100644
>>> --- a/src/security/security_apparmor.c
>>> +++ b/src/security/security_apparmor.c
>>> @@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
>>>
>>>   /* Called when hotplugging */
>>>   static int
>>> -AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>>> -                              virDomainDefPtr def,
>>> -                              virStorageSourcePtr src,
>>> -                              virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
>>> +AppArmorSetSecurityImageLabelInternal(virSecurityManagerPtr mgr,
>>> +                                      virDomainDefPtr def,
>>> +                                      virStorageSourcePtr src)
>>>   {
>>> -    virSecurityLabelDefPtr secdef;
>>>       g_autofree char *vfioGroupDev = NULL;
>>>       const char *path;
>>>
>>> -    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
>>> -    if (!secdef || !secdef->relabel)
>>> -        return 0;
>>> -
>>> -    if (!secdef->imagelabel)
>>> -        return 0;
>>> -
>>>       if (src->type == VIR_STORAGE_TYPE_NVME) {
>>>           const virStorageSourceNVMeDef *nvme = src->nvme;
>>>
>>> @@ -797,6 +788,30 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>>>       return reload_profile(mgr, def, path, true);
>>>   }
>>>
>>> +static int
>>> +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>>> +                              virDomainDefPtr def,
>>> +                              virStorageSourcePtr src,
>>> +                              virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
>>> +{
>>> +    virSecurityLabelDefPtr secdef;
>>> +    virStorageSourcePtr n;
>>> +
>>> +    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
>>> +    if (!secdef || !secdef->relabel)
>>> +        return 0;
>>> +
>>> +    if (!secdef->imagelabel)
>>> +        return 0;
>>
>> So apparmor doesn't support per-image security labels?
>>
>>> +
>>> +    for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
>>> +        if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
>>
>> It feels a bit suboptimal to fork+exec the aahelper for every single
>> image. The selinux/dac drivers collect the list of things to do before
>> forking when we are in the transaction mode (or do just chown/selinux
>> labelling, which is trivial)
>>
>> Given that this is usually on an expensive code path, it's probably okay
>> for now though.
> 
> We are ok with the part above in the thread we have so far.
> But I've realized that I've forgotten Jim on my initial submission.
> 
> @Jim Fehlig any ack/nack/comment on this before I consider pushing it
> now that 7.0 is out?

I took a quick peek and the patch LGTM. Agree that it would be nice to batch the 
calls to virt-aa-helper. I'll make note of it as a potential upstream task, 
although I too struggle to find time for those.

Regards,
Jim