[PATCH 0/7] qemu: Cleanup code around TPM seclabels
*** BLURB HERE *** Michal Prívozník (7): qemu_security: Rework qemuSecurityCleanupTPMEmulator() qemu_security: Rename qemuSecurityCleanupTPMEmulator() qemu_security: Introduce qemuSecuritySetTPMLabels() qemu_tpm: Restore TPM labels on failed start qemu_tpm: Open code qemuSecurityStartTPMEmulator() qemu_security: Drop qemuSecurityStartTPMEmulator() docs: Recommend static seclabels for migration on shared storage docs/drvqemu.rst | 7 ++++ src/qemu/qemu_security.c | 90 ++++++++++------------------------------ src/qemu/qemu_security.h | 17 +++----- src/qemu/qemu_tpm.c | 15 ++++--- 4 files changed, 43 insertions(+), 86 deletions(-) -- 2.38.2
Currently, qemuSecurityCleanupTPMEmulator() returns nothing which means a caller (well, there's only one - qemuExtTPMStop()) can't produce a warning when restoring seclabels on TPM state failed. True, qemuSecurityCleanupTPMEmulator() does report a warning itself, but only in one specific error path. Make the function return an integer, just like the rest of qemuSecurity*Restore() functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 21 ++++++++++++--------- src/qemu/qemu_security.h | 6 +++--- src/qemu/qemu_tpm.c | 3 ++- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index def4061488..a0b78764e5 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -576,26 +576,29 @@ qemuSecurityStartTPMEmulator(virQEMUDriver *driver, } -void +int qemuSecurityCleanupTPMEmulator(virQEMUDriver *driver, virDomainObj *vm, bool restoreTPMStateLabel) { qemuDomainObjPrivate *priv = vm->privateData; - bool transactionStarted = false; + int ret = -1; - if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) - transactionStarted = true; + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; - virSecurityManagerRestoreTPMLabels(driver->securityManager, - vm->def, restoreTPMStateLabel); + if (virSecurityManagerRestoreTPMLabels(driver->securityManager, + vm->def, restoreTPMStateLabel) < 0) + goto cleanup; - if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, + if (virSecurityManagerTransactionCommit(driver->securityManager, -1, priv->rememberOwner) < 0) - VIR_WARN("Unable to run security manager transaction"); + goto cleanup; + ret = 0; + cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + return ret; } diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 969a47fc17..0b19f48ef2 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -94,9 +94,9 @@ int qemuSecurityStartTPMEmulator(virQEMUDriver *driver, int *exitstatus, int *cmdret); -void qemuSecurityCleanupTPMEmulator(virQEMUDriver *driver, - virDomainObj *vm, - bool restoreTPMStateLabel); +int qemuSecurityCleanupTPMEmulator(virQEMUDriver *driver, + virDomainObj *vm, + bool restoreTPMStateLabel); int qemuSecuritySetSavedStateLabel(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index f2edaf5eaa..8778d43913 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -1143,7 +1143,8 @@ qemuExtTPMStop(virQEMUDriver *driver, if (outgoingMigration || qemuTPMHasSharedStorage(vm->def)) restoreTPMStateLabel = false; - qemuSecurityCleanupTPMEmulator(driver, vm, restoreTPMStateLabel); + if (qemuSecurityCleanupTPMEmulator(driver, vm, restoreTPMStateLabel) < 0) + VIR_WARN("Unable to restore labels on TPM state and/or log file"); } -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:51 +0100, Michal Privoznik wrote:
Currently, qemuSecurityCleanupTPMEmulator() returns nothing which means a caller (well, there's only one - qemuExtTPMStop()) can't produce a warning when restoring seclabels on TPM state failed. True, qemuSecurityCleanupTPMEmulator() does report a warning itself, but only in one specific error path.
Make the function return an integer, just like the rest of qemuSecurity*Restore() functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 21 ++++++++++++--------- src/qemu/qemu_security.h | 6 +++--- src/qemu/qemu_tpm.c | 3 ++- 3 files changed, 17 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The qemuSecurityCleanupTPMEmulator() function calls virSecurityManagerRestoreTPMLabels() and thus the proper name is qemuSecurityRestoreTPMLabels(). Rename it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 6 +++--- src/qemu/qemu_security.h | 6 +++--- src/qemu/qemu_tpm.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index a0b78764e5..82d686b0e3 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -577,9 +577,9 @@ qemuSecurityStartTPMEmulator(virQEMUDriver *driver, int -qemuSecurityCleanupTPMEmulator(virQEMUDriver *driver, - virDomainObj *vm, - bool restoreTPMStateLabel) +qemuSecurityRestoreTPMLabels(virQEMUDriver *driver, + virDomainObj *vm, + bool restoreTPMStateLabel) { qemuDomainObjPrivate *priv = vm->privateData; int ret = -1; diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 0b19f48ef2..b6f917a62f 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -94,9 +94,9 @@ int qemuSecurityStartTPMEmulator(virQEMUDriver *driver, int *exitstatus, int *cmdret); -int qemuSecurityCleanupTPMEmulator(virQEMUDriver *driver, - virDomainObj *vm, - bool restoreTPMStateLabel); +int qemuSecurityRestoreTPMLabels(virQEMUDriver *driver, + virDomainObj *vm, + bool restoreTPMStateLabel); int qemuSecuritySetSavedStateLabel(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 8778d43913..200ff0de6f 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -1143,7 +1143,7 @@ qemuExtTPMStop(virQEMUDriver *driver, if (outgoingMigration || qemuTPMHasSharedStorage(vm->def)) restoreTPMStateLabel = false; - if (qemuSecurityCleanupTPMEmulator(driver, vm, restoreTPMStateLabel) < 0) + if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel) < 0) VIR_WARN("Unable to restore labels on TPM state and/or log file"); } -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:52 +0100, Michal Privoznik wrote:
The qemuSecurityCleanupTPMEmulator() function calls virSecurityManagerRestoreTPMLabels() and thus the proper name is qemuSecurityRestoreTPMLabels(). Rename it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 6 +++--- src/qemu/qemu_security.h | 6 +++--- src/qemu/qemu_tpm.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that we have qemuSecurityRestoreTPMLabels() we might as well have qemuSecuritySetTPMLabels(). The aim here is to remove qemuSecurityStartTPMEmulator() which couples two separate things into a single function call. Therefore, introduce qemuSecuritySetTPMLabels() which does only set seclabels on the TPM state. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 26 ++++++++++++++++++++++++++ src/qemu/qemu_security.h | 4 ++++ 2 files changed, 30 insertions(+) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 82d686b0e3..daf01bb803 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -576,6 +576,32 @@ qemuSecurityStartTPMEmulator(virQEMUDriver *driver, } +int +qemuSecuritySetTPMLabels(virQEMUDriver *driver, + virDomainObj *vm, + bool setTPMStateLabel) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int ret = -1; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; + + if (virSecurityManagerSetTPMLabels(driver->securityManager, + vm->def, setTPMStateLabel) < 0) + goto cleanup; + + if (virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; +} + + int qemuSecurityRestoreTPMLabels(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index b6f917a62f..198f8ef0d4 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -94,6 +94,10 @@ int qemuSecurityStartTPMEmulator(virQEMUDriver *driver, int *exitstatus, int *cmdret); +int qemuSecuritySetTPMLabels(virQEMUDriver *driver, + virDomainObj *vm, + bool setTPMStateLabel); + int qemuSecurityRestoreTPMLabels(virQEMUDriver *driver, virDomainObj *vm, bool restoreTPMStateLabel); -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:53 +0100, Michal Privoznik wrote:
Now that we have qemuSecurityRestoreTPMLabels() we might as well have qemuSecuritySetTPMLabels(). The aim here is to remove qemuSecurityStartTPMEmulator() which couples two separate things into a single function call.
Therefore, introduce qemuSecuritySetTPMLabels() which does only set seclabels on the TPM state.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 26 ++++++++++++++++++++++++++ src/qemu/qemu_security.h | 4 ++++ 2 files changed, 30 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
If swtpm binary fails to start after successful exec() (e.g. it fails to initialize itself), the seclabels set in qemuSecurityStartTPMEmulator() are not restored. This is due to lacking qemuSecurityRestoreTPMLabels() call in the error path. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 200ff0de6f..03055002cb 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -927,6 +927,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virTimeBackOffVar timebackoff; const unsigned long long timeout = 1000; /* ms */ bool setTPMStateLabel = true; + bool teardownlabel = false; int cmdret = 0; pid_t pid = -1; @@ -970,6 +971,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, * already reported error. */ goto error; } + teardownlabel = true; if (virPidFileReadPath(pidfile, &pid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1012,6 +1014,8 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); + if (teardownlabel) + qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel); return -1; } -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:54 +0100, Michal Privoznik wrote:
If swtpm binary fails to start after successful exec() (e.g. it fails to initialize itself), the seclabels set in qemuSecurityStartTPMEmulator() are not restored. This is due to lacking qemuSecurityRestoreTPMLabels() call in the error path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When starting swtpm binary, the qemuSecurityStartTPMEmulator() is called which sets seclabel on the TPM state and then uses qemuSecurityCommandRun() to execute the swtpm binary with proper seclabel. Well, the aim is to ditch qemuSecurityStartTPMEmulator() because it entangles two distinct operations. Just call functions for them separately. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 03055002cb..b2748eb6a4 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -927,7 +927,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virTimeBackOffVar timebackoff; const unsigned long long timeout = 1000; /* ms */ bool setTPMStateLabel = true; - bool teardownlabel = false; int cmdret = 0; pid_t pid = -1; @@ -960,18 +959,18 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, setTPMStateLabel = false; } - if (qemuSecurityStartTPMEmulator(driver, vm, cmd, - cfg->swtpm_user, cfg->swtpm_group, - setTPMStateLabel, NULL, &cmdret) < 0) { + if (qemuSecuritySetTPMLabels(driver, vm, setTPMStateLabel) < 0) + return -1; + + if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user, + cfg->swtpm_group, NULL, &cmdret) < 0) goto error; - } if (cmdret < 0) { - /* virCommandRun() hidden in qemuSecurityStartTPMEmulator() + /* virCommandRun() hidden in qemuSecurityCommandRun() * already reported error. */ goto error; } - teardownlabel = true; if (virPidFileReadPath(pidfile, &pid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1014,8 +1013,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - if (teardownlabel) - qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel); + qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel); return -1; } -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:55 +0100, Michal Privoznik wrote:
When starting swtpm binary, the qemuSecurityStartTPMEmulator() is called which sets seclabel on the TPM state and then uses qemuSecurityCommandRun() to execute the swtpm binary with proper seclabel. Well, the aim is to ditch qemuSecurityStartTPMEmulator() because it entangles two distinct operations. Just call functions for them separately.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous cleanup this function is no longer used and thus can be dropped. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 77 ---------------------------------------- src/qemu/qemu_security.h | 9 ----- 2 files changed, 86 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index daf01bb803..beada669f7 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -499,83 +499,6 @@ qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver, } -/* - * qemuSecurityStartTPMEmulator: - * - * @driver: the QEMU driver - * @vm: the domain object - * @cmd: the command to run - * @uid: the uid to run the emulator - * @gid: the gid to run the emulator - * @setTPMStateLabel: whether TPM state should be labelled, or just logfile - * @existstatus: pointer to int returning exit status of process - * @cmdret: pointer to int returning result of virCommandRun - * - * Start the TPM emulator with appropriate labels. Apply security - * labels to files first. - * This function returns -1 on security setup error, 0 if all the - * setup was done properly. In case the virCommand failed to run - * 0 is returned but cmdret is set appropriately with the process - * exitstatus also set. - */ -int -qemuSecurityStartTPMEmulator(virQEMUDriver *driver, - virDomainObj *vm, - virCommand *cmd, - uid_t uid, - gid_t gid, - bool setTPMStateLabel, - int *exitstatus, - int *cmdret) -{ - qemuDomainObjPrivate *priv = vm->privateData; - int ret = -1; - bool transactionStarted = false; - - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) - return -1; - transactionStarted = true; - - if (virSecurityManagerSetTPMLabels(driver->securityManager, - vm->def, setTPMStateLabel) < 0) { - virSecurityManagerTransactionAbort(driver->securityManager); - return -1; - } - - if (virSecurityManagerTransactionCommit(driver->securityManager, - -1, priv->rememberOwner) < 0) - goto cleanup_abort; - transactionStarted = false; - - if (qemuSecurityCommandRun(driver, vm, cmd, uid, gid, exitstatus, cmdret) < 0) - goto cleanup; - - ret = 0; - - if (*cmdret < 0) - goto cleanup; - - return 0; - - cleanup: - if (!transactionStarted && - virSecurityManagerTransactionStart(driver->securityManager) >= 0) - transactionStarted = true; - - virSecurityManagerRestoreTPMLabels(driver->securityManager, - vm->def, setTPMStateLabel); - - if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, - -1, priv->rememberOwner) < 0) - VIR_WARN("Unable to run security manager transaction"); - - cleanup_abort: - virSecurityManagerTransactionAbort(driver->securityManager); - return ret; -} - - int qemuSecuritySetTPMLabels(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 198f8ef0d4..8d1c6b38c3 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -85,15 +85,6 @@ int qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver, virDomainObj *vm, virDomainNetDef *net); -int qemuSecurityStartTPMEmulator(virQEMUDriver *driver, - virDomainObj *vm, - virCommand *cmd, - uid_t uid, - gid_t gid, - bool setTPMStateLabel, - int *exitstatus, - int *cmdret); - int qemuSecuritySetTPMLabels(virQEMUDriver *driver, virDomainObj *vm, bool setTPMStateLabel); -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:56 +0100, Michal Privoznik wrote:
After previous cleanup this function is no longer used and thus can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 77 ---------------------------------------- src/qemu/qemu_security.h | 9 ----- 2 files changed, 86 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There are some network FSs (ceph, CIFS) that propagate XATTTs properly and thus SELinux labels too. In such case using dynamic seclabels would get in the way of migration as new seclabel is assigned to the domain on the destination and thus two processes with different labels (the source and the destination QEMU/helper process) would try to access the same file. One of them is necessarily going to be denied access. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/drvqemu.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/drvqemu.rst b/docs/drvqemu.rst index bbd51066a1..fa23912937 100644 --- a/docs/drvqemu.rst +++ b/docs/drvqemu.rst @@ -294,6 +294,13 @@ use the 'context' option when mounting the filesystem to set the default label to ``system_u:object_r:virt_image_t``. In the case of NFS, there is an alternative option, of enabling the ``virt_use_nfs`` SELinux boolean. +There are some network filesystems, however, that propagate SELinux labels +properly, just like a local filesystem (e.g. ceph of CIFS). In such case, +dynamic labelling (described below) might prevent migration of a virtual +machine as new unique SELinux label is assigned to the virtual machine on the +migration destination side. Users are advised to use static labels (``<seclabel +type='static' .../>``). + SELinux sVirt confinement ~~~~~~~~~~~~~~~~~~~~~~~~~ -- 2.38.2
On Wed, Dec 21, 2022 at 08:43:57 +0100, Michal Privoznik wrote:
There are some network FSs (ceph, CIFS) that propagate XATTTs properly and thus SELinux labels too. In such case using dynamic seclabels would get in the way of migration as new seclabel is assigned to the domain on the destination and thus two processes with different labels (the source and the destination QEMU/helper process) would try to access the same file. One of them is necessarily going to be denied access.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/drvqemu.rst | 7 +++++++ 1 file changed, 7 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa