Up until this point, we have avoided setting labels for
incoming migration when the TPM state is stored on a shared
filesystem. This seems to make sense, because since the
underlying storage is shared surely the labels will be as
well.

There's one problem, though: when a guest is migrated, the
SELinux context for the destination process is different from
the one of the source process.

We haven't hit any issues with the current approach so far
because NFS doesn't support SELinux, so effectively it doesn't
matter whether relabeling happens or not: even if the SELinux
contexts of the source and target processes are different,
both will be able to access the storage.

Now that it's possible for the local admin to manually mark
exported directories as shared filesystems, however, things
can get problematic.

Consider the case in which one host (mig-one) exports its
local filesystem /srv/nfs/libvirt/swtpm via NFS, and at the
same time bind-mounts it to /var/lib/libvirt/swtpm; another
host (mig-two) mounts the same filesystem to the same
location, this time via NFS. Additionally, in order to
allow migration in both directions, on mig-one the
/var/lib/libvirt/swtpm directory is listed in the
shared_filesystems qemu.conf option.

When migrating from mig-one to mig-two, things work just fine;
going in the opposite direction, however, results in an error:

  # virsh migrate cirros qemu+ssh://mig-one/system
  error: internal error: QEMU unexpectedly closed the monitor (vm='cirros'):
  qemu-system-x86_64: tpm-emulator: Setting the stateblob (type 1) failed with a TPM error 0x1f
  qemu-system-x86_64: error while loading state for instance 0x0 of device 'tpm-emulator'
  qemu-system-x86_64: load of migration failed: Input/output error

This is because the directory on mig-one is considered a
shared filesystem and thus labeling is skipped, resulting in
a SELinux denial.

The solution is quite simple: remove the check and always
relabel. We know that it's okay to do so not just because it
makes the error seen above go away, but also because no such
check currently exists for disks and other types of persistent
storage such as NVRAM files, which always get relabeled.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/qemu/qemu_tpm.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index cdf4bfbad2..e5a9fb8b16 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -929,7 +929,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
     g_autofree char *pidfile = NULL;
     virTimeBackOffVar timebackoff;
     const unsigned long long timeout = 1000; /* ms */
-    bool setTPMStateLabel = true;
     pid_t pid = -1;
 
     cfg = virQEMUDriverGetConfig(driver);
@@ -956,13 +955,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
     virCommandSetPidFile(cmd, pidfile);
     virCommandSetErrorFD(cmd, &errfd);
 
-    if (incomingMigration &&
-        virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems) == 1) {
-        /* security labels must have been set up on source already */
-        setTPMStateLabel = false;
-    }
-
-    if (qemuSecuritySetTPMLabels(driver, vm, setTPMStateLabel) < 0)
+    if (qemuSecuritySetTPMLabels(driver, vm, true) < 0)
         return -1;
 
     if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user,
@@ -1011,7 +1004,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
         virProcessKillPainfully(pid, true);
     if (pidfile)
         unlink(pidfile);
-    qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel);
+    qemuSecurityRestoreTPMLabels(driver, vm, true);
     return -1;
 }
 
-- 
2.44.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, May 02, 2024 at 19:39:42 +0200, Andrea Bolognani wrote:
> Up until this point, we have avoided setting labels for
> incoming migration when the TPM state is stored on a shared
> filesystem. This seems to make sense, because since the
> underlying storage is shared surely the labels will be as
> well.
> 
> There's one problem, though: when a guest is migrated, the
> SELinux context for the destination process is different from
> the one of the source process.
> 
> We haven't hit any issues with the current approach so far
> because NFS doesn't support SELinux, so effectively it doesn't
> matter whether relabeling happens or not: even if the SELinux
> contexts of the source and target processes are different,
> both will be able to access the storage.
> 
> Now that it's possible for the local admin to manually mark
> exported directories as shared filesystems, however, things
> can get problematic.
> 
> Consider the case in which one host (mig-one) exports its
> local filesystem /srv/nfs/libvirt/swtpm via NFS, and at the
> same time bind-mounts it to /var/lib/libvirt/swtpm; another
> host (mig-two) mounts the same filesystem to the same
> location, this time via NFS. Additionally, in order to
> allow migration in both directions, on mig-one the
> /var/lib/libvirt/swtpm directory is listed in the
> shared_filesystems qemu.conf option.
> 
> When migrating from mig-one to mig-two, things work just fine;
> going in the opposite direction, however, results in an error:
> 
>   # virsh migrate cirros qemu+ssh://mig-one/system
>   error: internal error: QEMU unexpectedly closed the monitor (vm='cirros'):
>   qemu-system-x86_64: tpm-emulator: Setting the stateblob (type 1) failed with a TPM error 0x1f
>   qemu-system-x86_64: error while loading state for instance 0x0 of device 'tpm-emulator'
>   qemu-system-x86_64: load of migration failed: Input/output error
> 
> This is because the directory on mig-one is considered a
> shared filesystem and thus labeling is skipped, resulting in
> a SELinux denial.
> 
> The solution is quite simple: remove the check and always
> relabel. We know that it's okay to do so not just because it
> makes the error seen above go away, but also because no such
> check currently exists for disks and other types of persistent
> storage such as NVRAM files, which always get relabeled.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  src/qemu/qemu_tpm.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org