Re: [libvirt] [PATCH v4 06/11] qemu: Extend QEMU with external TPM support

On 05/10/2018 05:57 PM, Stefan Berger wrote: > Implement functions for managing the storage of the external swtpm as well > as starting and stopping it. Also implement functions to use swtpm_setup, > which simulates the manufacturing of a TPM, which includes creation of > certificates for the device. > > Further, the external TPM needs storage on the host that we need to set > up before it can be run. We can clean up the host once the domain is > undefined. > > This patch also implements a small layer for external device support that > calls into the TPM device layer if a domain has an attached TPM. This is > the layer we will wire up later on. > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > src/qemu/Makefile.inc.am | 4 + > src/qemu/qemu_domain.c | 2 + > src/qemu/qemu_driver.c | 5 + > src/qemu/qemu_extdevice.c | 154 ++++++++++ > src/qemu/qemu_extdevice.h | 53 ++++ > src/qemu/qemu_migration.c | 3 + > src/qemu/qemu_process.c | 12 + > src/qemu/qemu_tpm.c | 753 ++++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_tpm.h | 50 +++ > 9 files changed, 1036 insertions(+) > create mode 100644 src/qemu/qemu_extdevice.c > create mode 100644 src/qemu/qemu_extdevice.h > create mode 100644 src/qemu/qemu_tpm.c > create mode 100644 src/qemu/qemu_tpm.h > [...] > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 37876b8..9370de3 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -48,6 +48,7 @@ > #include "qemu_migration_params.h" > #include "qemu_interface.h" > #include "qemu_security.h" > +#include "qemu_extdevice.h" > > #include "cpu/cpu.h" > #include "datatypes.h" > @@ -5872,6 +5873,10 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, > if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0) > goto cleanup; > > + VIR_DEBUG("Preparing external devices"); > + if (qemuExtDevicesPrepareHost(driver, vm->def) < 0) > + goto cleanup; > + > ret = 0; > cleanup: > virObjectUnref(cfg); > @@ -5955,6 +5960,9 @@ qemuProcessLaunch(virConnectPtr conn, > goto cleanup; > logfile = qemuDomainLogContextGetWriteFD(logCtxt); > > + if (qemuExtDevicesStart(driver, vm->def, logCtxt) < 0) > + goto cleanup; > + > VIR_DEBUG("Building emulator command line"); > if (!(cmd = qemuBuildCommandLine(driver, > qemuDomainLogContextGetManager(logCtxt), > @@ -6194,6 +6202,8 @@ qemuProcessLaunch(virConnectPtr conn, > ret = 0; > > cleanup: > + if (ret) if (ret < 0), right?

> + qemuExtDevicesStop(driver, vm->def); > qemuDomainSecretDestroy(vm); > virCommandFree(cmd); > virObjectUnref(logCtxt); > @@ -6614,6 +6624,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, > > qemuDomainCleanupRun(driver, vm); > > + qemuExtDevicesStop(driver, vm->def); > + > /* Stop autodestroy in case guest is restarted */ > qemuProcessAutoDestroyRemove(driver, vm); > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c > new file mode 100644 > index 0000000..024d24d > --- /dev/null > +++ b/src/qemu/qemu_tpm.c > @@ -0,0 +1,753 @@ > +/* > + * qemu_tpm.c: QEMU TPM support > + * > + * Copyright (C) 2018 IBM Corporation > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + * > + * Author: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > + */ > + [...] > +/* > + * qemuTPMCreateEmulatorStoragePath > + * > + * @swtpmStorageDir: directory for swtpm persistent state > + * @vmname: The name of the VM for which to create the storage > + * > + * Create the swtpm's storage path > + */ > +static char * > +qemuTPMCreateEmulatorStoragePath(const char *swtpmStorageDir, > + const char *vmname) You changed at some point to use the uuidstr - probably should change this too. Comment and argument

> +{ > + char *path = NULL; > + > + ignore_value(virAsprintf(&path, "%s/%s/tpm1.2", swtpmStorageDir, vmname)); > + > + return path; > +} > + > + [...] > +/* > + * qemuTPMCreateEmulatorStorage > + * > + * @storagepath: directory for swtpm's pesistent state persistent

> + * @created: a pointer to a bool that will be set to true if the > + * storage was created because it did not exist yet > + * @swtpm_user: The uid that needs to be able to access the directory > + * @swtpm_group: The gid that needs to be able to access the directory > + * > + * Unless the storage path for the swtpm for the given VM > + * already exists, create it and make it accessible for the given userid. > + * Adapt ownership of the directory and all swtpm's state files there. > + */ > +static int > +qemuTPMCreateEmulatorStorage(const char *storagepath, > + bool *created, > + uid_t swtpm_user, > + gid_t swtpm_group) > +{ > + int ret = -1; > + char *swtpmStorageDir = qemuTPMGetTPMStorageDir(storagepath); > + > + if (!swtpmStorageDir) > + return -1; > + > + if (qemuTPMEmulatorInitStorage(swtpmStorageDir) < 0) > + goto cleanup; > + > + *created = false; > + > + if (!virFileExists(storagepath)) > + *created = true; > + > + if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_group, > + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not create directory %s as %u:%d"), > + storagepath, swtpm_user, swtpm_group); > + goto cleanup; > + } > + > + if (virFileChownFiles(storagepath, swtpm_user, swtpm_group) < 0) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + VIR_FREE(swtpmStorageDir); > + > + return ret; > +} > + > + [...] > +/* > + * qemuTPMEmulatorPrepareHost: > + * > + * @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 > + * @qemu_user: uid that qemu will run with; we share the socket file with it > + * @shortName: short and unique name of the domain > + * > + * Prepare the log directory for the swtpm and adjust ownership of it and the > + * log file we will be using. Prepare the state directory where we will share > + * the socket between tss and qemu users. > + */ > +static int > +qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, > + const char *logDir, > + const char *vmname, > + uid_t swtpm_user, > + gid_t swtpm_group, > + const char *swtpmStateDir, > + uid_t qemu_user, > + const char *shortName) > +{ > + int ret = -1; > + > + if (qemuTPMEmulatorInit() < 0) > + return -1; > + > + /* create log dir ... */ > + if (virFileMakePathWithMode(logDir, 0730) < 0) > + return -1; > + > + /* ... and adjust ownership */ > + if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group, > + VIR_DIR_CREATE_ALLOW_EXIST) < 0) > + goto cleanup; Do we really need both? IOW: Why not just virDirCreate

> + > + /* create logfile name ... */ > + if (!tpm->data.emulator.logfile && > + virAsprintf(&tpm->data.emulator.logfile, "%s/%s-swtpm.log", > + logDir, vmname) < 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) { > + virReportSystemError(errno, > + _("Could not chown on swtpm logfile %s"), > + tpm->data.emulator.logfile); > + goto cleanup; > + } > + > + /* > + create our swtpm state dir ... > + - QEMU user needs to be able to access the socket there > + - swtpm group needs to be able to create files there > + - in privileged mode 0570 would be enough, for non-privileged mode > + we need 0770 > + */ > + if (virDirCreate(swtpmStateDir, 0770, qemu_user, swtpm_group, > + VIR_DIR_CREATE_ALLOW_EXIST) < 0) > + goto cleanup; > + > + /* create the socket filename */ > + if (!tpm->data.emulator.source.data.nix.path && > + !(tpm->data.emulator.source.data.nix.path = > + qemuTPMCreateEmulatorSocket(swtpmStateDir, shortName))) > + goto cleanup; > + tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX; > + > + ret = 0; > + > + cleanup: > + > + return ret; > +} [...] > +int > +qemuExtTPMInitPaths(virQEMUDriverPtr driver, > + virDomainDefPtr def) When I see qemuExt - I generally expect the code to be in qemu_extdevice.c... Similar for anything qemuExtTPM. I understand why the Ext is there (e.g. external), but not sure it's necessary.

FWIW: Although I didn't win my argument when last discussed, my feeling is static functions should be something like "tpmEmulatorInitPaths"; whereas, external functions should be qemuTPMEmulatorInitPaths.