subject:"\[libvirt\] \[PATCH 06\/12\] qemu\: Extend QEMU with external TPM support"

Re: [libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

2018-05-24 Thread Ján Tomko


On Wed, May 23, 2018 at 01:59:33PM -0400, Stefan Berger wrote:

On 05/23/2018 11:41 AM, Ján Tomko wrote:

On Tue, May 22, 2018 at 04:44:47PM -0400, Stefan Berger wrote:

+ * @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
+qemuTPMEmulatorPrepareHost(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 (qemuTPMEmulatorInit() < 0)
+    return -1;
+
+    /* create log dir ... allow 'tss' user to cd into it */
+    if (virFileMakePathWithMode(logDir, 0711) < 0)
+    return -1;
+
+    /* ... and adjust ownership */
+    if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0)
+    goto cleanup;
+
+    /* create logfile name ... */
+    if (!tpm->data.emulator.logfile &&
+    virAsprintf(>data.emulator.logfile, "%s/%s-swtpm.log",
+    logDir, vmname) < 0)


This should also use shortName.



The shortName has the ID of the domain in the name. So for short-lived
logs I would say yes. Though this should be a log like the one for the
VM that gets appended to every time the VM restarts. I'd rather not
change this.



My concern was the file name length, but even for qemu.logs we use
vm->def->name directly. So this should probably be okay.

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

2018-05-23 Thread Stefan Berger


On 05/23/2018 11:41 AM, Ján Tomko wrote:

On Tue, May 22, 2018 at 04:44:47PM -0400, Stefan Berger wrote:
Implement 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.

Further, the external TPM needs storage on the host that we need to set
up before it can be run. We can clean up the host once the domain is
undefined.

This patch also implements a small layer for external device support 
that

calls into the TPM device layer if a domain has an attached TPM. This is
the layer we will wire up later on.



Later on meaning in other patch series? Adding it for just one device
seems excessive, but that might be my personal opinion.


Signed-off-by: Stefan Berger 
Reviewed-by: John Ferlan 
---
src/qemu/Makefile.inc.am  |   4 +
src/qemu/qemu_domain.c    |   2 +
src/qemu/qemu_extdevice.c | 154 ++
src/qemu/qemu_extdevice.h |  53 
src/qemu/qemu_process.c   |  12 +
src/qemu/qemu_tpm.c   | 751 
++

src/qemu/qemu_tpm.h   |  50 +++
7 files changed, 1026 insertions(+)
create mode 100644 src/qemu/qemu_extdevice.c
create mode 100644 src/qemu/qemu_extdevice.h
create mode 100644 src/qemu/qemu_tpm.c
create mode 100644 src/qemu/qemu_tpm.h




+/*
+ * virtTPMGetTPMStorageDir:
+ *
+ * @storagepath: directory for swtpm's persistent state
+ *
+ * Derive the 'TPMStorageDir' from the storagepath by searching
+ * for the last '/'.
+ */
+static char *
+qemuTPMGetTPMStorageDir(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;
+}


In other places we already use mdir_name from gnulib for this
functionality.



I didn't know about this API. I will change it to that.





+/*
+ * qemuTPMCreateEmulatorStorage
+ *
+ * @storagepath: directory for swtpm's persistent state
+ * @created: a pointer to a bool that will be set to true if the
+ *   storage was created because it did not exist yet


Can't the caller call virFileExists and act accordingly?



We could, except that we have to call this function since it is 
adjusting access rights to files for the swtpm_user and swtpm_group.






+ * @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
+qemuTPMEmulatorPrepareHost(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 (qemuTPMEmulatorInit() < 0)
+    return -1;
+
+    /* create log dir ... allow 'tss' user to cd into it */
+    if (virFileMakePathWithMode(logDir, 0711) < 0)
+    return -1;
+
+    /* ... and adjust ownership */
+    if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0)
+    goto cleanup;
+
+    /* create logfile name ... */
+    if (!tpm->data.emulator.logfile &&
+    virAsprintf(>data.emulator.logfile, "%s/%s-swtpm.log",
+    logDir, vmname) < 0)


This should also use shortName.



The shortName has the ID of the domain in the name. So for short-lived 
logs I would say yes. Though this should be a log like the one for the 
VM that gets appended to every time the VM restarts. I'd rather not 
change this.






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

Re: [libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

2018-05-23 Thread Ján Tomko


On Tue, May 22, 2018 at 04:44:47PM -0400, Stefan Berger wrote:

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

Further, the external TPM needs storage on the host that we need to set
up before it can be run. We can clean up the host once the domain is
undefined.

This patch also implements a small layer for external device support that
calls into the TPM device layer if a domain has an attached TPM. This is
the layer we will wire up later on.



Later on meaning in other patch series? Adding it for just one device
seems excessive, but that might be my personal opinion.


Signed-off-by: Stefan Berger 
Reviewed-by: John Ferlan 
---
src/qemu/Makefile.inc.am  |   4 +
src/qemu/qemu_domain.c|   2 +
src/qemu/qemu_extdevice.c | 154 ++
src/qemu/qemu_extdevice.h |  53 
src/qemu/qemu_process.c   |  12 +
src/qemu/qemu_tpm.c   | 751 ++
src/qemu/qemu_tpm.h   |  50 +++
7 files changed, 1026 insertions(+)
create mode 100644 src/qemu/qemu_extdevice.c
create mode 100644 src/qemu/qemu_extdevice.h
create mode 100644 src/qemu/qemu_tpm.c
create mode 100644 src/qemu/qemu_tpm.h




+/*
+ * virtTPMGetTPMStorageDir:
+ *
+ * @storagepath: directory for swtpm's persistent state
+ *
+ * Derive the 'TPMStorageDir' from the storagepath by searching
+ * for the last '/'.
+ */
+static char *
+qemuTPMGetTPMStorageDir(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;
+}


In other places we already use mdir_name from gnulib for this
functionality.


+/*
+ * qemuTPMCreateEmulatorStorage
+ *
+ * @storagepath: directory for swtpm's persistent state
+ * @created: a pointer to a bool that will be set to true if the
+ *   storage was created because it did not exist yet


Can't the caller call virFileExists and act accordingly?


+ * @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
+qemuTPMEmulatorPrepareHost(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 (qemuTPMEmulatorInit() < 0)
+return -1;
+
+/* create log dir ... allow 'tss' user to cd into it */
+if (virFileMakePathWithMode(logDir, 0711) < 0)
+return -1;
+
+/* ... and adjust ownership */
+if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0)
+goto cleanup;
+
+/* create logfile name ... */
+if (!tpm->data.emulator.logfile &&
+virAsprintf(>data.emulator.logfile, "%s/%s-swtpm.log",
+logDir, vmname) < 0)


This should also use shortName.


+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 &&
+!(tpm->data.emulator.source.data.nix.path =
+  qemuTPMCreateEmulatorSocket(swtpmStateDir, shortName)))
+goto cleanup;
+tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX;
+
+ret = 0;
+
+ cleanup:
+
+return ret;
+}
+


[...]


+static int

[libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

2018-05-22 Thread Stefan Berger

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

Further, the external TPM needs storage on the host that we need to set
up before it can be run. We can clean up the host once the domain is
undefined.

This patch also implements a small layer for external device support that
calls into the TPM device layer if a domain has an attached TPM. This is
the layer we will wire up later on.

Signed-off-by: Stefan Berger 
Reviewed-by: John Ferlan 
---
 src/qemu/Makefile.inc.am  |   4 +
 src/qemu/qemu_domain.c|   2 +
 src/qemu/qemu_extdevice.c | 154 ++
 src/qemu/qemu_extdevice.h |  53 
 src/qemu/qemu_process.c   |  12 +
 src/qemu/qemu_tpm.c   | 751 ++
 src/qemu/qemu_tpm.h   |  50 +++
 7 files changed, 1026 insertions(+)
 create mode 100644 src/qemu/qemu_extdevice.c
 create mode 100644 src/qemu/qemu_extdevice.h
 create mode 100644 src/qemu/qemu_tpm.c
 create mode 100644 src/qemu/qemu_tpm.h

diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 7f50501f18..46797af4be 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 \
@@ -51,6 +53,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_security.h \
qemu/qemu_qapi.c \
qemu/qemu_qapi.h \
+   qemu/qemu_tpm.c \
+   qemu/qemu_tpm.h \
$(NULL)
 
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ca5b2c3485..bbf52baa0e 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"
@@ -7217,6 +7218,7 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
 VIR_WARN("unable to remove snapshot directory %s", snapDir);
 VIR_FREE(snapDir);
 }
+qemuExtDevicesCleanupHost(driver, vm->def);
 
 virDomainObjListRemove(driver->domains, vm);
 
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
new file mode 100644
index 00..790b19be9e
--- /dev/null
+++ b/src/qemu/qemu_extdevice.c
@@ -0,0 +1,154 @@
+/*
+ * 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
+ * .
+ *
+ * Author: Stefan Berger 
+ */
+
+#include 
+
+#include "qemu_extdevice.h"
+#include "qemu_domain.h"
+#include "qemu_tpm.h"
+
+#include "viralloc.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "virtime.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+VIR_LOG_INIT("qemu.qemu_extdevice")
+
+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(, "%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;
+}
+
+
+/*
+ * 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.
+ */
+static int
+qemuExtDevicesInitPaths(virQEMUDriverPtr driver,
+virDomainDefPtr def)
+{
+int ret = 0;
+
+if (def->tpm)
+ret =

Re: [libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

Re: [libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

Re: [libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

[libvirt] [PATCH 06/12] qemu: Extend QEMU with external TPM support

4 matches

Site Navigation

Mail list logo

Footer information