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

On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa(a)redhat.com&gt; 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(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. 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?