[libvirt] [PATCH 0/2] tpm2: Properly handle a removed logfile
If the swtpm's logfile was removed by the user, we get an error 'no transaction is set' from the security manager (DAC) since the labeling of the file failed the transaction in the commit() phase. In the failure case we will try to remove the label then in the error path and run into another commit() error and overwrite a more useful error message. So in this case we just call the transaction abort function. We also create an empty log file now since swtpm doesn't seem to be able to create one itself. Stefan Stefan Berger (2): tpm: Set transationStarted to false if commit failed tpm: Create empty log file if file was removed src/qemu/qemu_security.c | 6 ++++-- src/qemu/qemu_tpm.c | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) -- 2.21.0
Set the transactionStarted to false if the commit failed. If this is not done, then the failure path will report 'no transaction is set' and hide more useful error reports. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_security.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 87209d3781..3f0d19eba8 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -475,8 +475,9 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, } if (virSecurityManagerTransactionCommit(driver->securityManager, - -1, priv->rememberOwner) < 0) - goto cleanup; + -1, priv->rememberOwner) < 0) { + goto cleanup_abort; + } transactionStarted = false; if (virSecurityManagerSetChildProcessLabel(driver->securityManager, @@ -512,6 +513,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); + cleanup_abort: virSecurityManagerTransactionAbort(driver->securityManager); return ret; } -- 2.21.0
On Fri, Jul 26, 2019 at 11:51:46AM -0400, Stefan Berger wrote:
Set the transactionStarted to false if the commit failed. If this is not done, then the failure path will report 'no transaction is set' and hide more useful error reports.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_security.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 87209d3781..3f0d19eba8 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -475,8 +475,9 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, }
if (virSecurityManagerTransactionCommit(driver->securityManager, - -1, priv->rememberOwner) < 0) - goto cleanup; + -1, priv->rememberOwner) < 0) { + goto cleanup_abort; + } transactionStarted = false;
if (virSecurityManagerSetChildProcessLabel(driver->securityManager, @@ -512,6 +513,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction");
+ cleanup_abort: virSecurityManagerTransactionAbort(driver->securityManager); return ret; }
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Create an empty log file if the log file was removed, otherwise the transaction to set the security labels on the file will fail. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 7efd635831..77ef601f74 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -340,9 +340,13 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, logDir, vmname) < 0) goto cleanup; + if (!virFileExists(tpm->data.emulator.logfile) && + virFileTouch(tpm->data.emulator.logfile, 0644) < 0) { + goto cleanup; + } + /* ... and make sure it can be accessed by swtpm_user */ - if (virFileExists(tpm->data.emulator.logfile) && - chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) { + if (chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) { virReportSystemError(errno, _("Could not chown on swtpm logfile %s"), tpm->data.emulator.logfile); -- 2.21.0
On Fri, Jul 26, 2019 at 11:51:47AM -0400, Stefan Berger wrote:
Create an empty log file if the log file was removed, otherwise the transaction to set the security labels on the file will fail.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 7efd635831..77ef601f74 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -340,9 +340,13 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, logDir, vmname) < 0) goto cleanup;
+ if (!virFileExists(tpm->data.emulator.logfile) && + virFileTouch(tpm->data.emulator.logfile, 0644) < 0) { + goto cleanup; + } + /* ... and make sure it can be accessed by swtpm_user */ - if (virFileExists(tpm->data.emulator.logfile) && - chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) { + if (chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) { virReportSystemError(errno, _("Could not chown on swtpm logfile %s"), tpm->data.emulator.logfile);
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Stefan Berger