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

[Christian Ehrhardt]

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?

> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
>
>
> > +            return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int
> >  AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> >                         virDomainDefPtr def)
> > --
> > 2.30.0
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd