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

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(a)canonical.com&gt; > --- > 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.

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