[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 1/6] Provide support for the CUSE TPM

From: Stefan Berger
Subject: Re: [Qemu-devel] [PATCH v3 1/6] Provide support for the CUSE TPM
Date: Tue, 26 May 2015 21:53:10 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 05/26/2015 07:05 PM, Eric Blake wrote:
On 05/26/2015 03:33 PM, Stefan Berger wrote:
Rather than integrating TPM functionality into QEMU directly
using the TPM emulation of libtpms, we now integrate an external
emulated TPM device. This device is expected to implement a Linux
CUSE interface (CUSE = character device in userspace).

QEMU talks to the CUSE TPM using much functionality of the
passthrough driver. For example, the TPM commands and responses
are sent to the CUSE TPM using the read()/write() interface.
However, some out-of-band control needs to be done using the CUSE
TPM's ioctl's. The CUSE TPM currently defines and implements 14
different ioctls for controlling certain life-cycle aspects of
the emulated TPM. The ioctls can be regarded as a replacement for
direct function calls to a TPM emulator if the TPM were to be
directly integrated into QEMU.

One  of the ioctl's allows to get a bitmask of supported capabilities.
Each returned bit indicates which capabilties have been implemented.

An include file defining the various ioctls is added to QEMU.

The CUSE TPM and associated tools can be found here:


To use the external CUSE TPM, the CUSE TPM should be started as follows:

/usr/bin/swtpm_cuse -n vtpm-test

QEMU can then be started using the following parameters:

qemu-system-x86_64 \
        [...] \
         -tpmdev cuse-tpm,id=tpm0,cancel-path=/dev/null,path=/dev/vtpm-test \
         -device tpm-tis,id=tpm0,tpmdev=tpm0 \

Signed-off-by: Stefan Berger <address@hidden>
Cc: Eric Blake <address@hidden>
At this point, I'm only doing a high-level overview (public interface,
blatant findings) and not a fine-grained reading of the implementation.

Thanks anyway.

diff --git a/hw/tpm/tpm_ioctl.h b/hw/tpm/tpm_ioctl.h
new file mode 100644
index 0000000..d36e702
--- /dev/null
+++ b/hw/tpm/tpm_ioctl.h
@@ -0,0 +1,178 @@
+ * tpm_ioctl.h
+ *
+ * This file is licensed 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.
+ */
My understanding of copyleft (and insert the obligatory IANAL
disclaimer) is that it works by exploiting the copyright law - that is,
something cannot be [L]GPL unless there is also an assertion of
copyright ownership (I don't care who, merely that a copyright claim
exists somewhere in the file, nearby the license).

I added the copyright now, which was obviously missing.

+ * Data structure to get state blobs from the TPM. If the size of the
+ * blob exceeds the STATE_BLOB_SIZE, multiple reads with
+ * adjusted offset are necessary. The last packet is indicated by
+ * the length being smaller than the STATE_BLOB_SIZE.
If the read size is exactly STATE_BLOB_SIZE, does that result in 1
packet or 2?  Does it cause a 0-length packet to be attempted, or is it
broken into STATE_BLOB_SIZE-1 and 1?

It would be 2 packets, the 2nd one having 0-length.

+/* state_flags above : */
+#define STATE_FLAG_DECRYPTED     1 /* on input:  get decrypted state */
+#define STATE_FLAG_ENCRYPTED     2 /* on output: state is encrytped */

+ * Data structure to set state blobs in the TPM. If the size of the
+ * blob exceeds the STATE_BLOB_SIZE, multiple 'writes' are necessary.
+ * The last packet is indicated by the length being smaller than the
+ */
Same question as on read about an exact STATE_BLOB_SIZE write

Same here. 2 packets.

+struct ptm_setstate {
+    union {
+        struct {
+            uint32_t state_flags; /* may be STATE_FLAG_ENCRYPTED */
+            uint32_t tpm_number;  /* always set to 0 */
+            uint8_t type;         /* which blob to set */
+            uint32_t length;
+            uint8_t data[STATE_BLOB_SIZE];
This struct has padding blanks; is that going to matter?

The problem here could be a 64bit variable that would allign differently on a 32bit machine versus a 64bit machine or a 32bit executable running on a 64bit machine.At least there are no 64bit variables here, so it would be ok. However, we can still make the type member 32bit.

+typedef uint64_t ptmcap_t;
+typedef struct ptmest  ptmest_t;
+typedef struct ptmreset_est ptmreset_est_t;
+typedef struct ptmloc  ptmloc_t;
+typedef struct ptmhdata ptmhdata_t;
Why a change in 1 vs. 2 spaces on some of the types?

Technically, POSIX reserves the entire *_t namespace to itself, I'm a
bit worried that by doing 'typedef struct foo foo_t' we are not being
consistent with the rest of qemu, which does 'typedef struct foo foo'.

So remove the _t entirely?

+++ b/hw/tpm/tpm_passthrough.c
@@ -72,12 +74,18 @@ struct TPMPassthruState {
      bool had_startup_error;
TPMVersion tpm_version;
+    ptmcap_t cuse_cap; /* capabilties of the CUSE TPM */
+    uint8_t cur_locty_number; /* last set locality */

typedef struct TPMPassthruState TPMPassthruState; #define TPM_PASSTHROUGH_DEFAULT_DEVICE "/dev/tpm0" +#define TPM_PASSTHROUGH_USES_CUSE_TPM(tpm_pt) (tpm_pt->cuse_cap != 0)
+#define TPM_CUSE_IMPLEMENTS(tpm_tr, cap) ((tpm_pt->cuse_cap & cap) == cap)
Evaluates cap more than once, which may not be ideal.  Also
under-parenthesized in the face of arbitrary expressions for tpm_tr or cap.

Umm, how does the macro argument tpm_tr get used, and where is the macro
body tpm_pt scoped?

Better might be this (depending on your intent):
#define TPM_CUSE_IMPLEMENTS(tpm_tr, cap) \
    (((tpm_tr)->cuse_cap & (cap)) != 0)

if you know that cap will always be passed as one bit.  But if someone
intends to use the macro to test multiple bits at once, and return true
only if all of the bits are set, then living with multiple evaluation of
'cap' may be better.

The usage so far asks for whether a certain set of capabilities are _all_ implemented and for this the evaluation above is good in call cases. I'll add the additional parenthesis, though.

+static int tpm_passthrough_set_locality(TPMPassthruState *tpm_pt,
+                                        uint8_t locty_number)
+    int n;
+    ptmloc_t loc;
+        if (tpm_pt->cur_locty_number != locty_number) {
+            loc.u.req.loc = locty_number;
+            n = ioctl(tpm_pt->tpm_fd, PTM_SET_LOCALITY, &loc);
+            if (n < 0) {
+                error_report("tpm_cuse: could not set locality on "
+                             "CUSE TPM: %s (%i)",
+                             strerror(errno), errno);
Hmm, I wonder if error_setg_errno() followed by error_report_err() is
any nicer than manually calling strerror().  Probably not worth worrying

On the other hand, this code is not strictly portable - passing both
errno and strerror(errno) as arguments to a function has no sequencing
point defined on whether errno is collected first or second; if it is
collected second, strerror() may have clobbered errno.  Most code
doesn't bother with printing "%s (%i)" for errors; the %s alone is


+ * Gracefully shut down the external CUSE TPM
+ */
+static void tpm_passthrough_shutdown(TPMPassthruState *tpm_pt)
+    int n;
+    ptmres_t res;
+        n = ioctl(tpm_pt->tpm_fd, PTM_SHUTDOWN, &res);
+        if (n < 0) {
+            error_report("tpm_cuse: Could not cleanly shut down "
+                         "the CUSE TPM: %s (%i)",
+                         strerror(errno), errno);
Why not just 'if (ioctl(...) < 0) {' without needing 'n'?

Thought it was a coding style requirement .. but it isn't.

+        }
+    }
+ * Probe for the CUSE TPM by sending an ioctl() requesting its
+ * capability flags.
+ */
+static int tpm_passthrough_cuse_probe(TPMPassthruState *tpm_pt)
+    int rc = 0;
+    int n;
+    n = ioctl(tpm_pt->tpm_fd, PTM_GET_CAPABILITY, &tpm_pt->cuse_cap);
+    if (n < 0) {
+        error_report("Error: CUSE TPM was requested, but probing failed.");
Most qemu error messages intentionally do not end in period


@@ -306,6 +472,8 @@ static void tpm_passthrough_cancel_cmd(TPMBackend *tb)
      TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
      int n;
+    ptmres_t res;
+    static int error_printed;
You're using this as a bool...

+                } else if (res != TPM_SUCCESS) {
+                    if (!error_printed) {
+                        error_report("TPM error code from command "
+                                     "cancellation of CUSE TPM: 0x%x", res);
+                        error_printed = true;
+                    }
...so declare it as one.


+++ b/qapi-schema.json
@@ -2974,10 +2974,11 @@
  # An enumeration of TPM types
  # @passthrough: TPM passthrough type
+# @cuse-tpm: CUSE TPM type
Missing '(since 2.4)' designator.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]