[PATCH 2/2] security_selinux: Move shortcut in virSecuritySELinuxSetImageLabelInternal() later

[Peter Krempa]

On Thu, Sep 22, 2022 at 13:40:44 +0200, Michal Privoznik wrote:
> At the beginning of virSecuritySELinuxSetImageLabelInternal()
> there's a check that allows the function return early. In
> previous patch the check was extended to not return early for
> NVMe disks. However, there's no such check in other drivers (DAC,
> AppArmor). Therefore, move the check a couple of line down so
> that the resulting code is at least somewhat similar to the rest
> of secdrivers.

In the DAC driver the (almost) exact check is inside
virSecurityDACSetOwnership, which has an additional argument to override
anything in virStorageSource, thus the commit message is slightly
misleading.

> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_selinux.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index a296cb7613..26c6b281cc 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1818,13 +1818,6 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
>      const char *path = src->path;
>      int ret;
>  
> -    /* Special case NVMe. Per virStorageSourceIsLocalStorage() it's
> -     * considered not local, but we still want the code below to set
> -     * label on VFIO group. */
> -    if (src->type != VIR_STORAGE_TYPE_NVME &&
> -        (!src->path || !virStorageSourceIsLocalStorage(src)))
> -        return 0;
> -
>      secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
>      if (!secdef || !secdef->relabel)
>          return 0;
> @@ -1882,6 +1875,8 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
>              return -1;
>  
>          path = vfioGroupDev;
> +    } else if (!path || !virStorageSourceIsLocalStorage(src)) {
> +        return 0;

The full context of the block related to NVME is:

    /* 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;

        if (!(vfioGroupDev = virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
            return -1;

        path = vfioGroupDev;
    }

    ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember);

    if (ret == 1 && !disk_seclabel) {
        /* If we failed to set a label, but virt_use_nfs let us
         * proceed anyway, then we don't need to relabel later.  */
        disk_seclabel = virSecurityDeviceLabelDefNew(SECURITY_SELINUX_NAME);
        if (!disk_seclabel)
            return -1;
        disk_seclabel->labelskip = true;
        VIR_APPEND_ELEMENT(src->seclabels, src->nseclabels, disk_seclabel);
        ret = 0;
    }

There are two odd things here:

1) The comment above the block is the same as in the DAC driver, but in
fact it makes no sense in the selinux driver as there's no
'chown_callback' in the selinux driver.

2) The code which follows 'virSecuritySELinuxSetFilecon' seems to be
dead as virSecuritySELinuxSetFilecon can't ever return '1'.

This seems to have changed when the security labelling remembering was
added. Specifically 'virSecuritySELinuxSetFileconImpl' does return 1
when the label was not set, but only one of the caller cares about the
return value to skip relabelling.

If that is desired you can:
 1) clean up the code to remove the dead code block
 2) call virSecuritySELinuxSetFilecon directly from the block
    special-casing NVMe disks and return right away.
 3) move the check for local storage below that block
 4) remove the misleading comment above the NVME special case block

That way it will resemble the code in other storage drivers more.