On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > > > > + 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
