$SUBJ: "Implement" On 05/04/2018 04:21 PM, Stefan Berger wrote: Two blank lines between functions is the new normal... ...and each argument on it's own line... ... here too (as well as the 2 empty lines on either side since we're touching the code). With the adjustments, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; but I do have some other concerns later in patch 7 when this is used as to whether it's necessary. John

On 05/04/2018 04:21 PM, Stefan Berger wrote: ? No src/libvirt_private.syms change to add the new function? Similar to 1/14 - 2 blank lines... I think this is more ExtractLines as compared to virStringFilterChars which filters out certain chars from a buffer and returns a buffer with those filtered out. Seems to be an incomplete thought, but I can adjust. So given Extract vs. Filter: * Search for a given @needle in each line of the @input buffer and * create a return buffer that contains only those lines with the * @needle. Each output buffer line can be further restricted by * providing @maxlinelen in which case the output line would be * truncated @maxlinelen with "..." appended to the line to indicate * the truncation. * * Returns NULL on failure or a buffer with the extracted lines. It * is up to the caller to free the returned buffer. ... and 1 line per argument. See below [1] + if (!input || !needle) { + virReportError(VIR_ERR_INALID_ARG, + _("neither input=%s nor needle=%s can be NULL"), + NULLSTR(input), NULLSTR(needle)); + return NULL; + } + Ran the patch through Coverity and it complained right here... I think you meant "if (eol)", right? [1] Unfortunately all this does is ensure no one calls this with (NULL, NULL,...)... In practice, if @input or @needle is assigned to NULL, the compiler won't catch it. With the adjustments, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; But like patch 1, I do have some other concerns later in patch 7 when this is used. John

On 05/04/2018 04:21 PM, Stefan Berger wrote: [...] syntax-check would have told you to use virBufferAdjustIndent around this and not use " <device..." With the adjustment, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

On 05/04/2018 04:21 PM, Stefan Berger wrote: I take it we don't need ftw.h any more... Two empty lines One arg per line. s/ret;/ret = -1;/ int direrr; path = NULL; s/ret/direrr Replace {...} w/ goto cleanup; Unnecessary. goto cleanup; Unnecessary if (direrr < 0) goto cleanup; ret = 0; cleanup: VIR_FREE(path); return ret; Skip the rest. Two empty lines With the adjustments, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

On 05/04/2018 04:21 PM, Stefan Berger wrote: virSecurityDACRestoreTPMFileLabel doesn't need to be changed? e.g.: ret = virSecurityDACRestoreChardevLabel(mgr, def, &tpm->data.emulator.source, false); Similarly for virSecuritySELinuxRestoreTPMFileLabelInt: tpmdev = tpm->data.emulator.source.data.nix.path; rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev); ? With the adjustments or at least an explanation in the commit message why they cannot be Restored, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

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(a)linux.vnet.ibm.com&gt; --- 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" +#include "viralloc.h" +#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 "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; + /** * virTPMCreateCancelPath: * @devpath: Path to the TPM device @@ -74,3 +94,517 @@ virTPMCreateCancelPath(const char *devpath) cleanup: return path; } + +/* + * 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")); + 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 + * + * 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) +{ + int ret = -1; + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath); + + if (!swtpmStorageDir) + return -1; + + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0) + return -1; + + *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) +{ + 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) +{ + int ret = -1; + + if (virTPMEmulatorInit() < 0) + return -1; + + /* create log dir ... */ + if (virFileMakePathWithMode(logDir, 0730) < 0) + goto cleanup; + + /* ... 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); + + 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) +{ + virCommandPtr cmd = NULL; + int exitstatus; + int rc = 0; + 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; + } + + virUUIDFormat(vmuuid, uuid); + if (virAsprintf(&vmid, "%s:%s", vmname, uuid) < 0) + goto cleanup; + + 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)); + errors = virStringFilterLines(buffer, "Error:", 160); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not run '%s'. exitstatus: %d;\n" + "%s"), + swtpm_setup, exitstatus, errors); + VIR_FREE(buffer); + VIR_FREE(errors); + } + rc = -1; + } + + cleanup: + VIR_FREE(vmid); + virCommandFree(cmd); + + return rc; +} + +/* + * 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) +{ + 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); + + 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); + 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; + 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__ */ -- 2.5.5

On 05/08/2018 04:30 PM, John Ferlan wrote: Probably best to do that... rather than passing the fields of virDomainTPMDef into the functions instead. Currently the functions have the prefix virTPM. That will have to change - to qemuTPM? So I'll merge these functions into qemu_extdevice.c? or another new file qemu_tpm.c ? obviously 'forgot' to run it. You mean the repetition of 'swtpm' is redundant? Should I adapt the reporting to use this type of command ? if (!(qemunbd = virFindFileInPath("qemu-nbd"))) { virReportSystemError(ENOENT, "%s", _("Unable to find 'qemu-nbd' binary in $PATH")); goto cleanup; } + I'll remove that but add a check for tpm->data.emulator.logfile before 'virAsprintf(&tpm->data.emulator.logfile,', so we don't have a memory leak here. I had that before and thought it was more convenient to show to the user what the problem was. I am not insisting ... let me drop this along with patches 1 & 2. Dropped these as well. Thanks for reviewing. Stefan

On 05/10/2018 03:29 PM, John Ferlan wrote: Will post v4 soon. Stefan

On 05/04/2018 04:21 PM, Stefan Berger wrote: I think you'd need to also alter libvirt.spec.in since you're adding new directories... That's one of those make rpm type activities IIRC. Is it intentionally the same as ->logDir? Or did you want to have it's own? Doesn't matter to me - just asking. This one has it's own... although I wonder if it should be swtpm/run to mimic cfg->stateDir As does this one... and I think the path here is fine as it matches other uses. Use the (uid_t) and (gid_t) cast's... John

Implement a layer for starting and stopping of external devices. The tpm-emulator is the only user of this layer. Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- src/qemu/Makefile.inc.am | 2 + src/qemu/qemu_extdevice.c | 300 ++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_extdevice.h | 43 +++++++ src/qemu/qemu_process.c | 13 ++ 4 files changed, 358 insertions(+) create mode 100644 src/qemu/qemu_extdevice.c create mode 100644 src/qemu/qemu_extdevice.h diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am index 63e7c87..d16e880 100644 --- a/src/qemu/Makefile.inc.am +++ b/src/qemu/Makefile.inc.am @@ -19,6 +19,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_domain_address.h \ qemu/qemu_cgroup.c \ qemu/qemu_cgroup.h \ + qemu/qemu_extdevice.c \ + qemu/qemu_extdevice.h \ qemu/qemu_hostdev.c \ qemu/qemu_hostdev.h \ qemu/qemu_hotplug.c \ diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c new file mode 100644 index 0000000..f3f337d --- /dev/null +++ b/src/qemu/qemu_extdevice.c @@ -0,0 +1,300 @@ +/* + * qemu_extdevice.c: QEMU external devices support + * + * Copyright (C) 2014, 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; + */ + +#include <config.h> + +#include "qemu_extdevice.h" +#include "qemu_domain.h" + +#include "viralloc.h" +#include "virlog.h" +#include "virstring.h" +#include "virtime.h" +#include "virtpm.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +VIR_LOG_INIT("qemu.qemu_extdevice") + +static int +qemuExtDeviceLogCommand(qemuDomainLogContextPtr logCtxt, + virCommandPtr cmd, + const char *info) +{ + int ret = -1; + char *timestamp = NULL; + char *logline = NULL; + int logFD; + + logFD = qemuDomainLogContextGetWriteFD(logCtxt); + + if ((timestamp = virTimeStringNow()) == NULL) + goto cleanup; + + if (virAsprintf(&logline, "%s: Starting external device: %s\n", + timestamp, info) < 0) + goto cleanup; + + if (safewrite(logFD, logline, strlen(logline)) < 0) + goto cleanup; + + virCommandWriteArgLog(cmd, logFD); + + ret = 0; + + cleanup: + VIR_FREE(timestamp); + VIR_FREE(logline); + + return ret; +} + + +static int +qemuExtTPMInitPaths(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int ret = 0; + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = virTPMEmulatorInitPaths(def->tpm, cfg->swtpmStorageDir, + def->uuid); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + + virObjectUnref(cfg); + + return ret; +} + + +static int +qemuExtTPMPrepareHost(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int ret = 0; + char *shortName = NULL; + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + shortName = virDomainDefGetShortName(def); + if (!shortName) + goto cleanup; + + ret = virTPMEmulatorPrepareHost(def->tpm, cfg->swtpmLogDir, + def->name, cfg->swtpm_user, + cfg->swtpm_group, + cfg->swtpmStateDir, cfg->user, + shortName); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + +cleanup: + VIR_FREE(shortName); + virObjectUnref(cfg); + + return ret; +} + + +/* + * qemuExtTPMStartEmulator: + * + * @driver: QEMU driver + * @def: domain definition + * @logCtxt: log context + * + * Start the external TPM Emulator: + * - have the command line built + * - start the external TPM Emulator and sync with it before QEMU start + */ +static int +qemuExtTPMStartEmulator(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) +{ + int ret = -1; + virCommandPtr cmd = NULL; + int exitstatus; + char *errbuf = NULL; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainTPMDefPtr tpm = def->tpm; + char *shortName = virDomainDefGetShortName(def); + + if (!shortName) + return -1; + + /* stop any left-over TPM emulator for this VM */ + virTPMEmulatorStop(cfg->swtpmStateDir, shortName); + + if (!(cmd = virTPMEmulatorBuildCommand(tpm, def->name, def->uuid, + driver->privileged, + cfg->swtpm_user, + cfg->swtpm_group))) + goto cleanup; + + if (qemuExtDeviceLogCommand(logCtxt, cmd, "TPM Emulator") < 0) + goto cleanup; + + virCommandSetErrorBuffer(cmd, &errbuf); + + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { + VIR_ERROR("Could not start 'swtpm'. exitstatus: %d\n" + "stderr: %s\n", exitstatus, errbuf); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not start 'swtpm'. exitstatus: %d, " + "error: %s"), exitstatus, errbuf); + goto error; + } + + ret = 0; + + cleanup: + VIR_FREE(shortName); + VIR_FREE(errbuf); + virCommandFree(cmd); + + virObjectUnref(cfg); + + return ret; + + error: + VIR_FREE(tpm->data.emulator.source.data.nix.path); + + goto cleanup; +} + +static int +qemuExtTPMStart(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) +{ + int ret = 0; + virDomainTPMDefPtr tpm = def->tpm; + + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = qemuExtTPMStartEmulator(driver, def, logCtxt); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + + return ret; +} + +static void +qemuExtTPMStop(virQEMUDriverPtr driver, virDomainDefPtr def) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *shortName = NULL; + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + shortName = virDomainDefGetShortName(def); + if (!shortName) + goto cleanup; + + virTPMEmulatorStop(cfg->swtpmStateDir, shortName); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + +cleanup: + VIR_FREE(shortName); + virObjectUnref(cfg); +} + +/* + * qemuExtDevicesInitPaths: + * + * @driver: QEMU driver + * @def: domain definition + * + * Initialize paths of external devices so that it is known where state is + * stored and we can remove directories and files in case of domain XML + * changes. + */ +int +qemuExtDevicesInitPaths(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + int ret = 0; + + if (def->tpm) + ret = qemuExtTPMInitPaths(driver, def); + + return ret; +} + +/* + * qemuExtDevicesPrepareHost: + * + * @driver: QEMU driver + * @def: domain definition + * + * Prepare host storage paths for external devices. + */ +int +qemuExtDevicesPrepareHost(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + int ret = 0; + + if (def->tpm) + ret = qemuExtTPMPrepareHost(driver, def); + + return ret; +} + +int +qemuExtDevicesStart(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) +{ + int ret = 0; + + if (def->tpm) + ret = qemuExtTPMStart(driver, def, logCtxt); + + return ret; +} + +void +qemuExtDevicesStop(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + if (def->tpm) + qemuExtTPMStop(driver, def); +} diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h new file mode 100644 index 0000000..fd6b630 --- /dev/null +++ b/src/qemu/qemu_extdevice.h @@ -0,0 +1,43 @@ +/* + * qemu_extdevice.h: QEMU external devices support + * + * Copyright (C) 2014, 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; + */ +#ifndef __QEMU_EXTDEVICE_H__ +# define __QEMU_EXTDEVICE_H__ + +# include "qemu_conf.h" +# include "qemu_domain.h" + +int qemuExtDevicesInitPaths(virQEMUDriverPtr driver, + virDomainDefPtr def) + ATTRIBUTE_RETURN_CHECK; + +int qemuExtDevicesPrepareHost(virQEMUDriverPtr driver, + virDomainDefPtr def) + ATTRIBUTE_RETURN_CHECK; + +int qemuExtDevicesStart(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) + ATTRIBUTE_RETURN_CHECK; + +void qemuExtDevicesStop(virQEMUDriverPtr driver, virDomainDefPtr def); + +#endif /* __QEMU_EXTDEVICE_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9233d26..2b07530 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,10 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; logfile = qemuDomainLogContextGetWriteFD(logCtxt); + if (qemuExtDevicesInitPaths(driver, vm->def) < 0 || + qemuExtDevicesStart(driver, vm->def, logCtxt) < 0) + goto cleanup; + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(driver, qemuDomainLogContextGetManager(logCtxt), @@ -6194,6 +6203,8 @@ qemuProcessLaunch(virConnectPtr conn, ret = 0; cleanup: + if (ret) + qemuExtDevicesStop(driver, vm->def); qemuDomainSecretDestroy(vm); virCommandFree(cmd); virObjectUnref(logCtxt); @@ -6614,6 +6625,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainCleanupRun(driver, vm); + qemuExtDevicesStop(driver, vm->def); + /* Stop autodestroy in case guest is restarted */ qemuProcessAutoDestroyRemove(driver, vm); -- 2.5.5

On 05/08/2018 04:50 PM, John Ferlan wrote: Removed. Until you now told me that this is the new rule, I was sometimes looking around in the file what others had used there before. ok, in this file I was inconsistent.@@ -6194,6 +6203,8 @@ qemuProcessLaunch(virConnectPtr conn, In all the scenarios I have tried so far I haven't come across having to add something to this function. If this is triggered by restart of libvirtd, I haven't seen a failure. I could have missed something, though. Stefan

On 05/04/2018 04:21 PM, Stefan Berger wrote: 2 blank lines and void virDomainTPMDelete(virDomainDefPtr def) Don't lose the "const" Don't lose the "const" 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. Don't lose the "const" 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

On 05/04/2018 04:21 PM, Stefan Berger wrote: syntax check will tell you "path=/tmp/lib/domain--1-TPM-VM/monitor.sock," can fit on the previous line. Probably should use DO_TEST_CAPS_LATEST here... John

In this patch we label the swtpm process with SELinux labels. We give it the same label as the QEMU process has. We label its state directory and files as well. The file and process labels now look as follows: Directory: /var/lib/libvirt/swtpm [root@localhost swtpm]# ls -lZ total 4 rwx------. 2 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr 5 16:46 testvm [root@localhost testvm]# ls -lZ total 8 -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr 5 16:46 tpm-00.permall The log in /var/log/swtpm/libvirt/qemu is labeled as follows: -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr 5 16:46 vtpm.log [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0 0.0 28172 3892 ? Ss 16:57 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/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:c254,c932 qemu 25669 99.0 0.0 3096704 48500 ? Sl 16:57 3:28 /bin/qemu-system-x86_64 [..] Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_extdevice.c | 22 ++++++++++- src/security/security_driver.h | 4 ++ src/security/security_manager.c | 17 +++++++++ src/security/security_manager.h | 3 ++ src/security/security_selinux.c | 82 +++++++++++++++++++++++++++++++++++++++++ src/security/security_stack.c | 19 ++++++++++ 7 files changed, 147 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e533b95..79b8afa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1334,6 +1334,7 @@ virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; +virSecurityManagerSetTPMLabels; virSecurityManagerStackAddNested; virSecurityManagerTransactionAbort; virSecurityManagerTransactionCommit; diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index f3f337d..eb7220d 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -166,12 +166,32 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, virCommandSetErrorBuffer(cmd, &errbuf); - if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { + if (virSecurityManagerSetTPMLabels(driver->securityManager, + def) < 0) + goto error; + + if (virSecurityManagerSetChildProcessLabel(driver->securityManager, + def, cmd) < 0) + goto error; + + if (virSecurityManagerPreFork(driver->securityManager) < 0) + goto error; + + /* make sure we run this with the appropriate user */ + virCommandSetUID(cmd, cfg->swtpm_user); + virCommandSetGID(cmd, cfg->swtpm_group); + + ret = virCommandRun(cmd, &exitstatus); + + virSecurityManagerPostFork(driver->securityManager); + + if (ret < 0 || exitstatus != 0) { VIR_ERROR("Could not start 'swtpm'. exitstatus: %d\n" "stderr: %s\n", exitstatus, errbuf); virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not start 'swtpm'. exitstatus: %d, " "error: %s"), exitstatus, errbuf); + ret = -1; goto error; } diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 95e7c4d..4aa415f 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -149,6 +149,8 @@ typedef int (*virSecurityDomainRestoreChardevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainChrSourceDefPtr dev_source, bool chardevStdioLogd); +typedef int (*virSecurityDomainSetTPMLabels) (virSecurityManagerPtr mgr, + virDomainDefPtr def); struct _virSecurityDriver { @@ -213,6 +215,8 @@ struct _virSecurityDriver { virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel; virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel; + + virSecurityDomainSetTPMLabels domainSetSecurityTPMLabels; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 71f7f59..48777bb 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1204,3 +1204,20 @@ virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr, virReportUnsupportedError(); return -1; } + + +int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + int ret; + + if (mgr->drv->domainSetSecurityTPMLabels) { + virObjectLock(mgr); + ret = mgr->drv->domainSetSecurityTPMLabels(mgr, vm); + virObjectUnlock(mgr); + + return ret; + } + + return 0; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index c36a8b4..671f6a8 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -194,4 +194,7 @@ int virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainChrSourceDefPtr dev_source, bool chardevStdioLogd); +int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, + virDomainDefPtr vm); + #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 17bc07a..42a940b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3047,6 +3047,86 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr, return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel); } +/* + * _virSecuritySELinuxSetSecurityFileLabels: + * + * @mgr: the virSecurityManager + * @path: path to a directory or a file + * @seclabel: the security label + * + * Set the file labels on the given path; if the path is a directory + * we label all files found there, including the directory itself, + * otherwise we just label the file. + */ +static int +_virSecuritySELinuxSetSecurityFileLabels(virSecurityManagerPtr mgr, + const char *path, + virSecurityLabelDefPtr seclabel) +{ + int ret = 0; + struct dirent *ent; + char *filename = NULL; + DIR *dir; + + if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel))) + return ret; + + if (virDirOpen(&dir, path) < 0) + return 0; + + while ((ret = virDirRead(dir, &ent, path)) > 0) { + if (ent->d_type != DT_REG) + continue; + + if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) { + ret = -1; + break; + } + ret = virSecuritySELinuxSetFilecon(mgr, filename, + seclabel->imagelabel); + VIR_FREE(filename); + if (ret) + break; + } + if (ret) + virReportSystemError(errno, _("Unable to label files under %s"), + path); + + virDirClose(&dir); + + return ret; +} + +static int +virSecuritySELinuxSetSecurityTPMLabels(virSecurityManagerPtr mgr, + virDomainDefPtr def) +{ + int ret = 0; + virSecurityLabelDefPtr seclabel; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (seclabel == NULL) + return 0; + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = _virSecuritySELinuxSetSecurityFileLabels( + mgr, def->tpm->data.emulator.storagepath, + seclabel); + if (ret == 0 && def->tpm->data.emulator.logfile) + ret = _virSecuritySELinuxSetSecurityFileLabels( + mgr, def->tpm->data.emulator.logfile, + seclabel); + break; + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + + return ret; +} + virSecurityDriver virSecurityDriverSELinux = { .privateDataLen = sizeof(virSecuritySELinuxData), .name = SECURITY_SELINUX_NAME, @@ -3106,4 +3186,6 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityChardevLabel = virSecuritySELinuxSetChardevLabel, .domainRestoreSecurityChardevLabel = virSecuritySELinuxRestoreChardevLabel, + + .domainSetSecurityTPMLabels = virSecuritySELinuxSetSecurityTPMLabels, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 9615f9f..7f10ef0 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -760,6 +760,23 @@ virSecurityStackDomainRestoreChardevLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackSetSecurityTPMLabels(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetTPMLabels(item->securityManager, + vm) < 0) + rc = -1; + } + + return rc; +} + virSecurityDriver virSecurityDriverStack = { .privateDataLen = sizeof(virSecurityStackData), .name = "stack", @@ -822,4 +839,6 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityChardevLabel = virSecurityStackDomainSetChardevLabel, .domainRestoreSecurityChardevLabel = virSecurityStackDomainRestoreChardevLabel, + + .domainSetSecurityTPMLabels = virSecurityStackSetSecurityTPMLabels, }; -- 2.5.5

On 05/04/2018 04:21 PM, Stefan Berger wrote: Perhaps this would be better as just version='2' since you're in a <tpm ...> block? I trust we check that somewhere ... Maybe this should just be "TPM_VERSION_DEFAULT" 2 blank lines syntax-check complained about the unnecessary { } Kind of odd to do the AddList and then make the check, too. path=/tmp/lib/domain--1-TPM-VM/monitor.sock, can be on the previous line according to syntax-check Another one where I wonder if DO_TEST_CAPS_LATEST should be used. Beyond the XML syntax, things look reasonable. I'll wait for the next spin to see what happens. John

Add the external swtpm to the emulator cgroup so that upper limits of CPU usage can be enforced on the emulated TPM. To enable this we need to have the swtpm write its process id (pid) into a file. We then read it from the file to configure the emulator cgroup. The PID file is created in /var/run/libvirt/qemu/swtpm: [root@localhost swtpm]# ls -lZ /var/run/libvirt/qemu/swtpm/ total 4 -rw-r--r--. 1 tss tss system_u:object_r:qemu_var_run_t:s0 5 Apr 10 12:26 1-testvm-swtpm.pid srw-rw----. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 10 12:26 1-testvm-swtpm.sock The swtpm command line now looks as follows: root@localhost testvm]# ps auxZ | grep swtpm | grep socket | grep -v grep system_u:system_r:virtd_t:s0:c597,c632 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 --pid file=/var/run/libvirt/qemu/swtpm/1-testvm-swtpm.pid Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_cgroup.h | 1 + src/qemu/qemu_extdevice.c | 19 +++++++++++++++++ src/qemu/qemu_process.c | 4 ++++ src/util/virtpm.c | 33 +++++++++++++++++++++++++++++ 7 files changed, 112 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c98d26a..f542c8e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2624,6 +2624,7 @@ void virDomainTPMDefFree(virDomainTPMDefPtr def) VIR_FREE(def->data.emulator.source.data.nix.path); VIR_FREE(def->data.emulator.storagepath); VIR_FREE(def->data.emulator.logfile); + VIR_FREE(def->data.emulator.pidfile); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 826ff26..49c77f7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1311,6 +1311,7 @@ struct _virDomainTPMDef { virDomainChrSourceDef source; char *storagepath; char *logfile; + char *pidfile; } emulator; } data; }; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1a5adca..f86ac3f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -38,6 +38,7 @@ #include "virnuma.h" #include "virsystemd.h" #include "virdevmapper.h" +#include "virpidfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1146,6 +1147,58 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, int +qemuSetupCgroupForExtDevices(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainTPMDefPtr tpm = vm->def->tpm; + virCgroupPtr cgroup_temp = NULL; + pid_t pid; + int ret = -1; + + if (priv->cgroup == NULL) + return 0; /* Not supported, so claim success */ + + /* + * If CPU cgroup controller is not initialized here, then we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, then there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, + false, &cgroup_temp) < 0) + goto cleanup; + + if (tpm) { + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (virPidFileReadPath(tpm->data.emulator.pidfile, &pid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not read swtpm's pidfile %s"), + tpm->data.emulator.pidfile); + goto cleanup; + } + if (virCgroupAddTask(cgroup_temp, pid) < 0) + goto cleanup; + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + } + + ret = 0; + +cleanup: + virCgroupFree(&cgroup_temp); + + return ret; +} + + +int qemuSetupGlobalCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 3b8ff60..478bf7e 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -69,6 +69,7 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, long long quota); int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupGlobalCpuCgroup(virDomainObjPtr vm); +int qemuSetupCgroupForExtDevices(virDomainObjPtr vm); int qemuRemoveCgroup(virDomainObjPtr vm); typedef struct _qemuCgroupEmulatorAllNodesData qemuCgroupEmulatorAllNodesData; diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index eb7220d..779e759 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -148,6 +148,9 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainTPMDefPtr tpm = def->tpm; char *shortName = virDomainDefGetShortName(def); + char *pidfiledata = NULL; + int timeout; + int len; if (!shortName) return -1; @@ -195,6 +198,22 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, goto error; } + /* check that the swtpm has written its pid into the file */ + timeout = 1000; /* ms */ + while ((len = virFileReadHeaderQuiet(tpm->data.emulator.pidfile, + 10, &pidfiledata)) <= 0) { + if (len == 0 && timeout > 0) { + timeout -= 50; + usleep(50 * 1000); + continue; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("swtpm did not write pidfile '%s'"), + tpm->data.emulator.pidfile); + goto error; + } + VIR_FREE(pidfiledata); + ret = 0; cleanup: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2b07530..bf0e4da 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6076,6 +6076,10 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupEmulator(vm) < 0) goto cleanup; + VIR_DEBUG("Setting cgroup for external devices (if required)"); + if (qemuSetupCgroupForExtDevices(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting up resctrl"); if (qemuProcessResctrlCreate(driver, vm) < 0) goto cleanup; diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 0617326..3d8a195 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -39,6 +39,7 @@ #include "virlog.h" #include "virtpm.h" #include "virutil.h" +#include "virpidfile.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -376,6 +377,25 @@ int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm, } /* + * virTPMCreatePidFilename + */ +static char *virTPMCreatePidFilename(const char *swtpmStateDir, + const char *shortName) +{ + char *pidfile = NULL; + char *devname = NULL; + + if (virAsprintf(&devname, "%s-swtpm", shortName) < 0) + return NULL; + + pidfile = virPidFileBuildPath(swtpmStateDir, devname); + + VIR_FREE(devname); + + return pidfile; +} + +/* * virTPMEmulatorPrepareHost: * * @tpm: tpm definition @@ -442,6 +462,10 @@ int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, goto cleanup; tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX; + if (!(tpm->data.emulator.pidfile = + virTPMCreatePidFilename(swtpmStateDir, shortName))) + goto cleanup; + ret = 0; cleanup: @@ -619,6 +643,9 @@ virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, break; } + virCommandAddArg(cmd, "--pid"); + virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.pidfile); + return cmd; error: @@ -646,6 +673,7 @@ virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName) virCommandPtr cmd; char *pathname; char *errbuf = NULL; + char *pidfile; if (virTPMEmulatorInit() < 0) return; @@ -674,6 +702,11 @@ virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName) unlink(pathname); cleanup: + /* clean up the PID file */ + if ((pidfile = virTPMCreatePidFilename(swtpmStateDir, shortName))) { + unlink(pidfile); + VIR_FREE(pidfile); + } VIR_FREE(pathname); VIR_FREE(errbuf); } -- 2.5.5

On 05/04/2018 04:21 PM, Stefan Berger wrote: So that I don't forget ;-) - there should be a "next" patch as well that adds some text to docs/news.xml in order to describe "at a high level" the new feature or improvement being done. Perhaps something that should be done for the existing series I pushed, but neglected to remember to ask about while reviewing. There are varying "arguments" about using/saving a pidfile or pid of a thread in recent posts vis-a-vis the pid stored in some file doesn't match what's running for various reasons. How does one know that that pid in the pidfile matches the pid of the running process without checking. You may want to look at src/util/virpidfile.c and various virPidFile* API's to see if they're useful. Somehow I have a feeling this will be similar to other consumers. Also, see the discussion during pr_helper review v3, patch 9: https://www.redhat.com/archives/libvir-list/2018-March/msg00749.html The pr_helper in the end is a process/thread being started to manage something - much like swtpm is a process/thread managing something (blackbox view). Since this only matters "if (tpm && tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)", then why not check that here and simplify the rest a bit more. Currently, the VIR_CGROUP_THREAD_EMULATOR is used for the qemu-kvm emulator (e.g. qemu-kvm process/thread). There's also VCPU and IOTHREAD cgroup enum's. So that makes me wonder if there should be a new "group" for "any" similar thread that's created based on domain configuration. This type of conversation is a bit out of my comfort zone though. Usually I defer to danpb on this although IIRC mkletzan also has some expertise here. Since this "concept" of having to limit the thread via cgroups has an impact on another series I've recently reviewed (the pr_helper), you should see that I responded today to that series with a question of mprivozn about whether it would be needed there as well, see: https://www.redhat.com/archives/libvir-list/2018-May/msg00579.html I think the virPidFile definitely helps here although I haven't dug too deep into those API's. Not quite sure why we need to save this especially since during stop processing we don't use it and we can recreate it fairly easily. This code could just have a local @pidfile = virTPMCreatePidFilename, then once added to the arglist, VIR_FREE(pidfile). Especially since we can build it up on the fly using information we have. So why not use tpm->data.emulator.pidfile or pass it and only clean up "if (pidfile) {...}"... IOW: Pass as an argument @pidfile; otherwise, what's the purpose of keeping it around? Curious why is it libvirt's responsibility for this cleanup? If adding "--pid $path" to the swtpm startup has the result of creating a pid file that a consumer can read once started/running, then shouldn't it be the responsibility of that same image/code to remove the pidfile as part of it's clean up processing indicating that the process no longer exists? OK so yeah, swtpm could die, not clean things up, and then what? Well for sure the domain isn't going to last, right? So perhaps on cleanup I could see reason for ensuring the pidfile doesn't exist, but I also assume that swtpm would write a "clean" pid if it finds --pid on the command line. So does it really matter beyond needing to somehow know that swtpm succeeded in starting "this" time. John