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

Peter Krempa pkrempa at redhat.com
Tue Jan 19 10:43:40 UTC 2021


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 at 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 at redhat.com>


> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static int
>  AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>                         virDomainDefPtr def)
> -- 
> 2.30.0
> 




More information about the libvir-list mailing list