Re: [libvirt] [PATCH v3 07/14] util: Extend virtpm.c with tpm-emulator support

9 May 2018

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

    