On 1/20/21 12:33 AM, Christian Ehrhardt wrote:
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?
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
