Re: [PATCH 2/4] qemu_tpm: Generate log file path among with storage path

Ján Tomko <jtomko@xxxxxxxxxx> · Mon, 1 Mar 2021 18:35:24 +0100

On a Monday in 2021, Michal Privoznik wrote:
When starting a guest with TPM of type='emulator' an external
process is started with it (swtmp) to emulat TPM. This external

*swtpm
*emulate

process is passed path to a log file via --logfile. The path to
the log file is generated in qemuTPMEmulatorPrepareHost() which
works, until the daemon is restarted. The problem is that the
path is not stored in private data or anywhere inside live XML
and thus later, when qemuExtTPMStop() is called (when shutting
off the guest) the stored logpath is NULL and thus it's seclabel


*its

is not cleaned up (see virSecuritySELinuxRestoreTPMLabels()).

Fortunately, qemuExtDevicesStop() (which calls qemuExtTPMStop()
eventually) does call qemuExtDevicesInitPaths() where the log
path can be generated again.

Basically, tpm->data.emulator.storagepath is generated in
qemuExtTPMInitPaths() and it's seclabels are restored properly,

*its

and this commit move logfile onto the same level.

This means, that the log path doesn't have to be generated in
qemuExtDevicesStart() because it was already done in
qemuExtDevicesPrepareHost().

This change also renders @vmname argument of
qemuTPMEmulatorPrepareHost() unused and thus is removed.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1769196
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/qemu/qemu_extdevice.c |  6 +++---
src/qemu/qemu_tpm.c       | 22 ++++++++++++++--------
2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 8fe7ceaa10..fdba22616c 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -132,6 +132,9 @@ qemuExtDevicesPrepareHost(virQEMUDriverPtr driver,
    virDomainDefPtr def = vm->def;
    size_t i;

+    if (qemuExtDevicesInitPaths(driver, def) < 0)
+        return -1;
+
    if (def->ntpms > 0 &&
        qemuExtTPMPrepareHost(driver, def) < 0)
        return -1;
@@ -169,9 +172,6 @@ qemuExtDevicesStart(virQEMUDriverPtr driver,
    virDomainDefPtr def = vm->def;
    size_t i;

-    if (qemuExtDevicesInitPaths(driver, def) < 0)
-        return -1;
-
    for (i = 0; i < def->nvideos; i++) {
        virDomainVideoDefPtr video = def->videos[i];

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 71339b785a..b11c5474a5 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -196,11 +196,15 @@ qemuTPMCreateEmulatorSocket(const char *swtpmStateDir,
 * @tpm: TPM definition for an emulator type
 * @swtpmStorageDir: the general swtpm storage dir which is used as a base
 *                   directory for creating VM specific directories
+ * @logDir: directory where swtpm writes its logs into
+ * @vmname: name of the VM
 * @uuid: the UUID of the VM
 */
static int
qemuTPMEmulatorInitPaths(virDomainTPMDefPtr tpm,
                         const char *swtpmStorageDir,
+                         const char *logDir,
+                         const char *vmname,
                         const unsigned char *uuid)
{
    char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -213,6 +217,11 @@ qemuTPMEmulatorInitPaths(virDomainTPMDefPtr tpm,
                                             tpm->version)))
        return -1;

+    if (!tpm->data.emulator.logfile &&
+        !(tpm->data.emulator.logfile =
+  qemuTPMCreateEmulatorLogPath(logDir, vmname)))

There is no need to check the return value, g_strdup_printf aborts on
OOM [1] and it was not checked in the function you moved it from [0]

+        return -1;
+
    return 0;
}

@@ -266,7 +275,6 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir,
 *
 * @tpm: tpm definition
 * @logDir: directory where swtpm writes its logs into
- * @vmname: name of the VM
 * @swtpm_user: uid to run the swtpm with
 * @swtpm_group: gid to run the swtpm with
 * @swtpmStateDir: directory for swtpm's persistent state
@@ -280,7 +288,6 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir,
static int
qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
                           const char *logDir,
-                           const char *vmname,
                           uid_t swtpm_user,
                           gid_t swtpm_group,
                           const char *swtpmStateDir,
@@ -299,10 +306,6 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
                     VIR_DIR_CREATE_ALLOW_EXIST) < 0)
        return -1;

-    /* create logfile name ... */
-    if (!tpm->data.emulator.logfile)
-        tpm->data.emulator.logfile = qemuTPMCreateEmulatorLogPath(logDir, vmname);
-

[0]

    if (!virFileExists(tpm->data.emulator.logfile) &&
        virFileTouch(tpm->data.emulator.logfile, 0644) < 0) {
        return -1;
@@ -702,7 +705,10 @@ qemuExtTPMInitPaths(virQEMUDriverPtr driver,
        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR)
            continue;

-        return qemuTPMEmulatorInitPaths(def->tpms[i], cfg->swtpmStorageDir,
+        return qemuTPMEmulatorInitPaths(def->tpms[i],
+                                        cfg->swtpmStorageDir,
+                                        cfg->swtpmLogDir,
+                                        def->name,
                                        def->uuid);
    }

@@ -727,7 +733,7 @@ qemuExtTPMPrepareHost(virQEMUDriverPtr driver,
            return -1;

        return qemuTPMEmulatorPrepareHost(def->tpms[i], cfg->swtpmLogDir,
-                                          def->name, cfg->swtpm_user,
+                                          cfg->swtpm_user,
                                          cfg->swtpm_group,
                                          cfg->swtpmStateDir, cfg->user,
                                          shortName);
--
2.26.2


With the check removed and at least one of the typos fixed:

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

[1] as of GLib 2.64, but we have a workaround for earlier versions in
glibcompat.h. Added by you, so I'm just talking to myself here. :)
Attachment:
signature.asc

Description: PGP signature




