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"
+#include "viralloc.h"
+#include "vircommand.h"
#include "virstring.h"
#include "virerror.h"
#include "viralloc.h"
#include "virfile.h"
+#include "virkmod.h"
+#include "virlog.h"
#include "virtpm.h"
+#include "virutil.h"
+#include "configmake.h"
#define VIR_FROM_THIS VIR_FROM_NONE
+VIR_LOG_INIT("util.tpm")
+
+/*
+ * executables for the swtpm; to be found on the host
+ */
+static char *swtpm_path;
+static char *swtpm_setup;
+static char *swtpm_ioctl;
+
/**
* virTPMCreateCancelPath:
* @devpath: Path to the TPM device
@@ -74,3 +94,517 @@ virTPMCreateCancelPath(const char *devpath)
cleanup:
return path;
}
+
+/*
+ * virTPMEmulatorInit
+ *
+ * Initialize the Emulator functions by searching for necessary
+ * executables that we will use to start and setup the swtpm
+ */
+static int
+virTPMEmulatorInit(void)
+{
+ if (!swtpm_path) {
+ swtpm_path = virFindFileInPath("swtpm");
+ if (!swtpm_path) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not find swtpm 'swtpm' in PATH"));
+ return -1;
+ }
+ if (!virFileIsExecutable(swtpm_path)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("TPM emulator %s is not an executable"),
+ swtpm_path);
+ VIR_FREE(swtpm_path);
+ return -1;
+ }
+ }
+
+ if (!swtpm_setup) {
+ swtpm_setup = virFindFileInPath("swtpm_setup");
+ if (!swtpm_setup) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not find 'swtpm_setup' in PATH"));
+ return -1;
+ }
+ if (!virFileIsExecutable(swtpm_setup)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("'%s' is not an executable"),
+ swtpm_setup);
+ VIR_FREE(swtpm_setup);
+ return -1;
+ }
+ }
+
+ if (!swtpm_ioctl) {
+ swtpm_ioctl = virFindFileInPath("swtpm_ioctl");
+ if (!swtpm_ioctl) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not find swtpm_ioctl in PATH"));
+ return -1;
+ }
+ if (!virFileIsExecutable(swtpm_ioctl)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("swtpm_ioctl program %s is not an executable"),
+ swtpm_ioctl);
+ VIR_FREE(swtpm_ioctl);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * virTPMCreateEmulatorStoragePath
+ *
+ * @swtpmStorageDir: directory for swtpm persistent state
+ * @vmname: The name of the VM for which to create the storage
+ *
+ * Create the swtpm's storage path
+ */
+static char *
+virTPMCreateEmulatorStoragePath(const char *swtpmStorageDir,
+ const char *vmname)
+{
+ char *path = NULL;
+
+ ignore_value(virAsprintf(&path, "%s/%s/tpm1.2", swtpmStorageDir, vmname));
+
+ return path;
+}
+
+/*
+ * virtTPMGetTPMStorageDir:
+ *
+ * @storagepath: directory for swtpm's pesistent state
+ *
+ * Derive the 'TPMStorageDir' from the storagepath by searching
+ * for the last '/'.
+ */
+static char *
+virTPMGetTPMStorageDir(const char *storagepath)
+{
+ const char *tail = strrchr(storagepath, '/');
+ char *path = NULL;
+
+ if (!tail) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get tail of storagedir %s"),
+ storagepath);
+ return NULL;
+ }
+ ignore_value(VIR_STRNDUP(path, storagepath, tail - storagepath));
+
+ return path;
+}
+
+/*
+ * virTPMEmulatorInitStorage
+ *
+ * Initialize the TPM Emulator storage by creating its root directory,
+ * which is typically found in /var/lib/libvirt/tpm.
+ *
+ */
+static int
+virTPMEmulatorInitStorage(const char *swtpmStorageDir)
+{
+ int rc = 0;
+
+ /* allow others to cd into this dir */
+ if (virFileMakePathWithMode(swtpmStorageDir, 0711) < 0) {
+ virReportSystemError(errno,
+ _("Could not create TPM directory %s"),
+ swtpmStorageDir);
+ rc = -1;
+ }
+
+ return rc;
+}
+
+/*
+ * virTPMCreateEmulatorStorage
+ *
+ * @storagepath: directory for swtpm's pesistent state
+ * @created: a pointer to a bool that will be set to true if the
+ * storage was created because it did not exist yet
+ * @swtpm_user: The uid that needs to be able to access the directory
+ * @swtpm_group: The gid that needs to be able to access the directory
+ *
+ * Unless the storage path for the swtpm for the given VM
+ * already exists, create it and make it accessible for the given userid.
+ * Adapt ownership of the directory and all swtpm's state files there.
+ */
+static int
+virTPMCreateEmulatorStorage(const char *storagepath,
+ bool *created,
+ uid_t swtpm_user, gid_t swtpm_group)
+{
+ int ret = -1;
+ char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath);
+
+ if (!swtpmStorageDir)
+ return -1;
+
+ if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0)
+ return -1;
+
+ *created = false;
+
+ if (!virFileExists(storagepath))
+ *created = true;
+
+ if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not create directory %s as %u:%d"),
+ storagepath, swtpm_user, swtpm_group);
+ goto cleanup;
+ }
+
+ if (virFileChownFiles(storagepath, swtpm_user, swtpm_group) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(swtpmStorageDir);
+
+ return ret;
+}
+
+void
+virTPMDeleteEmulatorStorage(virDomainTPMDefPtr tpm)
+{
+ char *path = virTPMGetTPMStorageDir(tpm->data.emulator.storagepath);
+
+ if (path) {
+ ignore_value(virFileDeleteTree(path));
+ VIR_FREE(path);
+ }
+}
+
+/*
+ * virTPMCreateEmulatorSocket:
+ *
+ * @swtpmStateDir: the directory where to create the socket in
+ * @shortName: short and unique name of the domain
+ *
+ * Create the vTPM device name from the given parameters
+ */
+static char *
+virTPMCreateEmulatorSocket(const char *swtpmStateDir, const char *shortName)
+{
+ char *path = NULL;
+
+ ignore_value(virAsprintf(&path, "%s/%s-swtpm.sock", swtpmStateDir,
+ shortName));
+
+ return path;
+}
+
+/*
+ * virTPMEmulatorInitPaths:
+ *
+ * @tpm: TPM definition for an emulator type
+ * @swtpmStorageDir: the general swtpm storage dir which is used as a base
+ * directory for creating VM specific directories
+ * @uuid: the UUID of the VM
+ */
+int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm,
+ const char *swtpmStorageDir,
+ const unsigned char *uuid)
+{
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ virUUIDFormat(uuid, uuidstr);
+
+ VIR_FREE(tpm->data.emulator.storagepath);
+ if (!(tpm->data.emulator.storagepath =
+ virTPMCreateEmulatorStoragePath(swtpmStorageDir, uuidstr)))
+ return -1;
+
+ return 0;
+}
+
+/*
+ * virTPMEmulatorPrepareHost:
+ *
+ * @tpm: tpm definition
+ * @logDir: directory where swtpm writes its logs into
+ * @vmname: name of the VM
+ * @swtpm_user: uid to run the swtpm with
+ * @swtpm_group: gid to run the swtpm with
+ * @swtpmStateDir: directory for swtpm's persistent state
+ * @qemu_user: uid that qemu will run with; we share the socket file with it
+ * @shortName: short and unique name of the domain
+ *
+ * Prepare the log directory for the swtpm and adjust ownership of it and the
+ * log file we will be using. Prepare the state directory where we will share
+ * the socket between tss and qemu users.
+ */
+int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
+ const char *logDir, const char *vmname,
+ uid_t swtpm_user, gid_t swtpm_group,
+ const char *swtpmStateDir,
+ uid_t qemu_user, const char *shortName)
+{
+ int ret = -1;
+
+ if (virTPMEmulatorInit() < 0)
+ return -1;
+
+ /* create log dir ... */
+ if (virFileMakePathWithMode(logDir, 0730) < 0)
+ goto cleanup;
+
+ /* ... and adjust ownership */
+ if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0)
+ goto cleanup;
+
+ /* create logfile name ... */
+ if (virAsprintf(&tpm->data.emulator.logfile, "%s/%s-swtpm.log",
+ logDir, vmname) < 0)
+ goto cleanup;
+
+ /* ... and make sure it can be accessed by swtpm_user */
+ if (virFileExists(tpm->data.emulator.logfile) &&
+ chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
+ virReportSystemError(errno,
+ _("Could not chown on swtpm logfile %s"),
+ tpm->data.emulator.logfile);
+ goto cleanup;
+ }
+
+ /*
+ create our swtpm state dir ...
+ - QEMU user needs to be able to access the socket there
+ - swtpm group needs to be able to create files there
+ - in privileged mode 0570 would be enough, for non-privileged mode
+ we need 0770
+ */
+ if (virDirCreate(swtpmStateDir, 0770, qemu_user, swtpm_group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0)
+ goto cleanup;
+
+ /* create the socket filename */
+ if (!(tpm->data.emulator.source.data.nix.path =
+ virTPMCreateEmulatorSocket(swtpmStateDir, shortName)))
+ goto cleanup;
+ tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX;
+
+ ret = 0;
+
+ cleanup:
+ if (ret)
+ VIR_FREE(tpm->data.emulator.logfile);
+
+ return ret;
+}
+
+/*
+ * virTPMEmulatorRunSetup
+ *
+ * @storagepath: path to the directory for TPM state
+ * @vmname: the name of the VM
+ * @vmuuid: the UUID of the VM
+ * @privileged: whether we are running in privileged mode
+ * @swtpm_user: The userid to switch to when setting up the TPM;
+ * typically this should be the uid of 'tss' or 'root'
+ * @swtpm_group: The group id to switch to
+ * @logfile: The file to write the log into; it must be writable
+ * for the user given by userid or 'tss'
+ *
+ * Setup the external swtpm by creating endorsement key and
+ * certificates for it.
+ */
+static int
+virTPMEmulatorRunSetup(const char *storagepath, const char *vmname,
+ const unsigned char *vmuuid, bool privileged,
+ uid_t swtpm_user, gid_t swtpm_group,
+ const char *logfile)
+{
+ virCommandPtr cmd = NULL;
+ int exitstatus;
+ int rc = 0;
+ char uuid[VIR_UUID_STRING_BUFLEN];
+ char *vmid = NULL;
+ off_t logstart;
+
+ if (!privileged) {
+ return virFileWriteStr(logfile,
+ _("Did not create EK and certificates since "
+ "this requires privileged mode\n"),
+ 0600);
+ }
+
+ cmd = virCommandNew(swtpm_setup);
+ if (!cmd) {
+ rc = -1;
+ goto cleanup;
+ }
+
+ virUUIDFormat(vmuuid, uuid);
+ if (virAsprintf(&vmid, "%s:%s", vmname, uuid) < 0)
+ goto cleanup;
+
+ virCommandSetUID(cmd, swtpm_user);
+ virCommandSetGID(cmd, swtpm_group);
+
+ virCommandAddArgList(cmd,
+ "--tpm-state", storagepath,
+ "--vmid", vmid,
+ "--logfile", logfile,
+ "--createek",
+ "--create-ek-cert",
+ "--create-platform-cert",
+ "--lock-nvram",
+ "--not-overwrite",
+ NULL);
+
+ virCommandClearCaps(cmd);
+
+ /* get size of logfile */
+ logstart = virFileLength(logfile, -1);
+ if (logstart < 0)
+ logstart = 0;
+
+ if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
+ char *buffer = NULL, *errors;
+ off_t loglength = virFileLength(logfile, -1);
+
+ if (loglength > logstart) {
+ ignore_value(virFileReadOffsetQuiet(logfile, logstart,
+ loglength - logstart,
+ &buffer));
+ errors = virStringFilterLines(buffer, "Error:", 160);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not run '%s'. exitstatus: %d;\n"
+ "%s"),
+ swtpm_setup, exitstatus, errors);
+ VIR_FREE(buffer);
+ VIR_FREE(errors);
+ }
+ rc = -1;
+ }
+
+ cleanup:
+ VIR_FREE(vmid);
+ virCommandFree(cmd);
+
+ return rc;
+}
+
+/*
+ * virTPMEmulatorBuildCommand:
+ *
+ * @tpm: TPM definition
+ * @vmname: The name of the VM
+ * @vmuuid: The UUID of the VM
+ * @privileged: whether we are running in privileged mode
+ * @swtpm_user: The uid for the swtpm to run as (drop privileges to from root)
+ * @swtpm_group: The gid for the swtpm to run as
+ *
+ * Create the virCommand use for starting the emulator
+ * Do some initializations on the way, such as creation of storage
+ * and emulator setup.
+ */
+virCommandPtr
+virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname,
+ const unsigned char *vmuuid, bool privileged,
+ uid_t swtpm_user, gid_t swtpm_group)
+{
+ virCommandPtr cmd = NULL;
+ bool created = false;
+
+ if (virTPMCreateEmulatorStorage(tpm->data.emulator.storagepath,
+ &created, swtpm_user, swtpm_group) < 0)
+ return NULL;
+
+ if (created &&
+ virTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid,
+ privileged, swtpm_user, swtpm_group,
+ tpm->data.emulator.logfile) < 0)
+ goto error;
+
+ unlink(tpm->data.emulator.source.data.nix.path);
+
+ cmd = virCommandNew(swtpm_path);
+ if (!cmd)
+ goto error;
+
+ virCommandClearCaps(cmd);
+
+ virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL);
+ virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600",
+ tpm->data.emulator.source.data.nix.path);
+
+ virCommandAddArg(cmd, "--tpmstate");
+ virCommandAddArgFormat(cmd, "dir=%s,mode=0600",
+ tpm->data.emulator.storagepath);
+
+ virCommandAddArg(cmd, "--log");
+ virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.logfile);
+
+ virCommandSetUID(cmd, swtpm_user);
+ virCommandSetGID(cmd, swtpm_group);
+
+ return cmd;
+
+ error:
+ if (created)
+ virTPMDeleteEmulatorStorage(tpm);
+
+ VIR_FREE(tpm->data.emulator.source.data.nix.path);
+ VIR_FREE(tpm->data.emulator.storagepath);
+
+ virCommandFree(cmd);
+
+ return NULL;
+}
+
+/*
+ * virTPMEmulatorStop
+ * @swtpmStateDir: A directory where the socket is located
+ * @shortName: short and unique name of the domain
+ *
+ * Gracefully stop the swptm
+ */
+void
+virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName)
+{
+ virCommandPtr cmd;
+ char *pathname;
+ char *errbuf = NULL;
+
+ if (virTPMEmulatorInit() < 0)
+ return;
+
+ if (!(pathname = virTPMCreateEmulatorSocket(swtpmStateDir, shortName)))
+ return;
+
+ if (!virFileExists(pathname))
+ goto cleanup;
+
+ cmd = virCommandNew(swtpm_ioctl);
+ if (!cmd) {
+ VIR_FREE(pathname);
+ goto cleanup;
+ }
+
+ virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
+
+ virCommandSetErrorBuffer(cmd, &errbuf);
+
+ ignore_value(virCommandRun(cmd, NULL));
+
+ virCommandFree(cmd);
+
+ /* clean up the socket */
+ unlink(pathname);
+
+ cleanup:
+ VIR_FREE(pathname);
+ VIR_FREE(errbuf);
+}
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index b21fc05..63f75b8 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -1,7 +1,7 @@
/*
* virtpm.h: TPM support
*
- * Copyright (C) 2013 IBM Corporation
+ * Copyright (C) 2013,2018 IBM Corporation
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -22,6 +22,37 @@
#ifndef __VIR_TPM_H__
# define __VIR_TPM_H__
+# include "vircommand.h"
+
+typedef struct _virDomainTPMDef virDomainTPMDef;
+typedef virDomainTPMDef *virDomainTPMDefPtr;
+
char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_NOINLINE;
+int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm,
+ const char *swtpmStorageDir,
+ const unsigned char *uuid)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
+ const char *logDir, const char *vmname,
+ uid_t swtpm_user, gid_t swtpm_group,
+ const char *swtpmStateDir,
+ uid_t qemu_user, const char *shortName)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK;
+virCommandPtr virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
+ const char *vmname,
+ const unsigned char *vmuuid,
+ bool privileged,
+ uid_t swtpm_user,
+ gid_t swtpm_group)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+void virTPMEmulatorStop(const char *swtpmStateDir,
+ const char *shortName)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+void virTPMDeleteEmulatorStorage(virDomainTPMDefPtr tpm)
+ ATTRIBUTE_NONNULL(1);
+
#endif /* __VIR_TPM_H__ */
--
2.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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__ */
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 ?
>
>> +#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?
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;
}
+
>> +/*
>> + * 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.
>
>> +
>> + 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.
>
>> + 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/10/2018 03:29 PM, John Ferlan wrote:
>
> 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
Will post v4 soon.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.