In the case of outgoing migration, we avoid restoring the
remembered labels for the TPM state directory because doing so
would risk cutting off storage access for the target node.

Even in that case though, we should still forget (unref) the
remembered labels: if we don't, the source node will keep
thinking that the state directory is in use.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/security/security_selinux.c | 54 +++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 3e213a553b..4f13d305d9 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -210,6 +210,51 @@ virSecuritySELinuxRecallLabel(const char *path,
 }
 
 
+/**
+ * virSecuritySELinuxForgetLabels:
+ * @path: file or directory to work on
+ *
+ * Forgets rememebered SELinux labels for @path, including its
+ * children if it is a directory.
+ *
+ * This is intended to be used in cleanup paths, so failure to forget
+ * a single label is not considered fatal; instead, a best-effort
+ * attempt to continue and forget as many labels as possible will be
+ * made.
+ *
+ * Returns: 0 on success, <0 on failure
+ */
+static int
+virSecuritySELinuxForgetLabels(const char *path)
+{
+    int ret = 0;
+    struct dirent *ent;
+    g_autoptr(DIR) dir = NULL;
+    g_autofree char *con = NULL;
+
+    if (virSecuritySELinuxRecallLabel(path, &con) < 0)
+        VIR_WARN("Failed to forget remembered SELinux labels for %s, ignoring", path);
+
+    if (!virFileIsDir(path))
+        return 0;
+
+    if (virDirOpen(&dir, path) < 0)
+        return -1;
+
+    while ((ret = virDirRead(dir, &ent, path)) > 0) {
+        g_autofree char *spath = NULL;
+        g_autofree char *scon = NULL;
+
+        spath = g_strdup_printf("%s/%s", path, ent->d_name);
+
+        if (virSecuritySELinuxRecallLabel(spath, &scon) < 0)
+            VIR_WARN("Failed to forget remembered SELinux labels for %s, ignoring", spath);
+    }
+
+    return ret;
+}
+
+
 static int virSecuritySELinuxSetFilecon(virSecurityManager *mgr,
                                         const char *path,
                                         const char *tcon,
@@ -3709,6 +3754,15 @@ virSecuritySELinuxRestoreTPMLabels(virSecurityManager *mgr,
         if (restoreTPMStateLabel) {
             ret = virSecuritySELinuxRestoreFileLabels(mgr,
                                                       def->tpms[i]->data.emulator.storagepath);
+        } else {
+            g_autofree char *oldlabel = NULL;
+
+            /* Even if we're not restoring the original label for the
+             * TPM state directory, we should still forget any
+             * remembered label so that a subsequent attempt at TPM
+             * startup will not fail due to the state directory being
+             * considered as still in use */
+            ignore_value(virSecuritySELinuxForgetLabels(def->tpms[i]->data.emulator.storagepath));
         }
 
         if (ret == 0 &&
-- 
2.46.0

On Fri, Aug 30, 2024 at 17:13:44 +0200, Andrea Bolognani wrote:
> In the case of outgoing migration, we avoid restoring the
> remembered labels for the TPM state directory because doing so
> would risk cutting off storage access for the target node.
> 
> Even in that case though, we should still forget (unref) the
> remembered labels: if we don't, the source node will keep
> thinking that the state directory is in use.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/security/security_selinux.c | 54 +++++++++++++++++++++++++++++++++

I wanted to first complain that it's missing the 'dac' driver counter
part, but weirdly enough the 'dac' security driver is completely missing
the impl for:

 domainSetSecurityTPMLabels and domainRestoreSecurityTPMLabels

Do we assume that the paths for the TPM emulator have always the correct
owner?

>  1 file changed, 54 insertions(+)
> 
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 3e213a553b..4f13d305d9 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -210,6 +210,51 @@ virSecuritySELinuxRecallLabel(const char *path,
>  }
>  
>  
> +/**
> + * virSecuritySELinuxForgetLabels:
> + * @path: file or directory to work on
> + *
> + * Forgets rememebered SELinux labels for @path, including its
> + * children if it is a directory.
> + *
> + * This is intended to be used in cleanup paths, so failure to forget
> + * a single label is not considered fatal; instead, a best-effort
> + * attempt to continue and forget as many labels as possible will be
> + * made.
> + *
> + * Returns: 0 on success, <0 on failure
> + */
> +static int
> +virSecuritySELinuxForgetLabels(const char *path)
> +{
> +    int ret = 0;
> +    struct dirent *ent;
> +    g_autoptr(DIR) dir = NULL;
> +    g_autofree char *con = NULL;
> +
> +    if (virSecuritySELinuxRecallLabel(path, &con) < 0)
> +        VIR_WARN("Failed to forget remembered SELinux labels for %s, ignoring", path);
> +
> +    if (!virFileIsDir(path))
> +        return 0;
> +
> +    if (virDirOpen(&dir, path) < 0)
> +        return -1;
> +
> +    while ((ret = virDirRead(dir, &ent, path)) > 0) {
> +        g_autofree char *spath = NULL;
> +        g_autofree char *scon = NULL;
> +
> +        spath = g_strdup_printf("%s/%s", path, ent->d_name);
> +
> +        if (virSecuritySELinuxRecallLabel(spath, &scon) < 0)
> +            VIR_WARN("Failed to forget remembered SELinux labels for %s, ignoring", spath);
> +    }
> +
> +    return ret;

'ret' is never updated, thus this is 'return 0';

> +}
> +
> +
>  static int virSecuritySELinuxSetFilecon(virSecurityManager *mgr,
>                                          const char *path,
>                                          const char *tcon,
> @@ -3709,6 +3754,15 @@ virSecuritySELinuxRestoreTPMLabels(virSecurityManager *mgr,
>          if (restoreTPMStateLabel) {
>              ret = virSecuritySELinuxRestoreFileLabels(mgr,
>                                                        def->tpms[i]->data.emulator.storagepath);
> +        } else {
> +            g_autofree char *oldlabel = NULL;

This variable is unused.

> +
> +            /* Even if we're not restoring the original label for the
> +             * TPM state directory, we should still forget any
> +             * remembered label so that a subsequent attempt at TPM
> +             * startup will not fail due to the state directory being
> +             * considered as still in use */
> +            ignore_value(virSecuritySELinuxForgetLabels(def->tpms[i]->data.emulator.storagepath));

Why bother returning '-1' from virDirOpen() if you ignore it here. Make
the function void if you don't care.

>          }
>  
>          if (ret == 0 &&


This function has pre-existing very questionable logic in handling
failure:


    for (i = 0; i < def->ntpms; i++) {
        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR)
            continue;

        if (restoreTPMStateLabel) {
            ret = virSecuritySELinuxRestoreFileLabels(mgr,
                                                      def->tpms[i]->data.emulator.storagepath);
        }

        if (ret == 0 &&
            def->tpms[i]->data.emulator.logfile) {
            ret = virSecuritySELinuxRestoreFileLabels(mgr,
                                                      def->tpms[i]->data.emulator.logfile);
        }
    }

So, if 'restoreTPMStateLabel' is true it returns failure only if the
restore of the last TPM's labels has failed and doesn't restore the
labels for just that logfile if that happens.

If restoreTPMStateLabel is false, then if restoring of one logfile's
seclabel failed then, no other ones are restored and failure is
returned.

That makes no sense.

Obviously this is for a different patch, but since you seem to be keen
on fixing labelling for TPMs ...

Thus:

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

if:
 - you fix all the thing's I've pointed out
 - remove the return value from virSecuritySELinuxForgetLabels
 - add a note to the commit message saying that this is doing just
   selinux stuff because the DAC stuff actually isn't implemented.

On Mon, Sep 02, 2024 at 04:55:30PM GMT, Peter Krempa wrote:
> I wanted to first complain that it's missing the 'dac' driver counter
> part, but weirdly enough the 'dac' security driver is completely missing
> the impl for:
>
>  domainSetSecurityTPMLabels and domainRestoreSecurityTPMLabels
>
> Do we assume that the paths for the TPM emulator have always the correct
> owner?

I guess so? I noticed this as well and wanted to look into addressing
this gap, but I was starting to seriously run out of steam by that
point so I decided to leave it alone for now. It doesn't seem to get
in the way in practice.

> This function has pre-existing very questionable logic in handling
> failure:
>
[...]
>
> Obviously this is for a different patch, but since you seem to be keen
> on fixing labelling for TPMs ...

It would be nice to fix this. Just like the above though, it's a
pre-existing issue so it should be okay to address it with a
follow-up series and avoid it holding up this feature further.

In the meantime, I've posted [v7] which should hopefully take care of
all your other concerns.


[v7] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CXPDCLE3QN6VGNZKYBOP2K2UM4TFMH4S/
-- 
Andrea Bolognani / Red Hat / Virtualization