1. Patchew
  2. Libvirt
  3. View series

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

Christian Ehrhardt posted 1 patch 3 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210119102316.270452-1-christian.ehrhardt@canonical.com
src/security/security_apparmor.c | 39 ++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 12 deletions(-)
[PATCH] apparmor: let image label setting loop over backing files
Posted by Christian Ehrhardt 3 years, 3 months ago 
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@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;
+
+    for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
+        if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
 static int
 AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
                        virDomainDefPtr def)
-- 
2.30.0
Re: [PATCH] apparmor: let image label setting loop over backing files
Posted by Peter Krempa 3 years, 3 months ago 
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@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.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>


> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static int
>  AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>                         virDomainDefPtr def)
> -- 
> 2.30.0
>
Re: [PATCH] apparmor: let image label setting loop over backing files
Posted by Christian Ehrhardt 3 years, 3 months ago 
On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa@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@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?

This was present before, it just got moved as part of this change.
IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel
and later on only used to check if the struct is ok (if it would be NULL that
would indicate a non initialized element).

If I'm missing some further hidden meaning of "imagelabel" please let me know.

> > +
> > +    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.

I agree, but right now virt-aa-helper has no interface to take multiple
files in one pass.
Furthermore that also calls apparmor_parser which also could be done once.
Furthermore on the "append a rule" use case IMHO aa-helper isn't
doing much of it's original task - it becomes a overly huge "append a line and
call the parser".

I agree that we maybe should batch this. But when we touch it for that - at
least for the "append a rule" use case we might not want to use
virt-aa-helper at all.
But then this becomes a much bigger rewrite with moving e.g. the knowledge how
to get from UUID->filepath into libvirt to be usable from
virt-aa-helper (for other tasks)
but also from the labeling calls. A few more might move, like the
apparmor_parser call.

I've taken a note that it would be good to at least try and POC that, but TBH
that list is growing recently without much draining it :-)

But for the change presented I'd like to keep it to the issue that
we'd want to fix right now.

> 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.

Thanks!

> Reviewed-by: Peter Krempa <pkrempa@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
Re: [PATCH] apparmor: let image label setting loop over backing files
Posted by Peter Krempa 3 years, 3 months ago 
On Tue, Jan 19, 2021 at 12:15:31 +0100, Christian Ehrhardt wrote:
> On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa@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@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,

[...]

> > > +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?
> 
> This was present before, it just got moved as part of this change.
> IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel
> and later on only used to check if the struct is ok (if it would be NULL that
> would indicate a non initialized element).
> 
> If I'm missing some further hidden meaning of "imagelabel" please let me know.

Here secdef->imagelabel is a global per-VM label, but you can configure
a per disk (or rather per-image) label with a <seclabel> element under
<source>. See:

https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms

right the first example.

This allows to control labelling of individual files, but I'm not sure
if such concept makes sense for apparmor. For now it seems to be
ignored, maybe it should be at least validated if it doesn't make sense.
Re: [PATCH] apparmor: let image label setting loop over backing files
Posted by Christian Ehrhardt 3 years, 3 months ago 
On Tue, Jan 19, 2021 at 12:28 PM Peter Krempa <pkrempa@redhat.com> wrote:
>
> On Tue, Jan 19, 2021 at 12:15:31 +0100, Christian Ehrhardt wrote:
> > On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa@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@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,
>
> [...]
>
> > > > +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?
> >
> > This was present before, it just got moved as part of this change.
> > IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel
> > and later on only used to check if the struct is ok (if it would be NULL that
> > would indicate a non initialized element).
> >
> > If I'm missing some further hidden meaning of "imagelabel" please let me know.
>
> Here secdef->imagelabel is a global per-VM label, but you can configure
> a per disk (or rather per-image) label with a <seclabel> element under
> <source>. See:
>
> https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms
>
> right the first example.
>
> This allows to control labelling of individual files, but I'm not sure
> if such concept makes sense for apparmor.

AFAIU the concept would not make much sense for apparmor.
The seclabel/relable config per source are useful to control
dac/selinux and if they put labels on the entity of "a path/file".
But in that sense apparmor isn't controlling attributes of the path at
all, it is more like a whitelist associated to the qemu-process.

> For now it seems to be
> ignored, maybe it should be at least validated if it doesn't make sense.

I don't yet see how it would fit, but I appreciate the discussion -
thanks Peter!


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
Re: [PATCH] apparmor: let image label setting loop over backing files
Posted by Christian Ehrhardt 3 years, 3 months ago 
On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa@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@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@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
Re: [PATCH] apparmor: let image label setting loop over backing files
Posted by Jim Fehlig 3 years, 3 months ago 
On 1/20/21 12:33 AM, Christian Ehrhardt wrote:
> On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa@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@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?

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.