[libvirt] [PATCH v3 23/30] virSecuritySELinuxRestoreImageLabelInt: Don't skip non-local storage

[Cole Robinson]

On 12/2/19 9:26 AM, Michal Privoznik wrote:
> This function is currently not called for any type of storage
> source that is not considered 'local' (as defined by
> virStorageSourceIsLocalStorage()). Well, NVMe disks are not
> 'local' from that point of view and therefore we will need to
> call this function more frequently.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_apparmor.c | 25 ++++++++++++----
>  src/security/security_dac.c      | 30 +++++++++++++++++++
>  src/security/security_selinux.c  | 51 +++++++++++++++++++++++++++-----
>  3 files changed, 92 insertions(+), 14 deletions(-)
> 
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 21560b2330..7cea788931 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -779,9 +779,8 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>                                virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
>  {
>      virSecurityLabelDefPtr secdef;
> -
> -    if (!src->path || !virStorageSourceIsLocalStorage(src))
> -        return 0;
> +    g_autofree char *vfioGroupDev = NULL;
> +    const char *path;
>  
>      secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
>      if (!secdef || !secdef->relabel)
> @@ -790,15 +789,29 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>      if (!secdef->imagelabel)
>          return 0;
>  
> +    if (src->type == VIR_STORAGE_TYPE_NVME) {
> +        const virStorageSourceNVMeDef *nvme = src->nvme;
> +
> +        if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> +            return -1;
> +
> +        path = vfioGroupDev;
> +    } else {
> +        if (!src->path || !virStorageSourceIsLocalStorage(src))
> +            return 0;
> +
> +        path = src->path;
> +    }
> +
>      /* if the device doesn't exist, error out */
> -    if (!virFileExists(src->path)) {
> +    if (!virFileExists(path)) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("\'%s\' does not exist"),
> -                       src->path);
> +                       path);
>          return -1;
>      }
>  
> -    return reload_profile(mgr, def, src->path, true);
> +    return reload_profile(mgr, def, path, true);
>  }
>  
>  static int
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index a9a1fad5d7..411aa1b159 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -911,6 +911,19 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
>              return -1;
>      }
>  
> +    /* This is not very clean. But so far we don't have NVMe
> +     * storage pool backend so that its chownCallback would be
> +     * called. And this place looks least offensive. */
> +    if (src->type == VIR_STORAGE_TYPE_NVME) {
> +        const virStorageSourceNVMeDef *nvme = src->nvme;
> +        g_autofree char *vfioGroupDev = NULL;
> +
> +        if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> +            return -1;
> +
> +        return virSecurityDACSetOwnership(mgr, NULL, vfioGroupDev, user, group, false);
> +    }
> +
>      /* We can't do restore on shared resources safely. Not even
>       * with refcounting implemented in XATTRs because if there
>       * was a domain running with the feature turned off the
> @@ -1017,6 +1030,23 @@ virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
>          }
>      }
>  
> +    /* This is not very clean. But so far we don't have NVMe
> +     * storage pool backend so that its chownCallback would be
> +     * called. And this place looks least offensive. */
> +    if (src->type == VIR_STORAGE_TYPE_NVME) {
> +        const virStorageSourceNVMeDef *nvme = src->nvme;
> +        g_autofree char *vfioGroupDev = NULL;
> +
> +        if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> +            return -1;
> +
> +        /* Ideally, we would check if there is not another PCI
> +         * device within domain def that is in the same IOMMU
> +         * group. But we're not doing that for hostdevs yet. */
> +
> +        return virSecurityDACRestoreFileLabelInternal(mgr, NULL, vfioGroupDev, false);
> +    }
> +
>      return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true);
>  }
>  

I think my suggested helpers will simplify selinux and apparmor a bit.
Not sure about DAC but I don't fully understand the chownCallback +
transaction stuff.

Looks mechanically sound AFAICT

Reviewed-by: Cole Robinson <crobinso at redhat.com>

- Cole