[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: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support |
Date: |
Mon, 5 Mar 2018 12:43:47 +0000 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
* Stefan Berger (address@hidden) wrote:
> 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 ?
Oh, not a device, hmmm that's odd; yeh probably best then.
Dave
>
> Thanks for the review.
>
> Stefan
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support, Marc-André Lureau, 2018/03/28
[Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support, Stefan Berger, 2018/03/01