qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migr


From: Stefan Berger
Subject: Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support
Date: Fri, 2 Mar 2018 21:52:35 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

On 03/02/2018 05:14 AM, Dr. David Alan Gilbert wrote:
* Marc-André Lureau (address@hidden) wrote:
Hi Stefan

On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
<address@hidden> wrote:
Extend the TPM emulator backend device with state migration support.

The external TPM emulator 'swtpm' provides a protocol over
its control channel to retrieve its state blobs. We implement
functions for getting and setting the different state blobs.
In case the setting of the state blobs fails, we return a
negative errno code to fail the start of the VM.

Since we have an external TPM emulator, we need to make sure
that we do not migrate the state for as long as it is busy
processing a request. We need to wait for notification that
the request has completed processing.
Because qemu will have to deal with an external state, managed by the
tpm emulator (different from other storage/nvram):

- the emulator statedir must be different between source and dest? Can
it be the same?
- what is the story regarding versionning? Can you migrate from/to any
version, or only the same version?
- can the host source and destination be of different endianess?
- how is suspend and snapshot working?

There are probably other complications that I can't think of
immediately, but David or Juan help may help avoid mistakes.
Yes, I think that just about covers it.

It probably deserves a few explanations in docs/specs/tpm.txt.

Signed-off-by: Stefan Berger <address@hidden>
---
  hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
It would be worth to add some basic tests. Either growing our own
tests/tpm-emu.c test emulator

or checking that swtpm is present (and of required version?) to
run/skip the test a more complete test.

  1 file changed, 302 insertions(+), 10 deletions(-)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index b787aee..da877e5 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -55,6 +55,19 @@
  #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == 
(cap))

  /* data structures */
+
+/* blobs from the TPM; part of VM state when migrating */
+typedef struct TPMBlobBuffers {
+    uint32_t permanent_flags;
+    TPMSizedBuffer permanent;
+
+    uint32_t volatil_flags;
+    TPMSizedBuffer volatil;
+
+    uint32_t savestate_flags;
+    TPMSizedBuffer savestate;
+} TPMBlobBuffers;
+
  typedef struct TPMEmulator {
      TPMBackend parent;

@@ -70,6 +83,8 @@ typedef struct TPMEmulator {

      unsigned int established_flag:1;
      unsigned int established_flag_cached:1;
+
+    TPMBlobBuffers state_blobs;
Suspicious addition, it shouldn't be necessary in running state. It
could be allocated on demand? Even better if the field is removed, but
this may not be possible with VMSTATE macros.

  } TPMEmulator;


@@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
      return 0;
  }

-static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
+                                     bool is_resume)
Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?

  {
      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
      ptm_init init = {
@@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, 
size_t buffersize)
      };
      ptm_res res;

+    DPRINTF("%s   is_resume: %d", __func__, is_resume);
+
extra spaces, you may also add buffersize while at it.
Also it would be good to use trace_'s where possible rather than
DPRINTFs.

Here's a branch where I am doing that now:

https://github.com/stefanberger/qemu-tpm/commits/tracing

I would leave a DEBUG_TIS in the tpm_tis.c, though.

That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
(and we could drop TPMSizedBuffer).
Also, given this is an arbitrary sized chunk, this should probably use
g_try_malloc and check the result rather than letting g_malloc assert on
failure (especially true on the source side).

Will fix. Thanks.



+    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
+    if (n != totlength) {
+        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
+                     type, strerror(errno));
+        return -1;
+    }
+    tsb->size = totlength;
+
+    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
+            type, tsb->size, *flags);
+
+    return 0;
+}
+
+static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
+{
+    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
+                                    &state_blobs->permanent,
+                                    &state_blobs->permanent_flags) < 0 ||
+        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
+                                    &state_blobs->volatil,
+                                    &state_blobs->volatil_flags) < 0 ||
+        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
+                                    &state_blobs->savestate,
+                                    &state_blobs->savestate_flags) < 0) {
+        goto err_exit;
+    }
+
+    return 0;
+
+ err_exit:
+    tpm_sized_buffer_reset(&state_blobs->volatil);
+    tpm_sized_buffer_reset(&state_blobs->permanent);
+    tpm_sized_buffer_reset(&state_blobs->savestate);
+
+    return -1;
+}
+
+/*
+ * Transfer a TPM state blob to the TPM emulator.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of TPM state blob to transfer
+ * @tsb: TPMSizeBuffer containing the TPM state blob
+ * @flags: Flags describing the (encryption) state of the TPM state blob
+ */
+static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
+                                       uint32_t type,
+                                       TPMSizedBuffer *tsb,
+                                       uint32_t flags)
+{
+    uint32_t offset = 0;
+    ssize_t n;
+    ptm_setstate pss;
+    ptm_res tpm_result;
+
+    if (tsb->size == 0) {
+        return 0;
+    }
+
+    pss = (ptm_setstate) {
+        .u.req.state_flags = cpu_to_be32(flags),
+        .u.req.type = cpu_to_be32(type),
+        .u.req.length = cpu_to_be32(tsb->size),
+    };
+
+    /* write the header only */
+    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
+                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
+        error_report("tpm-emulator: could not set state blob type %d : %s",
+                     type, strerror(errno));
+        return -1;
+    }
+
+    /* now the body */
+    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
+                              &tsb->buffer[offset], tsb->size);
+    if (n != tsb->size) {
+        error_report("tpm-emulator: Writing the stateblob (type %d) "
+                     "failed: %s", type, strerror(errno));
+        return -1;
+    }
+
+    /* now get the result */
+    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
+                             (uint8_t *)&pss, sizeof(pss.u.resp));
+    if (n != sizeof(pss.u.resp)) {
+        error_report("tpm-emulator: Reading response from writing stateblob "
+                     "(type %d) failed: %s", type, strerror(errno));
+        return -1;
+    }
+
+    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
+    if (tpm_result != 0) {
+        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
+                     "with a TPM error 0x%x", type, tpm_result);
+        return -1;
+    }
+
+    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
+            type, tsb->size, flags);
+
+    return 0;
+}
+
+/*
+ * Set all the TPM state blobs.
+ *
+ * Returns a negative errno code in case of error.
+ */
+static int tpm_emulator_set_state_blobs(TPMBackend *tb)
+{
+    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+    DPRINTF("%s: %d", __func__, __LINE__);
+
+    if (tpm_emulator_stop_tpm(tb) < 0) {
+        return -EIO;
+    }
+
+    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
+                                    &state_blobs->permanent,
+                                    state_blobs->permanent_flags) < 0 ||
+        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
+                                    &state_blobs->volatil,
+                                    state_blobs->volatil_flags) < 0 ||
+        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
+                                    &state_blobs->savestate,
+                                    state_blobs->savestate_flags) < 0) {
+        return -EBADMSG;
+    }
+
+    DPRINTF("DONE SETTING STATEBLOBS");
UPPERCASES!

+
+    return 0;
+}
+
+static int tpm_emulator_pre_save(void *opaque)
+{
+    TPMBackend *tb = opaque;
+    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+
+    DPRINTF("%s", __func__);
+
+    tpm_backend_finish_sync(tb);
+
+    /* get the state blobs from the TPM */
+    return tpm_emulator_get_state_blobs(tpm_emu);
+}
+
+/*
+ * Load the TPM state blobs into the TPM.
+ *
+ * Returns negative errno codes in case of error.
+ */
+static int tpm_emulator_post_load(void *opaque,
+                                  int version_id __attribute__((unused)))
unnecessary attribute

+{
+    TPMBackend *tb = opaque;
+    int ret;
+
+    ret = tpm_emulator_set_state_blobs(tb);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
+        return -EIO;
+    }
+
I dislike the mix of functions returning errno and others, and the
lack of Error **, but it's not specific to this patch. Ok.  :)

+    return 0;
+}
+
+static const VMStateDescription vmstate_tpm_emulator = {
+    .name = "tpm-emulator",
+    .version_id = 1,
+    .minimum_version_id = 0,
version_id = 1 & minimum_version_id = 0 ?

It's the first version, let's have version_id = 0 (and you could
remove minimum_version).

+    .minimum_version_id_old = 0,
No need to specify that, no load_state_old() either

+    .pre_save = tpm_emulator_pre_save,
+    .post_load = tpm_emulator_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
+        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
+        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
+                                     TPMEmulator, 1, 0,
+                                     state_blobs.permanent.size),
+
+        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
+        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
+        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
+                                     TPMEmulator, 1, 0,
+                                     state_blobs.volatil.size),
+
+        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
+        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
+        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
+                                     TPMEmulator, 1, 0,
+                                     state_blobs.savestate.size),
+
+        VMSTATE_END_OF_LIST()
+    }
+};
+
  static void tpm_emulator_inst_init(Object *obj)
  {
      TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
@@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
      tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
      tpm_emu->cur_locty_number = ~0;
      qemu_mutex_init(&tpm_emu->mutex);
+
+    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
  }

  /*
@@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
      }

      qemu_mutex_destroy(&tpm_emu->mutex);
+
+    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
It's best to avoid the vmstate_register/unregister and just set the
dc->vmsd pointer during class_init; see hw/virtio/virtio-balloon.c
virtio_balloon_class_init for an example.

hw/tpm/tpm_emulator.c:909:tpm_emulator_class_init: Object 0x55f4cbc72680 is not an instance of type device
Aborted

Can we leave it as it is ?


Thanks for the review.

   Stefan




reply via email to

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