qemu-devel
[Top][All Lists]
Advanced

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

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


From: Stefan Berger
Subject: Re: [Qemu-devel] [PATCH V8 13/14] Add a TPM backend null driver implementation
Date: Thu, 01 Sep 2011 22:41:04 -0400
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.18) Gecko/20110621 Fedora/3.1.11-1.fc14 Lightning/1.0b3pre Thunderbird/3.1.11

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";
+
+
+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




reply via email to

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