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

[Ján Tomko]

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 at redhat.com>
>---
> 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 at redhat.com>

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. :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210301/eecc64fd/attachment-0001.sig>