Re: [libvirt] [PATCH v3 10/14] qemu: Add support for external swtpm TPM emulator

On 05/04/2018 04:21 PM, Stefan Berger wrote: > This patch adds support for an external swtpm TPM emulator. The XML for > this type of TPM looks as follows: > > <tpm model='tpm-tis'> > <backend type='emulator'/> > </tpm> > > The XML will currently only start a TPM 1.2. > > Upon first start, libvirt will run `swtpm_setup`, which will simulate the > manufacturing of a TPM and create certificates for it and write them into > NVRAM locations of the emulated TPM. > > After that libvirt starts the swtpm TPM emulator using the `swtpm` executable. > > Once the VM terminates, libvirt uses the swtpm_ioctl executable to gracefully > shut down the `swtpm` in case it is still running (QEMU did not send shutdown) > or clean up the socket file. > > The above mentioned executables must be found in the PATH. > > The executables can either be run as root or started as root and switch to > the tss user. The requirement for the tss user comes through 'tcsd', which > is used for the simulation of the manufacturing. Which user is used can be > configured through qemu.conf. By default 'tss' is used. > > The swtpm writes out state into files. The state is kept in /var/lib/libvirt/swtpm: > > [root@localhost libvirt]# ls -lZ | grep swtpm > > drwx--x--x. 7 root root unconfined_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:22 swtpm > > The directory /var/lib/libvirt/swtpm maintains per-TPM state directories. > (Using the uuid of the VM for that since the name can change per VM renaming but > we need a stable directory name.) > > [root@localhost swtpm]# ls -lZ > total 4 > drwx------. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:46 485d0004-a48f-436a-8457-8a3b73e28568 > > [root@localhost 485d0004-a48f-436a-8457-8a3b73e28568]# ls -lZ > total 4 > drwx------. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 10 21:34 tpm1.2 > > [root@localhost tpm1.2]# ls -lZ > total 8 > -rw-r--r--. 1 tss tss system_u:object_r:virt_var_lib_t:s0 3648 Apr 5 16:46 tpm-00.permall > > The directory /var/run/libvirt/qemu/swtpm/ hosts the swtpm.sock that > QEMU uses to communicate with the swtpm: > > root@localhost domain-1-testvm]# ls -lZ > total 0 > srw-------. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 6 10:24 1-testvm-swtpm.sock > > The logfile for the swtpm is in /var/log/swtpm/libvirt/qemu: > > [root@localhost-3 qemu]# ls -lZ > total 4 > -rw-------. 1 tss tss unconfined_u:object_r:var_log_t:s0 2199 Apr 6 14:01 testvm-swtpm.log > > The processes are labeled as follows: > > [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep socket | grep -v grep > system_u:system_r:virtd_t:s0-s0:c0.c1023 tss 18697 0.0 0.0 28172 3892 ? Ss 16:46 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/1-testvm-swtpm.sock,mode=0600 --tpmstate dir=/var/lib/libvirt/swtpm/485d0004-a48f-436a-8457-8a3b73e28568/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log > > [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep > system_u:system_r:svirt_t:s0:c413,c430 qemu 18702 2.5 0.0 3036052 48676 ? Sl 16:46 0:08 /bin/qemu-system-x86_64 [...] > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > src/conf/domain_conf.c | 22 ++++++++++++++++++++++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_command.c | 39 +++++++++++++++++++++++++++++++++------ > src/qemu/qemu_domain.c | 3 +++ > src/qemu/qemu_driver.c | 7 +++++++ > 5 files changed, 66 insertions(+), 6 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index d9945dd..a42574a 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -2593,6 +2593,24 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) > } > } > 2 blank lines and void virDomainTPMDelete(virDomainDefPtr def) > +void virDomainTPMDelete(virDomainDefPtr def) > +{ > + virDomainTPMDefPtr tpm = def->tpm; > + > + if (!tpm) > + return; > + > + switch (tpm->type) { > + case VIR_DOMAIN_TPM_TYPE_EMULATOR: > + virTPMDeleteEmulatorStorage(tpm); > + break; > + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: > + case VIR_DOMAIN_TPM_TYPE_LAST: > + /* nothing to do */ > + break; > + } > +} > + > void virDomainTPMDefFree(virDomainTPMDefPtr def) > { > if (!def) > @@ -27614,6 +27632,10 @@ virDomainDeleteConfig(const char *configDir, > goto cleanup; > } > > + /* in case domain is NOT running, remove any TPM storage */ > + if (!dom->persistent) > + virDomainTPMDelete(dom->def); > + > ret = 0; > > cleanup: > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index eebfc72..e533b95 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -559,6 +559,7 @@ virDomainTimerTrackTypeToString; > virDomainTPMBackendTypeFromString; > virDomainTPMBackendTypeToString; > virDomainTPMDefFree; > +virDomainTPMDelete; > virDomainTPMModelTypeFromString; > virDomainTPMModelTypeToString; > virDomainUSBDeviceDefForeach; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index bb330bf..c02b783 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -9425,21 +9425,31 @@ qemuBuildTPMDevStr(const virDomainDef *def, > > > static char * > -qemuBuildTPMBackendStr(const virDomainDef *def, > +qemuBuildTPMBackendStr(virDomainDef *def, Don't lose the "const"

> virCommandPtr cmd, > virQEMUCapsPtr qemuCaps, > int *tpmfd, > - int *cancelfd) > + int *cancelfd, > + char **chardev) > { > - const virDomainTPMDef *tpm = def->tpm; > + virDomainTPMDef *tpm = def->tpm; Don't lose the "const" > virBuffer buf = VIR_BUFFER_INITIALIZER; > - const char *type = virDomainTPMBackendTypeToString(tpm->type); > + const char *type = NULL; > char *cancel_path = NULL, *devset = NULL; > const char *tpmdev; > > *tpmfd = -1; > *cancelfd = -1; > > + switch (tpm->type) { > + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: > + case VIR_DOMAIN_TPM_TYPE_EMULATOR: > + type = virDomainTPMBackendTypeToString(tpm->type); > + break; > + case VIR_DOMAIN_TPM_TYPE_LAST: default: virReportEnumRangeError(virDomainTPMBackendType, tpm->type); We need some sort of error message otherwise we get failed for some reason which is never fun to diagnose.

> + goto error; > + } > + > virBufferAsprintf(&buf, "%s,id=tpm-%s", type, tpm->info.alias); > > switch (tpm->type) { > @@ -9491,6 +9501,16 @@ qemuBuildTPMBackendStr(const virDomainDef *def, > > break; > case VIR_DOMAIN_TPM_TYPE_EMULATOR: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR)) > + goto no_support; > + > + virBufferAddLit(&buf, ",chardev=chrtpm"); > + > + if (virAsprintf(chardev, "socket,id=chrtpm,path=%s", > + tpm->data.emulator.source.data.nix.path) < 0) > + goto error; > + > + break; > case VIR_DOMAIN_TPM_TYPE_LAST: > goto error; > } > @@ -9517,10 +9537,11 @@ qemuBuildTPMBackendStr(const virDomainDef *def, > > static int > qemuBuildTPMCommandLine(virCommandPtr cmd, > - const virDomainDef *def, > + virDomainDef *def, Don't lose the "const" > virQEMUCapsPtr qemuCaps) > { > char *optstr; > + char *chardev = NULL; > int tpmfd = -1; > int cancelfd = -1; > char *fdset; > @@ -9529,12 +9550,18 @@ qemuBuildTPMCommandLine(virCommandPtr cmd, > return 0; > > if (!(optstr = qemuBuildTPMBackendStr(def, cmd, qemuCaps, > - &tpmfd, &cancelfd))) > + &tpmfd, &cancelfd, > + &chardev))) > return -1; > > virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); > VIR_FREE(optstr); > > + if (chardev) { > + virCommandAddArgList(cmd, "-chardev", chardev, NULL); > + VIR_FREE(chardev); > + } > + > if (tpmfd >= 0) { > fdset = qemuVirCommandGetFDSet(cmd, tpmfd); > if (!fdset) > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index d3eac43..57a82dc 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -34,6 +34,7 @@ > #include "qemu_migration.h" > #include "qemu_migration_params.h" > #include "qemu_security.h" > +#include "qemu_extdevice.h" > #include "viralloc.h" > #include "virlog.h" > #include "virerror.h" > @@ -7166,6 +7167,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, > VIR_WARN("unable to remove snapshot directory %s", snapDir); > VIR_FREE(snapDir); > } > + if (!qemuExtDevicesInitPaths(driver, vm->def)) I know it's more or less functionally equivalent, but it's better to use "if (qemuExtDevicesInitPaths(driver, vm->def) == 0)" since the function is not a boolean or pointer returning function. With suggested adjustments,

Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John > + virDomainTPMDelete(vm->def); > > virObjectRef(vm); > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 9ce97ea..f496f89 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -60,6 +60,7 @@ > #include "qemu_migration_params.h" > #include "qemu_blockjob.h" > #include "qemu_security.h" > +#include "qemu_extdevice.h" > > #include "virerror.h" > #include "virlog.h" > @@ -7349,6 +7350,9 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) > goto endjob; > } > > + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) > + goto endjob; > + > if (qemuDomainObjStart(dom->conn, driver, vm, flags, > QEMU_ASYNC_JOB_START) < 0) > goto endjob; > @@ -7494,6 +7498,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, > if (!(vm = qemuDomObjFromDomain(dom))) > return -1; > > + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) > + return -1; > + > cfg = virQEMUDriverGetConfig(driver); > > if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0) >