On 05/08/2018 04:01 PM, John Ferlan wrote:

On 05/04/2018 04:21 PM, Stefan Berger wrote:

...

Extend the DAC and SELinux modules with support for the tpm-emulator.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/security/security_dac.c | 4 ++++ src/security/security_selinux.c | 5 +++++ 2 files changed, 9 insertions(+)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 5efbc27..351f6f4 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1373,6 +1373,10 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = virSecurityDACSetChardevLabel(mgr, def, + &tpm->data.emulator.source, + false); + break; case VIR_DOMAIN_TPM_TYPE_LAST: break; } virSecurityDACRestoreTPMFileLabel doesn't need to be changed? e.g.:

Hm, this is setting the DAC label for the Unix socket so that QEMU can connect to it. swtpm creates it but then also removes it upon termination. So basically restoring the security label is not need. Maybe I should add a comment in the code why it's not there. The same is true for the SELinux label below.

ret = virSecurityDACRestoreChardevLabel(mgr, def, &tpm->data.emulator.source, false);

...

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f5ba877..17bc07a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1473,6 +1473,11 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, } break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: + tpmdev = tpm->data.emulator.source.data.nix.path; + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel); + if (rc < 0) + return -1; + break; case VIR_DOMAIN_TPM_TYPE_LAST: break; }

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@redhat.com>

John

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@linux.vnet.ibm.com> --- 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__ */

On 05/09/2018 01:47 PM, Stefan Berger wrote:
> On 05/08/2018 04:30 PM, John Ferlan wrote:
>>
>> 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@linux.vnet.ibm.com>
>>> ---
>>>   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.
> 
> 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 ?
> 
> 

qemu_tpm.c seems good for those specific things

> 
>>
>>> +#include "viralloc.h"
>> syntax-check would have told you not to include "viralloc.h" twice...
> 
> obviously 'forgot' to run it.
> 
>>
>>> +#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.
> 
> You mean the repetition of 'swtpm' is redundant?

yes.

> 
> 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;
>     }
> +

That seems reasonable.

>>> +/*
>>> + * 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)".
> 
> 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.
> 

ah - oh right - that's probably better anyway.

>>
>>> +
>>> +    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.
> 
> I had that before and thought it was more convenient to show to the user
> what the problem was.
> 
>>
>>
>> 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.
> 
> I am not insisting ... let me drop this along with patches 1 & 2.
> 

At some point the handling of errors in an error path just becomes one
of those seems like a good idea. I didn't mind the extra lines of output
and effort because I'm one who likes verbose error messages.

John

>>
>>> +            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?
> 
> Dropped these as well.
> 
> 
>>
>>> +
>>> +    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...
> 
> Thanks for reviewing.
> 
>    Stefan
> 

On 05/08/2018 04:30 PM, John Ferlan wrote:

On 05/04/2018 04:21 PM, Stefan Berger wrote:

...

Extend qemu_conf with user and group for running the tpm-emulator and add directories to the configuration for the locations of the log, state, and socket of the tpm-emulator.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/libvirtd_qemu.aug | 5 +++++ src/qemu/qemu.conf | 8 +++++++ src/qemu/qemu_conf.c | 43 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 6 ++++++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 5 files changed, 64 insertions(+)

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.

Adding that to this patch.

...

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a..23bfe67 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,9 @@ module Libvirtd_qemu = let vxhs_entry = bool_entry "vxhs_tls" | str_entry "vxhs_tls_x509_cert_dir"

+ let swtpm_user_entry = str_entry "swtpm_user" + let swtpm_group_entry = str_entry "swtpm_group" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -137,6 +140,8 @@ module Libvirtd_qemu = | gluster_debug_level_entry | memory_entry | vxhs_entry + | swtpm_user_entry + | swtpm_group_entry

let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 3444185..26a6dc7 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -779,3 +779,11 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# User for the swtpm TPM Emulator +# +# Default is 'tss'; this is the same user that tcsd (TrouSerS) installs +# and uses; alternative is 'root' +# +#swtpm_user = "tss" +#swtpm_group = "tss" diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bfbb572..99c37c6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -159,6 +159,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) "%s/log/libvirt/qemu", LOCALSTATEDIR) < 0) goto error;

+ if (virAsprintf(&cfg->swtpmLogDir, + "%s/log/swtpm/libvirt/qemu", LOCALSTATEDIR) < 0) + goto error; + if (VIR_STRDUP(cfg->configBaseDir, SYSCONFDIR "/libvirt") < 0) goto error;

@@ -166,6 +170,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) "%s/run/libvirt/qemu", LOCALSTATEDIR) < 0) goto error;

+ if (virAsprintf(&cfg->swtpmStateDir, + "%s/run/libvirt/qemu/swtpm", LOCALSTATEDIR) < 0) + goto error; + if (virAsprintf(&cfg->cacheDir, "%s/cache/libvirt/qemu", LOCALSTATEDIR) < 0) goto error; @@ -186,6 +194,13 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; if (virAsprintf(&cfg->memoryBackingDir, "%s/ram", cfg->libDir) < 0) goto error; + if (virAsprintf(&cfg->swtpmStorageDir, "%s/lib/libvirt/swtpm", + LOCALSTATEDIR) < 0) + goto error; + if (virGetUserID("tss", &cfg->swtpm_user) < 0) + cfg->swtpm_user = 0; /* fall back to root */ + if (virGetGroupID("tss", &cfg->swtpm_group) < 0) + cfg->swtpm_group = 0; /* fall back to root */ } else { char *rundir; char *cachedir; @@ -199,6 +214,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) VIR_FREE(cachedir); goto error; } + if (virAsprintf(&cfg->swtpmLogDir, + "%s/qemu/log", cachedir) < 0) { Is it intentionally the same as ->logDir? Or did you want to have it's own? Doesn't matter to me - just asking.

Yes. Permissions are not an issue in this case while in the privileged case I had to put the swtpm logs elsewhere due to file permissions.

...

+ VIR_FREE(cachedir); + goto error; + } if (virAsprintf(&cfg->cacheDir, "%s/qemu/cache", cachedir) < 0) { VIR_FREE(cachedir); goto error; @@ -214,6 +234,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) } VIR_FREE(rundir);

+ if (virAsprintf(&cfg->swtpmStateDir, "%s/swtpm", cfg->stateDir) < 0) + goto error; + This one has it's own... although I wonder if it should be swtpm/run to mimic cfg->stateDir

If 'run' implies that the directory can be deleted, like seems to be the case of /var/run/ between reboots, then we cannot put it there since the state of the TPM needs to be preserved.

...

if (!(cfg->configBaseDir = virGetUserConfigDirectory())) goto error;

@@ -233,6 +256,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; if (virAsprintf(&cfg->memoryBackingDir, "%s/qemu/ram", cfg->configBaseDir) < 0) goto error; + if (virAsprintf(&cfg->swtpmStorageDir, "%s/qemu/swtpm", cfg->configBaseDir) < 0) + goto error;

As does this one... and I think the path here is fine as it matches other uses.

...

+ cfg->swtpm_user = -1; + cfg->swtpm_group = -1; Use the (uid_t) and (gid_t) cast's...

Done.

John

...

}

if (virAsprintf(&cfg->configDir, "%s/qemu", cfg->configBaseDir) < 0) @@ -351,7 +378,9 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->configDir); VIR_FREE(cfg->autostartDir); VIR_FREE(cfg->logDir); + VIR_FREE(cfg->swtpmLogDir); VIR_FREE(cfg->stateDir); + VIR_FREE(cfg->swtpmStateDir);

VIR_FREE(cfg->libDir); VIR_FREE(cfg->cacheDir); @@ -400,6 +429,7 @@ static void virQEMUDriverConfigDispose(void *obj) virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);

VIR_FREE(cfg->memoryBackingDir); + VIR_FREE(cfg->swtpmStorageDir); }

@@ -471,6 +501,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, size_t i, j; char *stdioHandler = NULL; char *user = NULL, *group = NULL; + char *swtpm_user = NULL, *swtpm_group = NULL; char **controllers = NULL; char **hugetlbfs = NULL; char **nvram = NULL; @@ -907,6 +938,16 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) goto cleanup;

+ if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) + goto cleanup; + if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "swtpm_group", &swtpm_group) < 0) + goto cleanup; + if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0) + goto cleanup; + ret = 0;

cleanup: @@ -917,6 +958,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, VIR_FREE(corestr); VIR_FREE(user); VIR_FREE(group); + VIR_FREE(swtpm_user); + VIR_FREE(swtpm_group); virConfFree(conf); return ret; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e1ad546..19dc0bc 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -102,7 +102,9 @@ struct _virQEMUDriverConfig { char *configDir; char *autostartDir; char *logDir; + char *swtpmLogDir; char *stateDir; + char *swtpmStateDir; /* These two directories are ones QEMU processes use (so must match * the QEMU user/group */ char *libDir; @@ -111,6 +113,7 @@ struct _virQEMUDriverConfig { char *snapshotDir; char *channelTargetDir; char *nvramDir; + char *swtpmStorageDir;

char *defaultTLSx509certdir; bool checkdefaultTLSx509certdir; @@ -206,6 +209,9 @@ struct _virQEMUDriverConfig {

bool vxhsTLS; char *vxhsTLSx509certdir; + + uid_t swtpm_user; + gid_t swtpm_group; };

/* Main driver state */ diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9..6d6e1d4 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -100,3 +100,5 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } +{ "swtpm_user" = "tss" } +{ "swtpm_group" = "tss" }

On 05/04/2018 04:21 PM, Stefan Berger wrote:

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@linux.vnet.ibm.com> --- 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@linux.vnet.ibm.com> + */ + +#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;

Leaking @cfg (repeats often)

+ + /* 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);

Still not clear why VIR_FREE here since virDomainTPMDefFree does it.

+ + goto cleanup; +} +

Right about here you went back to single blank line between functions.

+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@linux.vnet.ibm.com> + */ +#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);

Anything need to be done during qemuProcessReconnect? John

On 05/08/2018 05:07 PM, John Ferlan wrote:

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@linux.vnet.ibm.com> --- 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"

a left-over from some previous call that made changes to 'tpm'. fixed.

...

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.

All other cases I see use the same function without error message. Not sure what you mean. We seem to follow a pattern with this now.

...

+ 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,

Done. Thanks.

Reviewed-by: John Ferlan <jferlan@redhat.com>

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)

On 05/08/2018 05:15 PM, John Ferlan wrote:

On 05/04/2018 04:21 PM, Stefan Berger wrote:

...

This patch adds extensions to existing test cases and specific test cases for the tpm-emulator.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- tests/qemuxml2argvdata/tpm-emulator.args | 27 +++++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 15 +++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 tests/qemuxml2argvdata/tpm-emulator.args

diff --git a/tests/qemuxml2argvdata/tpm-emulator.args b/tests/qemuxml2argvdata/tpm-emulator.args new file mode 100644 index 0000000..5970928 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-emulator.args @@ -0,0 +1,27 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name TPM-VM \ +-S \ +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \ +-m 2048 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,\ +path=/tmp/lib/domain--1-TPM-VM/monitor.sock,server,nowait \ syntax check will tell you "path=/tmp/lib/domain--1-TPM-VM/monitor.sock," can fit on the previous line.

...

+-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot order=c,menu=on \ +-usb \ +-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \ +-chardev socket,id=chrtpm,path=/dev/test \ +-device tpm-tis,tpmdev=tpm-tpm0,id=tpm0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 8ef7701..a80e3f2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -532,6 +532,19 @@ testCompareXMLToArgv(const void *data) } }

+ if (vm->def->tpm) { + switch (vm->def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (VIR_STRDUP(vm->def->tpm->data.emulator.source.data.file.path, + "/dev/test") < 0) + goto cleanup; + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + } + if (!(cmd = qemuProcessCreatePretendCmd(&driver, vm, migrateURI, (flags & FLAG_FIPS), false, VIR_QEMU_PROCESS_START_COLD))) { @@ -1989,6 +2002,8 @@ mymain(void) QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_CRB); DO_TEST_PARSE_ERROR("tpm-no-backend-invalid", QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS); + DO_TEST("tpm-emulator", + QEMU_CAPS_DEVICE_TPM_EMULATOR, QEMU_CAPS_DEVICE_TPM_TIS); Probably should use DO_TEST_CAPS_LATEST here...

Using that now. Stefan

John

...

DO_TEST_PARSE_ERROR("pci-domain-invalid", NONE);

On 05/04/2018 04:21 PM, Stefan Berger wrote:

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@linux.vnet.ibm.com> --- 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(-)

I think this looks OK - not my specialty 0-) though. I see security_manager, selinux, etc. and my eyes start glazing over! Anyway, I assume the reason there's no Restore processing is because everything is deleted at shutdown, right?

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 virSecurity...

+{ + 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); }

2 blank lines

+/* + * _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)

if (ret < 0) Things look reasonable to my untrained eyes - on the next series - hopefully someone else can poke into this particular patch as well. John

+ 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, };

On 05/08/2018 05:38 PM, John Ferlan wrote:

On 05/04/2018 04:21 PM, Stefan Berger wrote:

...

This patch extends the TPM's device XML with TPM 2 support. This only works for the emulator type backend and looks as follows:

<tpm model='tpm-tis'> <backend type='emulator' tpmversion='2'/> Perhaps this would be better as just version='2' since you're in a <tpm ...> block?

Ok. Changed it.

...

</tpm>

The swtpm process now has --tpm2 as an additional parameter:

system_u:system_r:svirt_t:s0:c597,c632 tss 18477 11.8 0.0 28364 3868 ? Rs 11:13 13:50 /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/tpm2,mode=0640 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log --tpm2 --pid file=/var/run/libvirt/qemu/swtpm/testvm-swtpm.pid

The version of the TPM can be changed and the state of the TPM is preserved.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 17 +++++- docs/schemas/domaincommon.rng | 12 ++++ src/conf/domain_conf.c | 21 ++++++- src/conf/domain_conf.h | 6 ++ src/util/virtpm.c | 79 ++++++++++++++++++++++++-- tests/qemuxml2argvdata/tpm-emulator-tpm2.args | 27 +++++++++ tests/qemuxml2argvdata/tpm-emulator-tpm2.xml | 30 ++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmloutdata/tpm-emulator-tpm2.xml | 34 +++++++++++ 9 files changed, 221 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2.args create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator-tpm2.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 2a8912f..08df78a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7663,7 +7663,7 @@ qemu-kvm -net nic,model=? /dev/null ... <devices> <tpm model='tpm-tis'> - <backend type='emulator'> + <backend type='emulator' tpmversion='2'> </backend> </tpm> </devices> @@ -7713,6 +7713,21 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> </dd> + <dt><code>tpmversion</code></dt> + <dd> + <p> + The <code>tpmversion</code> attribute indicates the version + of the TPM. By default a TPM 1.2 is created. This attribute + only works with the <code>emulator</code> backend. The following + versions are supported: + </p> + <ul> + <li>'1.2' : creates a TPM 1.2</li> + <li>'2' : creates a TPM 2</li> + </ul> + Note that once a certain version of a TPM has been created for + a guest, the version must not be changed anymore. + </dd>

I trust we check that somewhere ...

The restriction is not there anymore. I removed this sentence.

...

</dl>

<h4><a id="elementsNVRAM">NVRAM device</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c65a9a3..a452a13 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4143,6 +4143,18 @@ </attribute> </group> </choice> + <choice> + <group> + <optional> + <attribute name="tpmversion"> + <choice> + <value>1.2</value> + <value>2</value> + </choice> + </attribute> + </optional> + </group> + </choice> </element> </define>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a42574a..c98d26a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12609,7 +12609,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, * or like this: * * <tpm model='tpm-tis'> - * <backend type='emulator'/> + * <backend type='emulator' tpmversion='2'/> * </tpm> */ static virDomainTPMDefPtr @@ -12622,6 +12622,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, char *path = NULL; char *model = NULL; char *backend = NULL; + char *tpmversion = NULL; virDomainTPMDefPtr def; xmlNodePtr save = ctxt->node; xmlNodePtr *backends = NULL; @@ -12668,6 +12669,20 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; }

+ tpmversion = virXMLPropString(backends[0], "tpmversion"); + if (!tpmversion || STREQ(tpmversion, "1.2")) { + def->tpmversion = VIR_DOMAIN_TPM_VERSION_1_2; + /* only TIS available for emulator */ + if (def->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) + def->model = VIR_DOMAIN_TPM_MODEL_TIS; + } else if (STREQ(tpmversion, "2")) { + def->tpmversion = VIR_DOMAIN_TPM_VERSION_2; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported TPM version '%s'"), + tpmversion); + } + switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: path = virXPathString("string(./backend/device/@path)", ctxt); @@ -12692,6 +12707,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(model); VIR_FREE(backend); VIR_FREE(backends); + VIR_FREE(tpmversion); ctxt->node = save; return def;

@@ -24849,6 +24865,9 @@ virDomainTPMDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<backend type='%s'", virDomainTPMBackendTypeToString(def->type));

+ if (def->tpmversion == VIR_DOMAIN_TPM_VERSION_2) + virBufferAddLit(buf, " tpmversion='2'"); + switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: virBufferAddLit(buf, ">\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c304b08..826ff26 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1291,12 +1291,18 @@ typedef enum { VIR_DOMAIN_TPM_TYPE_LAST } virDomainTPMBackendType;

+typedef enum { + VIR_DOMAIN_TPM_VERSION_1_2,

Maybe this should just be "TPM_VERSION_DEFAULT"

I'd rather keep the explicit VERSION_1_2.

...

+ VIR_DOMAIN_TPM_VERSION_2, +} virDomainTPMVersion; + # define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"

struct _virDomainTPMDef { virDomainTPMBackendType type; virDomainDeviceInfo info; virDomainTPMModel model; + virDomainTPMVersion tpmversion; union { struct { virDomainChrSourceDef source; diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 76bbb21..0617326 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -52,6 +52,8 @@ static char *swtpm_path; static char *swtpm_setup; static char *swtpm_ioctl;

+static bool swtpm_supports_tpm2; + /** * virTPMCreateCancelPath: * @devpath: Path to the TPM device @@ -96,6 +98,38 @@ virTPMCreateCancelPath(const char *devpath) }

2 blank lines

...

/* + * virTPMCheckForTPM2Support + * + * Check whether swtpm_setup supports TPM 2 + */ +static void +virTPMCheckForTPM2Support(void) +{ + virCommandPtr cmd; + char *help = NULL; + + if (!swtpm_setup) + return; + + cmd = virCommandNew(swtpm_setup); + if (!cmd) + return; + + virCommandAddArg(cmd, "--help"); + virCommandSetOutputBuffer(cmd, &help); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (strstr(help, "--tpm2")) + swtpm_supports_tpm2 = true; + + cleanup: + virCommandFree(cmd); + VIR_FREE(help); +} + +/* * virTPMEmulatorInit * * Initialize the Emulator functions by searching for necessary @@ -134,6 +168,7 @@ virTPMEmulatorInit(void) VIR_FREE(swtpm_setup); return -1; } + virTPMCheckForTPM2Support(); }

if (!swtpm_ioctl) { @@ -160,16 +195,28 @@ virTPMEmulatorInit(void) * * @swtpmStorageDir: directory for swtpm persistent state * @vmname: The name of the VM for which to create the storage + * @tpmversion: version of the TPM * * Create the swtpm's storage path */ static char * virTPMCreateEmulatorStoragePath(const char *swtpmStorageDir, - const char *vmname) + const char *vmname, + virDomainTPMVersion tpmversion) { char *path = NULL; + const char *dir = ""; + + switch (tpmversion) { + case VIR_DOMAIN_TPM_VERSION_1_2: + dir = "tpm1.2"; + break; + case VIR_DOMAIN_TPM_VERSION_2: + dir = "tpm2"; + break; + }

- ignore_value(virAsprintf(&path, "%s/%s/tpm1.2", swtpmStorageDir, vmname)); + ignore_value(virAsprintf(&path, "%s/%s/%s", swtpmStorageDir, vmname, dir));

return path; } @@ -321,7 +368,8 @@ int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm,

VIR_FREE(tpm->data.emulator.storagepath); if (!(tpm->data.emulator.storagepath = - virTPMCreateEmulatorStoragePath(swtpmStorageDir, uuidstr))) + virTPMCreateEmulatorStoragePath(swtpmStorageDir, uuidstr, + tpm->tpmversion))) return -1;

return 0; @@ -415,6 +463,7 @@ int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, * @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' + * @tpmversion: The version of the TPM, either a TPM 1.2 or TPM 2 * * Setup the external swtpm by creating endorsement key and * certificates for it. @@ -423,7 +472,8 @@ 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) + const char *logfile, + const virDomainTPMVersion tpmversion) { virCommandPtr cmd = NULL; int exitstatus; @@ -452,6 +502,17 @@ virTPMEmulatorRunSetup(const char *storagepath, const char *vmname, virCommandSetUID(cmd, swtpm_user); virCommandSetGID(cmd, swtpm_group);

+ switch (tpmversion) { + case VIR_DOMAIN_TPM_VERSION_1_2: + break; + case VIR_DOMAIN_TPM_VERSION_2: + virCommandAddArgList(cmd, "--tpm2", NULL); + if (!swtpm_supports_tpm2) { + goto cleanup; + } syntax-check complained about the unnecessary { }

Kind of odd to do the AddList and then make the check, too.

Reversed.

...

+ break; + } + virCommandAddArgList(cmd, "--tpm-state", storagepath, "--vmid", vmid, @@ -525,7 +586,7 @@ virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, if (created && virTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid, privileged, swtpm_user, swtpm_group, - tpm->data.emulator.logfile) < 0) + tpm->data.emulator.logfile, tpm->tpmversion) < 0) goto error;

unlink(tpm->data.emulator.source.data.nix.path); @@ -550,6 +611,14 @@ virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, virCommandSetUID(cmd, swtpm_user); virCommandSetGID(cmd, swtpm_group);

+ switch (tpm->tpmversion) { + case VIR_DOMAIN_TPM_VERSION_1_2: + break; + case VIR_DOMAIN_TPM_VERSION_2: + virCommandAddArg(cmd, "--tpm2"); + break; + } + return cmd;

error: diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.args b/tests/qemuxml2argvdata/tpm-emulator-tpm2.args new file mode 100644 index 0000000..5970928 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.args @@ -0,0 +1,27 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name TPM-VM \ +-S \ +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \ +-m 2048 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,\ +path=/tmp/lib/domain--1-TPM-VM/monitor.sock,server,nowait \ path=/tmp/lib/domain--1-TPM-VM/monitor.sock, can be on the previous line according to syntax-check

...

+-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot order=c,menu=on \ +-usb \ +-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \ +-chardev socket,id=chrtpm,path=/dev/test \ +-device tpm-tis,tpmdev=tpm-tpm0,id=tpm0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml new file mode 100644 index 0000000..a1f39ea --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>TPM-VM</name> + <uuid>11d7cd22-da89-3094-6212-079a48a309a1</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>512288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.12'>hvm</type> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <tpm model='tpm-tis'> + <backend type='emulator' tpmversion='2'/> + </tpm> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a80e3f2..9a0910e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2004,6 +2004,8 @@ mymain(void) QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS); DO_TEST("tpm-emulator", QEMU_CAPS_DEVICE_TPM_EMULATOR, QEMU_CAPS_DEVICE_TPM_TIS); + DO_TEST("tpm-emulator-tpm2", + QEMU_CAPS_DEVICE_TPM_EMULATOR, QEMU_CAPS_DEVICE_TPM_TIS); Another one where I wonder if DO_TEST_CAPS_LATEST should be used.

Move to that macro. Stefan

On 05/04/2018 04:21 PM, Stefan Berger wrote:

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@linux.vnet.ibm.com> --- 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(+)

I have to run for the day, I'll look again in the morning at this one... I did make one note below as I ran the series through Coverity. John

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;

leaks @pidfiledata...

+ } + VIR_FREE(pidfiledata);

Just move this to after cleanup: and you fix the leak.

+ 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); }

On 05/09/2018 11:23 AM, John Ferlan wrote:

On 05/04/2018 04:21 PM, Stefan Berger wrote:

...

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@linux.vnet.ibm.com> --- 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(+)

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.

...

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; }; 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).

...

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 */ + 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.

I'll add this type of check calling qemuExtDevicesHasDevice(). That at least lets us isolate all the external device code.

...

+ /* + * 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) 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.

It probably should be a separate group.

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

Ok, I will have a look

...

+ 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; + }

This here was probably the strongest reason to carry the pidfile around in the tpm->data.emulator.pidfile. I now moved this into a function qemuExtDevicesSetupCgroup() so that I don't have to create this pidfile over here. It should probably have followed the pattern of caling into qemu_extdevice.c before.

...

+ 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); + I think the virPidFile definitely helps here although I haven't dug too deep into those API's.

...

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; +

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.

...

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); +

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.

I changed it towards that now. The patch will look quite different, though.

...

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); + }

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.

swtpm does clean up after itself and libvirt shouldn't need to do that. So, yeah, I will not hold onto this and drop it. Stefan

John

...

VIR_FREE(pathname); VIR_FREE(errbuf); }