On 05/04/2018 04:21 PM, Stefan Berger wrote: > Add 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. > > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> > --- > src/libvirt_private.syms | 5 + > src/util/virtpm.c | 536 ++++++++++++++++++++++++++++++++++++++++++++++- > src/util/virtpm.h | 33 ++- > 3 files changed, 572 insertions(+), 2 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 33fe75b..eebfc72 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2984,6 +2984,11 @@ virTimeStringThenRaw; > > # util/virtpm.h > virTPMCreateCancelPath; > +virTPMDeleteEmulatorStorage; > +virTPMEmulatorBuildCommand; > +virTPMEmulatorInitPaths; > +virTPMEmulatorPrepareHost; > +virTPMEmulatorStop; > > > # util/virtypedparam.h > diff --git a/src/util/virtpm.c b/src/util/virtpm.c > index d5c10da..76bbb21 100644 > --- a/src/util/virtpm.c > +++ b/src/util/virtpm.c > @@ -1,7 +1,7 @@ > /* > * virtpm.c: TPM support > * > - * Copyright (C) 2013 IBM Corporation > + * Copyright (C) 2013,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 > @@ -22,16 +22,36 @@ > > #include <config.h> > > +#include <sys/types.h> > #include <sys/stat.h> > +#include <unistd.h> > +#include <fcntl.h> > +#include <cap-ng.h> > > +#include "conf/domain_conf.h" syntax-check would have told you unsafe cross-directory include - IOW including conf/* files into util/* files is not allowed. So I think you need to rethink where some of these functions will go. I think they are mostly all used by the qemu_extdevice.c changes in patch 9, so perhaps they need to get folded into them. There at least you can grab the conf/domain_conf.h file. > +#include "viralloc.h" syntax-check would have told you not to include "viralloc.h" twice... > +#include "vircommand.h" > #include "virstring.h" > #include "virerror.h" > #include "viralloc.h" > #include "virfile.h" > +#include "virkmod.h" > +#include "virlog.h" > #include "virtpm.h" > +#include "virutil.h" #include "viruuid.h" to get virUUIDGenerate > +#include "configmake.h" > > #define VIR_FROM_THIS VIR_FROM_NONE > > +VIR_LOG_INIT("util.tpm") > + > +/* > + * executables for the swtpm; to be found on the host > + */ > +static char *swtpm_path; > +static char *swtpm_setup; > +static char *swtpm_ioctl; > + There's a love/hate relationship with static/globals... > /** > * virTPMCreateCancelPath: > * @devpath: Path to the TPM device > @@ -74,3 +94,517 @@ virTPMCreateCancelPath(const char *devpath) > cleanup: > return path; > } Two empty lines - pervasive comment here... > + > +/* > + * virTPMEmulatorInit > + * > + * Initialize the Emulator functions by searching for necessary > + * executables that we will use to start and setup the swtpm > + */ > +static int > +virTPMEmulatorInit(void) > +{ > + if (!swtpm_path) { > + swtpm_path = virFindFileInPath("swtpm"); > + if (!swtpm_path) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not find swtpm 'swtpm' in PATH")); The message feels redundant. > + return -1; > + } > + if (!virFileIsExecutable(swtpm_path)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("TPM emulator %s is not an executable"), > + swtpm_path); > + VIR_FREE(swtpm_path); > + return -1; > + } > + } > + > + if (!swtpm_setup) { > + swtpm_setup = virFindFileInPath("swtpm_setup"); > + if (!swtpm_setup) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not find 'swtpm_setup' in PATH")); > + return -1; > + } > + if (!virFileIsExecutable(swtpm_setup)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("'%s' is not an executable"), > + swtpm_setup); > + VIR_FREE(swtpm_setup); > + return -1; > + } > + } > + > + if (!swtpm_ioctl) { > + swtpm_ioctl = virFindFileInPath("swtpm_ioctl"); > + if (!swtpm_ioctl) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not find swtpm_ioctl in PATH")); > + return -1; > + } > + if (!virFileIsExecutable(swtpm_ioctl)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("swtpm_ioctl program %s is not an executable"), > + swtpm_ioctl); > + VIR_FREE(swtpm_ioctl); > + return -1; > + } > + } > + > + return 0; > +} > + > +/* > + * virTPMCreateEmulatorStoragePath > + * > + * @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 * > +virTPMCreateEmulatorStoragePath(const char *swtpmStorageDir, > + const char *vmname) > +{ > + char *path = NULL; > + > + ignore_value(virAsprintf(&path, "%s/%s/tpm1.2", swtpmStorageDir, vmname)); > + > + return path; > +} > + > +/* > + * virtTPMGetTPMStorageDir: > + * > + * @storagepath: directory for swtpm's pesistent state persistent > + * > + * Derive the 'TPMStorageDir' from the storagepath by searching > + * for the last '/'. > + */ > +static char * > +virTPMGetTPMStorageDir(const char *storagepath) > +{ > + const char *tail = strrchr(storagepath, '/'); > + char *path = NULL; > + > + if (!tail) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not get tail of storagedir %s"), > + storagepath); > + return NULL; > + } > + ignore_value(VIR_STRNDUP(path, storagepath, tail - storagepath)); > + > + return path; > +} > + > +/* > + * virTPMEmulatorInitStorage > + * > + * Initialize the TPM Emulator storage by creating its root directory, > + * which is typically found in /var/lib/libvirt/tpm. > + * > + */ > +static int > +virTPMEmulatorInitStorage(const char *swtpmStorageDir) > +{ > + int rc = 0; > + > + /* allow others to cd into this dir */ > + if (virFileMakePathWithMode(swtpmStorageDir, 0711) < 0) { > + virReportSystemError(errno, > + _("Could not create TPM directory %s"), > + swtpmStorageDir); > + rc = -1; > + } > + > + return rc; > +} > + > +/* > + * virTPMCreateEmulatorStorage > + * > + * @storagepath: directory for swtpm's pesistent state > + * @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 > +virTPMCreateEmulatorStorage(const char *storagepath, > + bool *created, > + uid_t swtpm_user, gid_t swtpm_group) One line each argument > +{ > + int ret = -1; > + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath); > + > + if (!swtpmStorageDir) > + return -1; > + > + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0) > + return -1; Leaks swtpmStorageDir > + > + *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; > +} > + > +void > +virTPMDeleteEmulatorStorage(virDomainTPMDefPtr tpm) > +{ > + char *path = virTPMGetTPMStorageDir(tpm->data.emulator.storagepath); > + > + if (path) { > + ignore_value(virFileDeleteTree(path)); > + VIR_FREE(path); > + } > +} > + > +/* > + * virTPMCreateEmulatorSocket: > + * > + * @swtpmStateDir: the directory where to create the socket in > + * @shortName: short and unique name of the domain > + * > + * Create the vTPM device name from the given parameters > + */ > +static char * > +virTPMCreateEmulatorSocket(const char *swtpmStateDir, const char *shortName) One line each argument > +{ > + char *path = NULL; > + > + ignore_value(virAsprintf(&path, "%s/%s-swtpm.sock", swtpmStateDir, > + shortName)); > + > + return path; > +} > + > +/* > + * virTPMEmulatorInitPaths: > + * > + * @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 > + * @uuid: the UUID of the VM > + */ > +int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm, > + const char *swtpmStorageDir, > + const unsigned char *uuid) > +{ > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + > + virUUIDFormat(uuid, uuidstr); > + > + VIR_FREE(tpm->data.emulator.storagepath); > + if (!(tpm->data.emulator.storagepath = > + virTPMCreateEmulatorStoragePath(swtpmStorageDir, uuidstr))) > + return -1; > + > + return 0; > +} > + > +/* > + * virTPMEmulatorPrepareHost: > + * > + * @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. > + */ > +int virTPMEmulatorPrepareHost(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) one line each argument > +{ > + int ret = -1; > + > + if (virTPMEmulatorInit() < 0) > + return -1; > + > + /* create log dir ... */ > + if (virFileMakePathWithMode(logDir, 0730) < 0) > + goto cleanup; I think we could just return -1; here. > + > + /* ... and adjust ownership */ > + if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group, > + VIR_DIR_CREATE_ALLOW_EXIST) < 0) > + goto cleanup; > + > + /* create logfile name ... */ > + if (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 = > + virTPMCreateEmulatorSocket(swtpmStateDir, shortName))) > + goto cleanup; > + tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX; > + > + ret = 0; > + > + cleanup: > + if (ret) > + VIR_FREE(tpm->data.emulator.logfile); Does this matter? Wouldn't the logfile be free'd in a failure path by virDomainTPMDefFree? If it does matter, use "if (ret < 0)". > + > + return ret; > +} > + > +/* > + * virTPMEmulatorRunSetup > + * > + * @storagepath: path to the directory for TPM state > + * @vmname: the name of the VM > + * @vmuuid: the UUID of the VM > + * @privileged: whether we are running in privileged mode > + * @swtpm_user: The userid to switch to when setting up the TPM; > + * typically this should be the uid of 'tss' or 'root' > + * @swtpm_group: The group id to switch to > + * @logfile: The file to write the log into; it must be writable > + * for the user given by userid or 'tss' > + * > + * Setup the external swtpm by creating endorsement key and > + * certificates for it. > + */ > +static int > +virTPMEmulatorRunSetup(const char *storagepath, const char *vmname, > + const unsigned char *vmuuid, bool privileged, > + uid_t swtpm_user, gid_t swtpm_group, > + const char *logfile) One arg per line > +{ > + virCommandPtr cmd = NULL; > + int exitstatus; > + int rc = 0; Use ret = -1; > + char uuid[VIR_UUID_STRING_BUFLEN]; > + char *vmid = NULL; > + off_t logstart; > + > + if (!privileged) { > + return virFileWriteStr(logfile, > + _("Did not create EK and certificates since " > + "this requires privileged mode\n"), > + 0600); > + } > + > + cmd = virCommandNew(swtpm_setup); > + if (!cmd) { > + rc = -1; > + goto cleanup; > + } Just goto cleanup; w/ @ret initialized to -1 > + > + virUUIDFormat(vmuuid, uuid); > + if (virAsprintf(&vmid, "%s:%s", vmname, uuid) < 0) > + goto cleanup; Because here you would have returned 0 and I don't think that's what you'd expect at this point... > + > + virCommandSetUID(cmd, swtpm_user); > + virCommandSetGID(cmd, swtpm_group); > + > + virCommandAddArgList(cmd, > + "--tpm-state", storagepath, > + "--vmid", vmid, > + "--logfile", logfile, > + "--createek", > + "--create-ek-cert", > + "--create-platform-cert", > + "--lock-nvram", > + "--not-overwrite", > + NULL); > + > + virCommandClearCaps(cmd); > + > + /* get size of logfile */ > + logstart = virFileLength(logfile, -1); > + if (logstart < 0) > + logstart = 0; > + > + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { > + char *buffer = NULL, *errors; > + off_t loglength = virFileLength(logfile, -1); > + > + if (loglength > logstart) { > + ignore_value(virFileReadOffsetQuiet(logfile, logstart, > + loglength - logstart, > + &buffer)); On error @buffer could be NULL > + errors = virStringFilterLines(buffer, "Error:", 160); If @buffer == NULL, then the above is not going to work. Also should it be _("Error:") or are we sure that the program is run with a specific language (e.g. i18n concerns)? Since we don't control the language of that output - it's a bit dicey to assume/use it. > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not run '%s'. exitstatus: %d;\n" > + "%s"), > + swtpm_setup, exitstatus, errors); Given the "concerns" raised, why not just direct the consumer to the @logfile rather than trying to report the error into the output. IOW: if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not run '%s'. exitstatus: %d;\n" "Check error log '%s' for details."), swtpm_setup, exitstatus, logfile); goto cleanup; } If you really want to keep this logic, then handling buffer == NULL before calling virStringFilterLines will need to be done and of course handling that @errors wouldn't be filled in. > + VIR_FREE(buffer); > + VIR_FREE(errors); > + } > + rc = -1; goto cleanup; > + } > + ret = 0; > + cleanup: > + VIR_FREE(vmid); > + virCommandFree(cmd); > + > + return rc; return ret; > +} > + > +/* > + * virTPMEmulatorBuildCommand: > + * > + * @tpm: TPM definition > + * @vmname: The name of the VM > + * @vmuuid: The UUID of the VM > + * @privileged: whether we are running in privileged mode > + * @swtpm_user: The uid for the swtpm to run as (drop privileges to from root) > + * @swtpm_group: The gid for the swtpm to run as > + * > + * Create the virCommand use for starting the emulator > + * Do some initializations on the way, such as creation of storage > + * and emulator setup. > + */ > +virCommandPtr > +virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, > + const unsigned char *vmuuid, bool privileged, > + uid_t swtpm_user, gid_t swtpm_group) One arg per line > +{ > + virCommandPtr cmd = NULL; > + bool created = false; > + > + if (virTPMCreateEmulatorStorage(tpm->data.emulator.storagepath, > + &created, swtpm_user, swtpm_group) < 0) > + return NULL; > + > + if (created && > + virTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid, > + privileged, swtpm_user, swtpm_group, > + tpm->data.emulator.logfile) < 0) > + goto error; > + > + unlink(tpm->data.emulator.source.data.nix.path); > + > + cmd = virCommandNew(swtpm_path); > + if (!cmd) > + goto error; > + > + virCommandClearCaps(cmd); > + > + virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL); > + virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", > + tpm->data.emulator.source.data.nix.path); > + > + virCommandAddArg(cmd, "--tpmstate"); > + virCommandAddArgFormat(cmd, "dir=%s,mode=0600", > + tpm->data.emulator.storagepath); > + > + virCommandAddArg(cmd, "--log"); > + virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.logfile); > + > + virCommandSetUID(cmd, swtpm_user); > + virCommandSetGID(cmd, swtpm_group); > + > + return cmd; > + > + error: > + if (created) > + virTPMDeleteEmulatorStorage(tpm); > + > + VIR_FREE(tpm->data.emulator.source.data.nix.path); > + VIR_FREE(tpm->data.emulator.storagepath); Similar question here about the VIR_FREE's - why not wait for virDomainTPMDefFree? > + > + virCommandFree(cmd); > + > + return NULL; > +} > + > +/* > + * virTPMEmulatorStop > + * @swtpmStateDir: A directory where the socket is located > + * @shortName: short and unique name of the domain > + * > + * Gracefully stop the swptm > + */ > +void > +virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName) > +{ > + virCommandPtr cmd; > + char *pathname; > + char *errbuf = NULL; > + > + if (virTPMEmulatorInit() < 0) > + return; > + > + if (!(pathname = virTPMCreateEmulatorSocket(swtpmStateDir, shortName))) > + return; > + > + if (!virFileExists(pathname)) > + goto cleanup; > + > + cmd = virCommandNew(swtpm_ioctl); > + if (!cmd) { > + VIR_FREE(pathname); ^^^^ Done in cleanup anyway. > + goto cleanup; > + } > + > + virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL); > + > + virCommandSetErrorBuffer(cmd, &errbuf); > + > + ignore_value(virCommandRun(cmd, NULL)); > + > + virCommandFree(cmd); > + > + /* clean up the socket */ > + unlink(pathname); > + > + cleanup: > + VIR_FREE(pathname); > + VIR_FREE(errbuf); > +} > diff --git a/src/util/virtpm.h b/src/util/virtpm.h > index b21fc05..63f75b8 100644 > --- a/src/util/virtpm.h > +++ b/src/util/virtpm.h > @@ -1,7 +1,7 @@ > /* > * virtpm.h: TPM support > * > - * Copyright (C) 2013 IBM Corporation > + * Copyright (C) 2013,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 > @@ -22,6 +22,37 @@ > #ifndef __VIR_TPM_H__ > # define __VIR_TPM_H__ > > +# include "vircommand.h" > + > +typedef struct _virDomainTPMDef virDomainTPMDef; > +typedef virDomainTPMDef *virDomainTPMDefPtr; > + Duplicated from domain_conf.h to avoid errors, crafty... I have a feeling most of this is going to be merged into patch 9... John > char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_NOINLINE; > > +int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm, > + const char *swtpmStorageDir, > + const unsigned char *uuid) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > + ATTRIBUTE_RETURN_CHECK; > +int virTPMEmulatorPrepareHost(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) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > + ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK; > +virCommandPtr virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, > + const char *vmname, > + const unsigned char *vmuuid, > + bool privileged, > + uid_t swtpm_user, > + gid_t swtpm_group) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > + ATTRIBUTE_RETURN_CHECK; > +void virTPMEmulatorStop(const char *swtpmStateDir, > + const char *shortName) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > +void virTPMDeleteEmulatorStorage(virDomainTPMDefPtr tpm) > + ATTRIBUTE_NONNULL(1); > + > #endif /* __VIR_TPM_H__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list