Re: [Qemu-devel] [PATCH V8 13/14] Add a TPM backend null driver implementation

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 10:41:04PM -0400, Stefan Berger wrote:
> On 09/01/2011 01:40 PM, Michael S. Tsirkin wrote:
> >On Wed, Aug 31, 2011 at 10:36:04AM -0400, Stefan Berger wrote:
> >>This patch adds a TPM null driver implementation acting as a backend for
> >>the TIS hardware emulation. The NULL driver responds to all commands with
> >>a TPM fault response.
> >>
> >>To use this null driver, use either
> >>
> >>-tpm null
> >>
> >>or
> >>
> >>-tpmdev null,id=tpm0 -device tpm-tis,tpmdev=tpm0
> >>
> >>as parameters on the command line.
> >>
> >>If TPM support is chosen via './configure --enable-tpm ...' TPM support is 
> >>now
> >>always compiled into Qemu and at least the null driver will be available on
> >>emulators for x86_64 and i386.
> >Why limit this to intel platforms then?
> >
> Same as with my previous comment: only hw/pc.c handles the TPM
> device for x86_64 and i386 emulators.
> >>v8:
> >>  - initializing 'in' variable
> >>
> >>Signed-off-by: Stefan Berger<address@hidden>
> >>
> >>---
> >>  Makefile.target |    2
> >>  configure       |    8 -
> >>  hw/tpm_null.c   |  327 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  qemu-options.hx |   13 +-
> >>  tpm.c           |    1
> >>  tpm.h           |    1
> >>  6 files changed, 341 insertions(+), 11 deletions(-)
> >>
> >>Index: qemu-git/hw/tpm_null.c
> >>===================================================================
> >>--- /dev/null
> >>+++ qemu-git/hw/tpm_null.c
> >>@@ -0,0 +1,327 @@
> >>+/*
> >>+ *  builtin 'null' TPM driver
> >Is this the same one an earlier patch removed?
> It's similar, yes, but previously it served to cut the 'builtin'
> backend's code size into two smaller patches.
> >>+ *
> >>+ *  Copyright (c) 2010, 2011 IBM Corporation
> >>+ *  Copyright (c) 2010, 2011 Stefan Berger
> >Why dual copyright btw?
> >
> Not a specific reason...
> >>+ *
> >>+ * 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 of the License, or (at your option) any later version.
> >>+ *
> >>+ * This library is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>+ * Lesser General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU Lesser General Public
> >>+ * License along with this library; if not, 
> >>see<http://www.gnu.org/licenses/>
> >>+ */
> >>+
> >>+#include "qemu-common.h"
> >>+#include "tpm.h"
> >>+#include "hw/hw.h"
> >>+#include "hw/tpm_tis.h"
> >>+#include "hw/pc.h"
> >>+
> >>+
> >>+//#define DEBUG_TPM
> >>+//#define DEBUG_TPM_SR /* suspend - resume */
> >don't use C++ comments please.
> I ran the perl script over the patches and it pointed that out to
> me. I only kept it because I found similar comments for DEBUG
> #defines in other files.
> >>+
> >>+
> >>+/* data structures */
> >>+
> >>+typedef struct ThreadParams {
> >>+    TPMState *tpm_state;
> >>+
> >>+    TPMRecvDataCB *recv_data_callback;
> >>+} ThreadParams;
> >>+
> >>+
> >>+/* local variables */
> >>+
> >>+static QemuThread thread;
> >>+
> >>+static bool thread_terminate;
> >>+static bool thread_running;
> >>+
> >>+static ThreadParams tpm_thread_params;
> >this lacks consistency in naming.
> >prefix everything with tpm_null?
> >
> Ok.
> 
> >>+
> >>+static const unsigned char tpm_std_fatal_error_response[10] = {
> >>+    0x00, 0xc4, 0x00, 0x00, 0x00, 0x0A, 0x00, 0x00, 0x00, 0x09 /* TPM_FAIL 
> >>*/
> >>+};
> >>+
> >>+static char dev_description[80];
> >Use symbolic constants above?
> >
> In this case dev_description could be a constant.
> 
> static char tpm_null_dev_description[] = "Null TPM backend driver";

Right. Same for the tpm_std_fatal_error_response magic numbers ...

> >>+
> >>+
> >>+static void *tpm_null_main_loop(void *d)
> >>+{
> >>+    ThreadParams *thr_parms = d;
> >>+    uint32_t in_len;
> >>+    uint8_t *in, *out;
> >>+    uint8_t locty;
> >>+
> >>+#ifdef DEBUG_TPM
> >>+    fprintf(stderr, "tpm: THREAD IS STARTING\n");
> >>+#endif
> >>+
> >>+    /* start command processing */
> >>+    while (!thread_terminate) {
> >>+        /* receive and handle commands */
> >>+        in_len = 0;
> >>+        do {
> >>+#ifdef DEBUG_TPM
> >>+            fprintf(stderr, "tpm: waiting for commands...\n");
> >>+#endif
> >>+
> >>+            if (thread_terminate) {
> >>+                break;
> >>+            }
> >>+
> >>+            qemu_mutex_lock(&thr_parms->tpm_state->state_lock);
> >>+
> >>+            /* in case we were to slow and missed the signal, the
> >>+               to_tpm_execute boolean tells us about a pending command */
> >>+            if (!thr_parms->tpm_state->to_tpm_execute) {
> >>+                qemu_cond_wait(&thr_parms->tpm_state->to_tpm_cond,
> >>+&thr_parms->tpm_state->state_lock);
> >>+            }
> >>+
> >>+            thr_parms->tpm_state->to_tpm_execute = false;
> >>+
> >>+            qemu_mutex_unlock(&thr_parms->tpm_state->state_lock);
> >>+
> >>+            if (thread_terminate) {
> >>+                break;
> >>+            }
> >>+
> >>+            locty = thr_parms->tpm_state->command_locty;
> >>+
> >>+            in = thr_parms->tpm_state->loc[locty].w_buffer.buffer;
> >>+            in_len = thr_parms->tpm_state->loc[locty].w_offset;
> >>+
> >>+            out = thr_parms->tpm_state->loc[locty].r_buffer.buffer;
> >>+
> >>+            memcpy(out, tpm_std_fatal_error_response,
> >>+                   sizeof(tpm_std_fatal_error_response));
> >>+
> >>+            out[1] = (in_len>  2&&  in[1]>= 0xc1&&  in[1]<= 0xc3)
> >>+                   ? in[1] + 3
> >>+                   : 0xc4;
> >>+#ifdef DEBUG_TPM
> >>+            fprintf(stderr, "tpm_null: sending fault response to VM\n");
> >>+#endif
> >>+            thr_parms->recv_data_callback(thr_parms->tpm_state, locty);
> >>+        } while (in_len>  0);
> >>+    }
> >>+
> >>+#ifdef DEBUG_TPM
> >>+    fprintf(stderr, "tpm: THREAD IS ENDING\n");
> >>+#endif
> >>+
> >>+    thread_running = false;
> >>+
> >>+    return NULL;
> >>+}
> >>+
> >>+
> >>+static void tpm_null_terminate_tpm_thread(void)
> >>+{
> >>+    if (!thread_running) {
> >>+        return;
> >>+    }
> >>+
> >>+#if defined DEBUG_TPM || defined DEBUG_TPM_SR
> >>+    fprintf(stderr, "tpm: TERMINATING RUNNING TPM THREAD\n");
> >>+#endif
> >>+
> >>+    if (!thread_terminate) {
> >>+        thread_terminate = true;
> >>+
> >>+        qemu_mutex_lock(&tpm_thread_params.tpm_state->state_lock);
> >>+        qemu_cond_signal(&tpm_thread_params.tpm_state->to_tpm_cond);
> >>+        qemu_mutex_unlock(&tpm_thread_params.tpm_state->state_lock);
> >>+
> >>+        memset(&thread, 0, sizeof(thread));
> >>+    }
> >>+}
> >>+
> >>+
> >>+static void tpm_null_tpm_atexit(void)
> >>+{
> >>+    tpm_null_terminate_tpm_thread();
> >>+}
> >>+
> >>+
> >>+/**
> >>+ * Start the TPM (thread). If it had been started before, then terminate
> >>+ * and start it again.
> >>+ */
> >>+static int tpm_null_startup_tpm(void)
> >>+{
> >>+    /* terminate a running TPM */
> >>+    tpm_null_terminate_tpm_thread();
> >>+
> >>+    /* reset the flag so the thread keeps on running */
> >>+    thread_terminate = false;
> >>+
> >>+    qemu_thread_create(&thread, tpm_null_main_loop,&tpm_thread_params);
> >>+
> >>+    thread_running = true;
> >>+
> >>+    return 0;
> >>+}
> >>+
> >>+
> >>+static int tpm_null_do_startup_tpm(void)
> >>+{
> >>+    return tpm_null_startup_tpm();
> >>+}
> >>+
> >>+
> >>+static int tpm_null_early_startup_tpm(void)
> >>+{
> >>+    return tpm_null_do_startup_tpm();
> >>+}
> >>+
> >>+
> >>+static int tpm_null_late_startup_tpm(void)
> >>+{
> >>+    return tpm_null_do_startup_tpm();
> >>+}
> >>+
> >>+
> >>+static void tpm_null_reset(void)
> >>+{
> >>+#if defined DEBUG_TPM || defined DEBUG_TPM_SR
> >>+    fprintf(stderr, "tpm: CALL TO TPM_RESET!\n");
> >>+#endif
> >>+
> >>+    tpm_null_terminate_tpm_thread();
> >>+}
> >>+
> >>+
> >>+/*
> >>+ * Since the null driver does not have much persistent storage
> >>+ * there is not much to do here...
> >>+ */
> >>+static int tpm_null_instantiate_with_volatile_data(TPMState *s)
> >>+{
> >>+    if (thread_running) {
> >>+#ifdef DEBUG_TPM_SR
> >>+        fprintf(stderr, "tpm: This is resume of a SNAPSHOT\n");
> >>+#endif
> >>+        tis_reset_for_snapshot_resume(s);
> >>+    }
> >>+
> >>+    return 0;
> >>+}
> >>+
> >>+
> >>+static int tpm_null_init(TPMState *s, TPMRecvDataCB *recv_data_cb)
> >>+{
> >>+    tpm_thread_params.tpm_state = s;
> >>+    tpm_thread_params.recv_data_callback = recv_data_cb;
> >>+
> >>+    atexit(tpm_null_tpm_atexit);
> >>+
> >>+    return 0;
> >>+}
> >>+
> >>+
> >>+static bool tpm_null_get_tpm_established_flag(void)
> >>+{
> >>+    return false;
> >>+}
> >>+
> >>+
> >>+static bool tpm_null_get_startup_error(void)
> >>+{
> >>+    return false;
> >>+}
> >>+
> >>+
> >>+/**
> >>+ * This function is called by tpm_tis.c once the TPM has processed
> >>+ * the last command and returned the response to the TIS.
> >>+ */
> >>+static int tpm_null_save_volatile_data(void)
> >>+{
> >>+    return 0;
> >>+}
> >>+
> >>+
> >>+static size_t tpm_null_realloc_buffer(TPMSizedBuffer *sb)
> >>+{
> >>+    size_t wanted_size = 4096;
> >>+
> >>+    if (sb->size != wanted_size) {
> >>+        sb->buffer = g_realloc(sb->buffer, wanted_size);
> >>+        if (sb->buffer != NULL) {
> >>+            sb->size = wanted_size;
> >>+        } else {
> >>+            sb->size = 0;
> >>+        }
> >>+    }
> >>+    return sb->size;
> >>+}
> >>+
> >>+
> >>+static const char *tpm_null_create_desc(void)
> >>+{
> >>+    static int done;
> >>+
> >>+    if (!done) {
> >>+        snprintf(dev_description, sizeof(dev_description),
> >>+                 "Null TPM backend driver");
> >>+        done = 1;
> >>+    }
> >>+
> >>+    return dev_description;
> >>+}
> >>+
> >>+
> >>+static TPMBackend *tpm_null_create(QemuOpts *opts, const char *id,
> >>+                                      const char *model)
> >>+{
> >>+    TPMBackend *driver;
> >>+
> >>+    driver = g_malloc(sizeof(TPMBackend));
> >>+    if (!driver) {
> >>+        fprintf(stderr, "Could not allocate memory.\n");
> >>+        return NULL;
> >>+    }
> >>+    driver->id = g_strdup(id);
> >>+    if (model) {
> >>+        driver->model = g_strdup(model);
> >>+    }
> >>+    driver->ops =&tpm_null_driver;
> >>+
> >>+    return driver;
> >>+}
> >>+
> >>+
> >>+static void tpm_null_destroy(TPMBackend *driver)
> >>+{
> >>+    g_free(driver->id);
> >>+    g_free(driver->model);
> >>+    g_free(driver);
> >>+}
> >>+
> >>+
> >>+TPMDriverOps tpm_null_driver = {
> >>+    .id                       = "null",
> >>+    .desc                     = tpm_null_create_desc,
> >>+    .job_for_main_thread      = NULL,
> >>+    .create                   = tpm_null_create,
> >>+    .destroy                  = tpm_null_destroy,
> >>+    .init                     = tpm_null_init,
> >>+    .early_startup_tpm        = tpm_null_early_startup_tpm,
> >>+    .late_startup_tpm         = tpm_null_late_startup_tpm,
> >>+    .realloc_buffer           = tpm_null_realloc_buffer,
> >>+    .reset                    = tpm_null_reset,
> >>+    .had_startup_error        = tpm_null_get_startup_error,
> >>+    .save_volatile_data       = tpm_null_save_volatile_data,
> >>+    .load_volatile_data       = tpm_null_instantiate_with_volatile_data,
> >>+    .get_tpm_established_flag = tpm_null_get_tpm_established_flag,
> >>+};
> >>Index: qemu-git/Makefile.target
> >>===================================================================
> >>--- qemu-git.orig/Makefile.target
> >>+++ qemu-git/Makefile.target
> >>@@ -233,7 +233,7 @@ obj-i386-y += debugcon.o multiboot.o
> >>  obj-i386-y += pc_piix.o
> >>  obj-i386-$(CONFIG_KVM) += kvmclock.o
> >>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>-obj-i386-$(CONFIG_TPM) += tpm_tis.o sha1.o
> >>+obj-i386-$(CONFIG_TPM) += tpm_tis.o sha1.o tpm_null.o
> >>  obj-i386-$(CONFIG_TPM_BUILTIN) += tpm_builtin.o
> >>
> >>  ifdef CONFIG_TPM_BUILTIN
> >>Index: qemu-git/tpm.c
> >>===================================================================
> >>--- qemu-git.orig/tpm.c
> >>+++ qemu-git/tpm.c
> >>@@ -24,6 +24,7 @@
> >>  #if defined(TARGET_I386) || defined(TARGET_X86_64)
> >>
> >>  static const TPMDriverOps *bes[] = {
> >>+&tpm_null_driver,
> >>  #ifdef CONFIG_TPM_BUILTIN
> >>      &tpm_builtin,
> >>  #endif
> >>Index: qemu-git/tpm.h
> >>===================================================================
> >>--- qemu-git.orig/tpm.h
> >>+++ qemu-git/tpm.h
> >>@@ -141,6 +141,7 @@ void tpm_measure_buffer(const void *buff
> >>                          TPMMeasureType type, uint8_t pcrindex,
> >>                          const void *data, uint32_t data_len);
> >>
> >>+extern TPMDriverOps tpm_null_driver;
> >>  extern TPMDriverOps tpm_builtin;
> >>
> >>  #endif /* _HW_TPM_CONFIG_H */
> >>Index: qemu-git/qemu-options.hx
> >>===================================================================
> >>--- qemu-git.orig/qemu-options.hx
> >>+++ qemu-git/qemu-options.hx
> >>@@ -1769,6 +1769,8 @@ DEF("tpm", HAS_ARG, QEMU_OPTION_tpm, \
> >>      "-tpm builtin,path=<path>[,model=<model>][,key=<aes key>]\n" \
> >>      "                enable a builtin TPM with state in file in path\n" \
> >>      "                and encrypt the TPM's state with the given AES 
> >> key\n" \
> >>+    "-tpm null       enable a TPM null driver that responds with a 
> >>fault\n" \
> >>+    "                message to every TPM request\n" \
> >>      "-tpm model=?    to list available TPM device models\n" \
> >>      "-tpm ?          to list available TPM backend types\n",
> >>      QEMU_ARCH_I386)
> >>@@ -1784,8 +1786,9 @@ The general form of a TPM device option
> >>
> >>  @item -tpmdev @var{backend} ,address@hidden [,@var{options}]
> >>  @findex -tpmdev
> >>-Backend type must be:
> >>address@hidden
> >>+Backend type must be one of:
> >>address@hidden,
> >>address@hidden
> >>
> >>  The specific backend type will determine the applicable options.
> >>  The @code{-tpmdev} options requires a @code{-device} option.
> >>@@ -1826,6 +1829,12 @@ using AES-CBC encryption scheme supply t
> >>  @example
> >>  -tpmdev 
> >> builtin,id=tpm0,path=<path_to_qcow2>,key=aes-cbc:0x1234567890abcdef01234567890abcdef
> >>  -device tpm-tis,tpmdev=tpm0
> >>  @end example
> >>+
> >>address@hidden -tpmdev null
> >>+
> >>+Creates an instance of a TPM null driver that responds to every command
> >>+with a fault message.
> >>+
> >>  @end table
> >>
> >>  The short form of a TPM device option is:
> >>Index: qemu-git/configure
> >>===================================================================
> >>--- qemu-git.orig/configure
> >>+++ qemu-git/configure
> >>@@ -2593,8 +2593,6 @@ EOF
> >>    libtpms=no
> >>    if compile_prog "" "-ltpms" ; then
> >>      libtpms=yes
> >>-  else
> >>-    tpm_need_pkgs="libtpms development package"
> >>    fi
> >>  fi
> >>
> >>@@ -3598,12 +3596,6 @@ if test "$tpm" = "yes"; then
> >>    if test "$has_tpm" = "1"; then
> >>        if test "$libtpms" = "yes" ; then
> >>            echo "CONFIG_TPM_BUILTIN=y">>  $config_target_mak
> >>-      else
> >>-          echo
> >>-          echo "TPM support cannot be added since no TPM backend can be 
> >>compiled."
> >>-          echo "Please install the $tpm_need_pkgs."
> >>-          echo
> >>-          exit 1
> >>        fi
> >>        echo "CONFIG_TPM=y">>  $config_host_mak
> >>    fi
> >Hmm, so now we remove some code added earlier?
> Yes. I added this for the purpose of bisectability but also
> logically previously the --enable-tpm configure parameter caused
> failure in case libtpms was not found to be installed. Now it
> doesn't fail anymore since at least the null driver doesn't have any
> dependencies and  can always be built.
> >Pls don't do this for new code: this makes review harder, not easier.
> >It makes sense for old code to ensure bisectability.
> So how should I change it? Should I not add this failure notice in
> the first place? How should the build configure script then handle
> the --enable-tpm if it cannot find libtpms and thus cannot fulfill
> the --enable-tpm?
> 
>    Stefan

Simply stuff to configure and makefile in the last patch.
The rest of the code can then be put in the final form directly
without bisectability issues as it does not compile until
fully applied.

-- 
MST