[PATCH 0/4] qemu_tmp: Generate log file path for emulated TPMs more frequently
The real fix is in 2/4 and the rest is just cleanups. Long story short, the path to log file is not stored anywhere and thus is forgotten on libvirtd restart which means it's seclabel is not restored properly. Michal Prívozník (4): qemu_tpm: Move logfile path generation into a separate function qemu_tpm: Generate log file path among with storage path qemu_tpm: Drop @logDir argument from qemuTPMEmulatorPrepareHost() virtmp: Fix @path handling in virTPMEmulatorInit() src/qemu/qemu_extdevice.c | 6 +++--- src/qemu/qemu_tpm.c | 45 +++++++++++++++++++++++++++++---------- src/util/virtpm.c | 4 +--- 3 files changed, 38 insertions(+), 17 deletions(-) -- 2.26.2
Strictly not needed, but the rest of paths is generated in separate functions. Helps with code readability. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index f94cad8230..71339b785a 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -82,6 +82,21 @@ qemuTPMCreateEmulatorStoragePath(const char *swtpmStorageDir, } +/** + * qemuTPMCreateEmulatorLogPath: + * @logDir: directory where swtpm writes its logs into + * @vmname: name of the VM + * + * Create the swtpm's log path. + */ +static char* +qemuTPMCreateEmulatorLogPath(const char *logDir, + const char *vmname) +{ + return g_strdup_printf("%s/%s-swtpm.log", logDir, vmname); +} + + /* * qemuTPMEmulatorInitStorage * @@ -286,7 +301,7 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, /* create logfile name ... */ if (!tpm->data.emulator.logfile) - tpm->data.emulator.logfile = g_strdup_printf("%s/%s-swtpm.log", logDir, vmname); + tpm->data.emulator.logfile = qemuTPMCreateEmulatorLogPath(logDir, vmname); if (!virFileExists(tpm->data.emulator.logfile) && virFileTouch(tpm->data.emulator.logfile, 0644) < 0) { -- 2.26.2
On a Monday in 2021, Michal Privoznik wrote:
Strictly not needed, but the rest of paths is generated in separate functions. Helps with code readability.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When starting a guest with TPM of type='emulator' an external process is started with it (swtmp) to emulat TPM. This external 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 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, 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@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))) + 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); - 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
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@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@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. :)
After previous commit, the log directory doesn't have to be passed as an extra argument but can be deducted from logfile path. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index b11c5474a5..5ee5efa6b0 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -274,7 +274,6 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, * qemuTPMEmulatorPrepareHost: * * @tpm: tpm definition - * @logDir: directory where swtpm writes its logs into * @swtpm_user: uid to run the swtpm with * @swtpm_group: gid to run the swtpm with * @swtpmStateDir: directory for swtpm's persistent state @@ -287,16 +286,19 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, */ static int qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, - const char *logDir, uid_t swtpm_user, gid_t swtpm_group, const char *swtpmStateDir, uid_t qemu_user, const char *shortName) { + g_autofree char *logDir = NULL; + if (virTPMEmulatorInit() < 0) return -1; + logDir = g_path_get_dirname(tpm->data.emulator.logfile); + /* create log dir ... allow 'tss' user to cd into it */ if (virFileMakePathWithMode(logDir, 0711) < 0) return -1; @@ -732,7 +734,7 @@ qemuExtTPMPrepareHost(virQEMUDriverPtr driver, if (!shortName) return -1; - return qemuTPMEmulatorPrepareHost(def->tpms[i], cfg->swtpmLogDir, + return qemuTPMEmulatorPrepareHost(def->tpms[i], cfg->swtpm_user, cfg->swtpm_group, cfg->swtpmStateDir, cfg->user, -- 2.26.2
On a Monday in 2021, Michal Privoznik wrote:
After previous commit, the log directory doesn't have to be passed as an extra argument but can be deducted from logfile path.
I'm not convinced this is an improvement. From the execution point of view it's one passed argument vs finding a slash and copying a string, so extra work. And for code clarity, the new version adds more lines and obscures the fact that logDir is configured per-host, not per-vm. If you feel qemuTPMEmulatorPrepareHost has too many arguments, you can pass cfg. Jano
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index b11c5474a5..5ee5efa6b0 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -274,7 +274,6 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, * qemuTPMEmulatorPrepareHost: * * @tpm: tpm definition - * @logDir: directory where swtpm writes its logs into * @swtpm_user: uid to run the swtpm with * @swtpm_group: gid to run the swtpm with * @swtpmStateDir: directory for swtpm's persistent state @@ -287,16 +286,19 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, */ static int qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, - const char *logDir, uid_t swtpm_user, gid_t swtpm_group, const char *swtpmStateDir, uid_t qemu_user, const char *shortName) { + g_autofree char *logDir = NULL; + if (virTPMEmulatorInit() < 0) return -1;
+ logDir = g_path_get_dirname(tpm->data.emulator.logfile); + /* create log dir ... allow 'tss' user to cd into it */ if (virFileMakePathWithMode(logDir, 0711) < 0) return -1; @@ -732,7 +734,7 @@ qemuExtTPMPrepareHost(virQEMUDriverPtr driver, if (!shortName) return -1;
- return qemuTPMEmulatorPrepareHost(def->tpms[i], cfg->swtpmLogDir, + return qemuTPMEmulatorPrepareHost(def->tpms[i], cfg->swtpm_user, cfg->swtpm_group, cfg->swtpmStateDir, cfg->user, -- 2.26.2
This function finds "swtmp", "swtpm_setup" and "swtpm_ioctl" binaries in $PATH and stores resolved paths in global variables so that they can be obtainer later. Anyway, the resolved path is marked as g_autofree and to avoid its freeing later on in the function the variable is set to NULL manually. Well, we have g_steal_pointer() for thath. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virtpm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/virtpm.c b/src/util/virtpm.c index b41eb00619..cd860140d3 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -323,16 +323,14 @@ virTPMEmulatorInit(void) _("Could not stat %s"), path); goto cleanup; } - *prgs[i].path = path; + *prgs[i].path = g_steal_pointer(&path); if (prgs[i].caps) { *prgs[i].caps = virTPMGetCaps(prgs[i].typeFromStringFn, path, prgs[i].parm); - path = NULL; if (!*prgs[i].caps) goto cleanup; } - path = NULL; } } -- 2.26.2
*tpm On a Monday in 2021, Michal Privoznik wrote:
This function finds "swtmp", "swtpm_setup" and "swtpm_ioctl" binaries in $PATH and stores resolved paths in global variables so that they can be obtainer later. Anyway, the resolved path is marked as g_autofree and to avoid its freeing later on in the function the variable is set to NULL manually. Well, we have g_steal_pointer() for thath.
*that
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virtpm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/util/virtpm.c b/src/util/virtpm.c index b41eb00619..cd860140d3 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -323,16 +323,14 @@ virTPMEmulatorInit(void) _("Could not stat %s"), path); goto cleanup; } - *prgs[i].path = path; + *prgs[i].path = g_steal_pointer(&path);
if (prgs[i].caps) { *prgs[i].caps = virTPMGetCaps(prgs[i].typeFromStringFn, path, prgs[i].parm);
The path is used here --------------------------^^^^
- path = NULL; if (!*prgs[i].caps) goto cleanup; } - path = NULL; } }
With that fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik