[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator
|
From: |
Valluri, Amarnath |
|
Subject: |
Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator |
|
Date: |
Tue, 26 Sep 2017 13:28:28 +0000 |
On Tue, 2017-09-26 at 14:24 +0200, Marc-André Lureau wrote:
> Hi
>
> On Tue, Sep 26, 2017 at 2:05 PM, Valluri, Amarnath
> <address@hidden> wrote:
> >
> > Hi Marc,
> >
> > Thanks for your time reviewing this patchset. Please find my inline
> > comments.
> >
> > On Sun, 2017-09-24 at 20:52 +0200, Marc-André Lureau wrote:
> > >
> > > Hi
> > >
> > > Thanks for the nice update, removing the exec() code, using
> > > chardev
> > > and a private socketpair. Some comments below:
> > >
> > > On Fri, Sep 22, 2017 at 2:33 PM, Amarnath Valluri
> > > <address@hidden> wrote:
> > > >
> > > >
> > > > This change introduces a new TPM backend driver that can
> > > > communicate with
> > > > swtpm(software TPM emulator) using unix domain socket
> > > > interface.
> > > > QEMU talks to
> > > > TPM emulator using socket based chardev backend device.
> > > >
> > > > Swtpm uses two Unix sockets for communications, one for plain
> > > > TPM
> > > > commands and
> > > > responses, and one for out-of-band control messages. QEMU
> > > > passes
> > > > data socket
> > > > been used over the control channel.
> > > >
> > > > The swtpm and associated tools can be found here:
> > > > https://github.com/stefanberger/swtpm
> > > >
> > > > The swtpm's control channel protocol specification can be found
> > > > here:
> > > > https://github.com/stefanberger/swtpm/wiki/Control-Channel-
> > > > Spec
> > > > ification
> > > >
> > > > Usage:
> > > > # setup TPM state directory
> > > > mkdir /tmp/mytpm
> > > > chown -R tss:root /tmp/mytpm
> > > > /usr/bin/swtpm_setup --tpm-state /tmp/mytpm --createek
> > > >
> > > > # Ask qemu to use TPM emulator with given tpm state
> > > > directory
> > > > qemu-system-x86_64 \
> > > > [...] \
> > > > -chardev socket,id=chrtpm,path=/tmp/swtpm-sock \
> > > > -tpmdev emulator,id=tpm0,chardev=chrtpm \
> > > > -device tpm-tis,tpmdev=tpm0 \
> > > > [...]
> > > >
> > > > Signed-off-by: Amarnath Valluri <address@hidden>
> > > > ---
> > > > configure | 15 +-
> > > > hmp.c | 5 +
> > > > hw/tpm/Makefile.objs | 1 +
> > > > hw/tpm/tpm_emulator.c | 649
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > hw/tpm/tpm_ioctl.h | 246 +++++++++++++++++++
> > > > qapi/tpm.json | 21 +-
> > > > qemu-options.hx | 22 +-
> > > > 7 files changed, 950 insertions(+), 9 deletions(-)
> > > > create mode 100644 hw/tpm/tpm_emulator.c
> > > > create mode 100644 hw/tpm/tpm_ioctl.h
> > > >
> > > > diff --git a/configure b/configure
> > > > index cb0f7ed..ce2df2d 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -3461,10 +3461,15 @@ fi
> > > > ##########################################
> > > > # TPM passthrough is only on x86 Linux
> > > >
> > > > -if test "$targetos" = Linux && test "$cpu" = i386 -o "$cpu" =
> > > > x86_64; then
> > > > - tpm_passthrough=$tpm
> > > > +if test "$targetos" = Linux; then
> > > > + tpm_emulator=$tpm
> > > > + if test "$cpu" = i386 -o "$cpu" = x86_64; then
> > > > + tpm_passthrough=$tpm
> > > > + else
> > > > + tpm_passthrough=no
> > > > + fi
> > > > else
> > > > - tpm_passthrough=no
> > > > + tpm_emulator=no
> > > > fi
> > > >
> > > > ##########################################
> > > > @@ -5359,6 +5364,7 @@ echo "gcov enabled $gcov"
> > > > echo "TPM support $tpm"
> > > > echo "libssh2 support $libssh2"
> > > > echo "TPM passthrough $tpm_passthrough"
> > > > +echo "TPM emulator $tpm_emulator"
> > > > echo "QOM debugging $qom_cast_debug"
> > > > echo "Live block migration $live_block_migration"
> > > > echo "lzo support $lzo"
> > > > @@ -5943,6 +5949,9 @@ if test "$tpm" = "yes"; then
> > > > if test "$tpm_passthrough" = "yes"; then
> > > > echo "CONFIG_TPM_PASSTHROUGH=y" >> $config_host_mak
> > > > fi
> > > > + if test "$tpm_emulator" = "yes"; then
> > > > + echo "CONFIG_TPM_EMULATOR=y" >> $config_host_mak
> > > It shouldn't require Linux, but posix (and I assume a port to
> > > other
> > > systems isn't impossible). same for build-sys / help / comments.
> > I agree, Can you suggest, what is the Qemu way of limiting this to
> > 'posix'.
> > >
> > >
> there is CONFIG_POSIX
Ok, thanks. I will check
>
> >
> > >
> > > >
> > > >
> > > > + fi
> > > > fi
> > > >
> > > > echo "TRACE_BACKENDS=$trace_backends" >> $config_host_mak
> > > > diff --git a/hmp.c b/hmp.c
> > > > index cf62b2e..7e69eca 100644
> > > > --- a/hmp.c
> > > > +++ b/hmp.c
> > > > @@ -995,6 +995,7 @@ void hmp_info_tpm(Monitor *mon, const QDict
> > > > *qdict)
> > > > Error *err = NULL;
> > > > unsigned int c = 0;
> > > > TPMPassthroughOptions *tpo;
> > > > + TPMEmulatorOptions *teo;
> > > >
> > > > info_list = qmp_query_tpm(&err);
> > > > if (err) {
> > > > @@ -1024,6 +1025,10 @@ void hmp_info_tpm(Monitor *mon, const
> > > > QDict
> > > > *qdict)
> > > > tpo->has_cancel_path ? ",cancel-
> > > > path="
> > > > : "",
> > > > tpo->has_cancel_path ? tpo-
> > > > >cancel_path
> > > > : "");
> > > > break;
> > > > + case TPM_TYPE_EMULATOR:
> > > > + teo = ti->options->u.emulator.data;
> > > > + monitor_printf(mon, ",chardev=%s", teo->chardev);
> > > > + break;
> > > > case TPM_TYPE__MAX:
> > > > break;
> > > > }
> > > > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> > > > index 64cecc3..41f0b7a 100644
> > > > --- a/hw/tpm/Makefile.objs
> > > > +++ b/hw/tpm/Makefile.objs
> > > > @@ -1,2 +1,3 @@
> > > > common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> > > > common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> > > > tpm_util.o
> > > > +common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o tpm_util.o
> > > > diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> > > > new file mode 100644
> > > > index 0000000..c02bbe2
> > > > --- /dev/null
> > > > +++ b/hw/tpm/tpm_emulator.c
> > > > @@ -0,0 +1,649 @@
> > > > +/*
> > > > + * emulator TPM driver
> > > > + *
> > > > + * Copyright (c) 2017 Intel Corporation
> > > > + * Author: Amarnath Valluri <address@hidden>
> > > > + *
> > > > + * Copyright (c) 2010 - 2013 IBM Corporation
> > > > + * Authors:
> > > > + * Stefan Berger <address@hidden>
> > > > + *
> > > > + * Copyright (C) 2011 IAIK, Graz University of Technology
> > > > + * Author: Andreas Niederl
> > > > + *
> > > > + * 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.gn
> > > > u.or
> > > > g/licenses/>
> > > > + *
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "qemu/sockets.h"
> > > > +#include "io/channel-socket.h"
> > > > +#include "sysemu/tpm_backend.h"
> > > > +#include "tpm_int.h"
> > > > +#include "hw/hw.h"
> > > > +#include "hw/i386/pc.h"
> > > > +#include "tpm_util.h"
> > > > +#include "tpm_ioctl.h"
> > > > +#include "migration/blocker.h"
> > > > +#include "qapi/error.h"
> > > > +#include "chardev/char-fe.h"
> > > > +
> > > > +#include <fcntl.h>
> > > > +#include <sys/types.h>
> > > > +#include <sys/stat.h>
> > > > +#include <stdio.h>
> > > > +
> > > > +#define DEBUG_TPM 0
> > > > +
> > > > +#define DPRINT(fmt, ...) do { \
> > > > + if (DEBUG_TPM) { \
> > > > + fprintf(stderr, fmt, ## __VA_ARGS__); \
> > > > + } \
> > > > +} while (0);
> > > > +
> > > > +#define DPRINTF(fmt, ...) DPRINT("tpm-emulator: "fmt"\n",
> > > > __VA_ARGS__)
> > > I would define a single DPRINTF() (& drop DPRINT usage below)
> > Ok, will do
> > >
> > >
> > >
> > > >
> > > >
> > > > +
> > > > +#define TYPE_TPM_EMULATOR "tpm-emulator"
> > > > +#define TPM_EMULATOR(obj) \
> > > > + OBJECT_CHECK(TPMEmulator, (obj), TYPE_TPM_EMULATOR)
> > > > +
> > > > +#define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps &
> > > > (cap)) == (cap))
> > > > +
> > > > +static const TPMDriverOps tpm_emulator_driver;
> > > > +
> > > > +/* data structures */
> > > > +typedef struct TPMEmulator {
> > > > + TPMBackend parent;
> > > > +
> > > > + TPMEmulatorOptions *options;
> > > Contrary to my comment on previous patch, I realize it is
> > > convenient
> > > to have a qapi pointer here, so you can use the free visitor
> > > later.
> > >
> > > >
> > > >
> > > > + CharBackend ctrl_dev;
> > > ctrl_chr perhaps? or just chr or be (the most common name in
> > > other
> > > devices).
> > >
> > > >
> > > >
> > > > + QIOChannel *data_ioc;
> > > > + bool op_executing;
> > > > + bool op_canceled;
> > > I think those 2 fields can be dropped, see below.
> > >
> > > >
> > > >
> > > > + bool had_startup_error;
> > > I think some little refactoring could remove the whole
> > > had_startup_error() backend & callback before or after the
> > > series.
> > > tpm_backend_startup_tpm() already returns an error code. device
> > > or
> > > common backend code could handle & remember that.
> > Yes, i agree, we can avoid had_startup_error() from DriverOps, i
> > will
> > do this.
> > >
> > >
> > > >
> > > >
> > > > + TPMVersion tpm_version;
> > > This is probably worth to consider in common code instead, but
> > > let's
> > > not worry about it.
> > >
> > > >
> > > >
> > > > + ptm_cap caps; /* capabilities of the TPM */
> > > > + uint8_t cur_locty_number; /* last set locality */
> > > > + QemuMutex state_lock;
> > > > + Error *migration_blocker;
> > > > +} TPMEmulator;
> > > > +
> > > > +
> > > > +static int tpm_emulator_ctrlcmd(CharBackend *dev, unsigned
> > > > long
> > > > cmd, void *msg,
> > > > + size_t msg_len_in, size_t
> > > > msg_len_out)
> > > > +{
> > > > + uint32_t cmd_no = cpu_to_be32(cmd);
> > > > + ssize_t n = sizeof(uint32_t) + msg_len_in;
> > > > + uint8_t *buf = NULL;
> > > > +
> > > > + buf = (uint8_t *)malloc(n);
> > > could be g_alloca() instead. Alternatively, why not call 2
> > > write()
> > > instead?
> > >
> > > none of the casts in this function are necessary, please remove
> > > them.
> > Ok, will change to aclloca()
> I just realized this is SOCK_SEQPACKET, ok
>
> >
> > >
> > >
> > > >
> > > >
> > > > + memcpy(buf, &cmd_no, sizeof(cmd_no));
> > > > + memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
> > > > +
> > > > + n += qemu_chr_fe_write_all(dev, (const uint8_t *)buf, n);
> > > weird
> > >
> > > >
> > > >
> > > > + free(buf);
> > > > +
> > > > + if (n > 0) {
> > > with the n += above, you'll get interesting behaviour :)
> > Ya, it was typo :)
> > >
> > >
> > > You probably want to check if any write above failed, and return
> > > early
> > > for the error case.
> > >
> > > Please improve the error handling in this function
> > >
> > > >
> > > >
> > > > + if (msg_len_out > 0) {
> > > > + n = qemu_chr_fe_read_all(dev, (uint8_t *)msg,
> > > > msg_len_out);
> > > > + /* simulate ioctl return value */
> > > > + if (n > 0) {
> > > > + n = 0;
> > > > + }
> > > > + } else {
> > > > + n = 0;
> > > > + }
> > > > + }
> > > > + return n;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_pt,
> > > tpm_pt was tpm_passthrough I suppose.
> > >
> > > Please rename tpm_pt tpm_emu (or I would suggest "self", but this
> > > isn't common name in qemu code - it's actually common in a close
> > > c-object world., and it is quite convenient...)
> > My interpretation about _pt was 'pointer' ;-) i will consider your
> > suggetion.
> > >
> > >
> > > >
> > > >
> > > > + const uint8_t *in,
> > > > uint32_t
> > > > in_len,
> > > > + uint8_t *out, uint32_t
> > > > out_len,
> > > > + bool *selftest_done)
> > > > +{
> > > > + ssize_t ret;
> > > > + bool is_selftest = false;
> > > > + const struct tpm_resp_hdr *hdr = NULL;
> > > > + Error *err = NULL;
> > > > +
> > > > + tpm_pt->op_canceled = false;
> > > > + tpm_pt->op_executing = true;
> > > > + if (selftest_done) {
> > > > + *selftest_done = false;
> > > > + is_selftest = tpm_util_is_selftest(in, in_len);
> > > > + }
> > > > +
> > > > + ret = qio_channel_write(tpm_pt->data_ioc, (char *)in,
> > > > in_len,
> > > > &err);
> > > hmm, too bad qio_channel_write() doesn't take a void *
> > >
> > > why not write_all()?
> > Agreed
> > >
> > >
> > > >
> > > >
> > > > + if (ret != in_len || err) {
> > > > + if (!tpm_pt->op_canceled || errno != ECANCELED) {
> > > I don't think ECANCELED make sense for emulator code.
> > >
> > > >
> > > >
> > > > + error_report("tpm-emulator: error while
> > > > transmitting
> > > > data "
> > > > + "to TPM: %s", err ?
> > > > error_get_pretty(err)
> > > > : "");
> > > > + error_free(err);
> > > > + }
> > > > + goto err_exit;
> > > > + }
> > > > +
> > > > + tpm_pt->op_executing = false;
> > > > +
> > > > + ret = qio_channel_read(tpm_pt->data_ioc, (char *)out,
> > > > out_len,
> > > > &err);
> > > > + if (ret < 0 || err) {
> > > read_all() ?
> > The issue with read_all() is it does not return the no of bytes it
> > read, so i would like to stict to _read()
> ok
>
> >
> > >
> > >
> > > >
> > > >
> > > > + if (!tpm_pt->op_canceled || errno != ECANCELED) {
> > > > + error_report("tpm-emulator: error while reading
> > > > data
> > > > from "
> > > > + "TPM: %s", err ?
> > > > error_get_pretty(err) :
> > > > "");
> > > > + error_free(err);
> > > > + }
> > > > + } else if (ret >= sizeof(*hdr)) {
> > > > + hdr = (struct tpm_resp_hdr *)out;
> > > > + }
> > > > +
> > > > + if (!hdr || be32_to_cpu(hdr->len) != ret) {
> > > > + error_report("tpm-emulator: received invalid response
> > > > "
> > > > + "packet from TPM with length :%ld", ret);
> > > > + ret = -1;
> > > > + goto err_exit;
> > > > + }
> > > > +
> > > > + if (is_selftest) {
> > > > + *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +
> > > > +err_exit:
> > > > + if (ret < 0) {
> > > > + tpm_util_write_fatal_error_response(out, out_len);
> > > > + }
> > > > +
> > > > + tpm_pt->op_executing = false;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_set_locality(TPMEmulator *tpm_pt,
> > > > uint8_t
> > > > locty_number)
> > > > +{
> > > > + ptm_loc loc;
> > > > +
> > > > + DPRINTF("%s : locality: 0x%x", __func__, locty_number);
> > > > +
> > > > + if (tpm_pt->cur_locty_number != locty_number) {
> > > return early instead if ==, to avoid code indent etc
> > Ok
> > >
> > >
> > > >
> > > >
> > > > + DPRINTF("setting locality : 0x%x", locty_number);
> > > > + loc.u.req.loc = locty_number;
> > > This number isn't set in be like the rest of the protocol?
> > I doubt if i get ur point :(, can you please elaborate.
> In the rest of the protocol, it uses big-endian. I suppose you should
> cpu_to_be32(locty_number).
>
> >
> > >
> > >
> > > >
> > > >
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_SET_LOCALITY, &loc,
> > > > + sizeof(loc), sizeof(loc)) < 0) {
> > > > + error_report("tpm-emulator: could not set locality
> > > > :
> > > > %s",
> > > > + strerror(errno));
> > > > + return -1;
> > > > + }
> > > > + loc.u.resp.tpm_result =
> > > > be32_to_cpu(loc.u.resp.tpm_result);
> > > > + if (loc.u.resp.tpm_result != 0) {
> > > > + error_report("tpm-emulator: TPM result for set
> > > > locality : 0x%x",
> > > > + loc.u.resp.tpm_result);
> > > > + return -1;
> > > > + }
> > > > + tpm_pt->cur_locty_number = locty_number;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void tpm_emulator_handle_request(TPMBackend *tb,
> > > > TPMBackendCmd cmd)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > + TPMLocality *locty = NULL;
> > > > + bool selftest_done = false;
> > > > +
> > > > + DPRINTF("processing command type %d", cmd);
> > > > +
> > > > + switch (cmd) {
> > > > + case TPM_BACKEND_CMD_PROCESS_CMD:
> > > > + qemu_mutex_lock(&tpm_pt->state_lock);
> > > > + locty = tb->tpm_state->locty_data;
> > > > + if (tpm_emulator_set_locality(tpm_pt,
> > > > + tb->tpm_state-
> > > > >locty_number)
> > > > < 0) {
> > > > + tpm_util_write_fatal_error_response(locty-
> > > > >
> > > > > r_buffer.buffer,
> > > > + locty-
> > > > >r_buffer.size);
> > > return / goto here instead of else.
> > We should not retrun from here, we have to propagte the error
> > response
> > to device, and unlock the state.
> > >
> > >
> > > >
> > > >
> > > > + } else {
> > > > + tpm_emulator_unix_tx_bufs(tpm_pt, locty-
> > > > >
> > > > > w_buffer.buffer,
> > > > + locty->w_offset,
> > > > + locty-
> > > > >
> > > > > r_buffer.buffer,
> > > > + locty-
> > > > >
> > > > > r_buffer.size,
> > > > + &selftest_done);
> > > no error handling?
> > The error case is supposed to handle by device, where we are
> > filling
> > into out buffer, using tpm_util_write_fatal_error_response().
> > >
> > >
> > > >
> > > >
> > > > + }
> > > > +
> > > > + tb->recv_data_callback(tb->tpm_state, tb->tpm_state-
> > > > >
> > > > > locty_number,
> > > > + selftest_done);
> > > > + qemu_mutex_unlock(&tpm_pt->state_lock);
> > > > +
> > > > + break;
> > > > + case TPM_BACKEND_CMD_INIT:
> > > > + case TPM_BACKEND_CMD_END:
> > > > + case TPM_BACKEND_CMD_TPM_RESET:
> > > > + /* nothing to do */
> > > > + break;
> > > > + }
> > > > +}
> > > > +
> > > > +/*
> > > > + * Gracefully shut down the external unixio TPM
> > > > + */
> > > > +static void tpm_emulator_shutdown(TPMEmulator *tpm_pt)
> > > > +{
> > > > + ptm_res res;
> > > > +
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev, CMD_SHUTDOWN,
> > > > &res, 0,
> > > > + sizeof(res)) < 0) {
> > > > + error_report("tpm-emulator: Could not cleanly shutdown
> > > > the
> > > > TPM: %s",
> > > > + strerror(errno));
> > > > + } else if (res != 0) {
> > > > + error_report("tpm-emulator: TPM result for sutdown:
> > > > 0x%x",
> > > > + be32_to_cpu(res));
> > > > + }
> > > > +}
> > > > +
> > > > +static int tpm_emulator_probe_caps(TPMEmulator *tpm_pt)
> > > > +{
> > > > + DPRINTF("%s", __func__);
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_GET_CAPABILITY,
> > > > + &tpm_pt->caps, 0, sizeof(tpm_pt-
> > > > >caps)) <
> > > > 0) {
> > > > + error_report("tpm-emulator: probing failed : %s",
> > > > strerror(errno));
> > > > + return -1;
> > > > + }
> > > > +
> > > > + tpm_pt->caps = be64_to_cpu(tpm_pt->caps);
> > > > +
> > > > + DPRINTF("capbilities : 0x%lx", tpm_pt->caps);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_check_caps(TPMEmulator *tpm_pt)
> > > > +{
> > > > + ptm_cap caps = 0;
> > > > + const char *tpm = NULL;
> > > > +
> > > > + /* check for min. required capabilities */
> > > > + switch (tpm_pt->tpm_version) {
> > > > + case TPM_VERSION_1_2:
> > > > + caps = PTM_CAP_INIT | PTM_CAP_SHUTDOWN |
> > > > PTM_CAP_GET_TPMESTABLISHED |
> > > > + PTM_CAP_SET_LOCALITY;
> > > > + tpm = "1.2";
> > > > + break;
> > > > + case TPM_VERSION_2_0:
> > > > + caps = PTM_CAP_INIT | PTM_CAP_SHUTDOWN |
> > > > PTM_CAP_GET_TPMESTABLISHED |
> > > > + PTM_CAP_SET_LOCALITY |
> > > > PTM_CAP_RESET_TPMESTABLISHED;
> > > > + tpm = "2";
> > > > + break;
> > > > + case TPM_VERSION_UNSPEC:
> > > > + error_report("tpm-emulator: TPM version has not been
> > > > set");
> > > > + return -1;
> > > > + }
> > > > +
> > > > + if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_pt, caps)) {
> > > > + error_report("tpm-emulator: TPM does not implement
> > > > minimum
> > > > set of "
> > > > + "required capabilities for TPM %s
> > > > (0x%x)",
> > > > tpm, (int)caps);
> > > > + return -1;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_startup_tpm(TPMBackend *tb)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > + ptm_init init;
> > > > + ptm_res res;
> > > > +
> > > > + DPRINTF("%s", __func__);
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev, CMD_INIT,
> > > > &init,
> > > > sizeof(init),
> > > > + sizeof(init)) < 0) {
> > > > + error_report("tpm-emulator: could not send INIT: %s",
> > > > + strerror(errno));
> > > > + goto err_exit;
> > > > + }
> > > > +
> > > > + res = be32_to_cpu(init.u.resp.tpm_result);
> > > > + if (res) {
> > > > + error_report("tpm-emulator: TPM result for CMD_INIT:
> > > > 0x%x", res);
> > > > + goto err_exit;
> > > > + }
> > > > + return 0;
> > > > +
> > > > +err_exit:
> > > > + tpm_pt->had_startup_error = true;
> > > > + return -1;
> > > > +}
> > > > +
> > > > +static bool tpm_emulator_get_tpm_established_flag(TPMBackend
> > > > *tb)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > + ptm_est est;
> > > > +
> > > > + DPRINTF("%s", __func__);
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_GET_TPMESTABLISHED, &est, 0,
> > > > + sizeof(est)) < 0) {
> > > > + error_report("tpm-emulator: Could not get the TPM
> > > > established flag: %s",
> > > > + strerror(errno));
> > > > + return false;
> > > > + }
> > > > + DPRINTF("established flag: %0x", est.u.resp.bit);
> > > > +
> > > > + return (est.u.resp.bit != 0);
> > > > +}
> > > > +
> > > > +static int tpm_emulator_reset_tpm_established_flag(TPMBackend
> > > > *tb,
> > > > + uint8_t
> > > > locty)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > + ptm_reset_est reset_est;
> > > > + ptm_res res;
> > > > +
> > > > + /* only a TPM 2.0 will support this */
> > > > + if (tpm_pt->tpm_version == TPM_VERSION_2_0) {
> > > > + reset_est.u.req.loc = tpm_pt->cur_locty_number;
> > > > +
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_RESET_TPMESTABLISHED,
> > > > + &reset_est,
> > > > sizeof(reset_est),
> > > > + sizeof(reset_est)) < 0) {
> > > > + error_report("tpm-emulator: Could not reset the
> > > > establishment bit: "
> > > > + "%s", strerror(errno));
> > > > + return -1;
> > > > + }
> > > > +
> > > > + res = be32_to_cpu(reset_est.u.resp.tpm_result);
> > > > + if (res) {
> > > > + error_report("tpm-emulator: TPM result for rest
> > > > establixhed flag: "
> > > > + "0x%x", res);
> > > > + return -1;
> > > > + }
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static bool tpm_emulator_had_startup_error(TPMBackend *tb)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +
> > > > + return tpm_pt->had_startup_error;
> > > > +}
> > > > +
> > > > +static void tpm_emulator_cancel_cmd(TPMBackend *tb)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > + ptm_res res;
> > > > +
> > > > + /*
> > > > + * As of Linux 3.7 the tpm_tis driver does not properly
> > > > cancel
> > > > + * commands on all TPM manufacturers' TPMs.
> > > > + * Only cancel if we're busy so we don't cancel someone
> > > > else's
> > > > + * command, e.g., a command executed on the host.
> > > > + */
> > > > + if (tpm_pt->op_executing) {
> > > The field is set in the worker thread. This is racy. Fortunately
> > > this
> > > is not relevant for emulator, I think you can simply drop that
> > > check
> > > and the variable. Stefan should confirm though.
> > >
> > > >
> > > >
> > > > + if (TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_pt,
> > > > PTM_CAP_CANCEL_TPM_CMD)) {
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_CANCEL_TPM_CMD,
> > > > + &res, 0, sizeof(res)) <
> > > > 0) {
> > > > + error_report("tpm-emulator: Could not cancel
> > > > command: %s",
> > > > + strerror(errno));
> > > > + } else if (res != 0) {
> > > > + error_report("tpm-emulator: Failed to cancel
> > > > TPM:
> > > > 0x%x",
> > > > + be32_to_cpu(res));
> > > > + } else {
> > > > + tpm_pt->op_canceled = true;
> > > > + }
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +static void tpm_emulator_reset(TPMBackend *tb)
> > > > +{
> > > > + DPRINTF("%s", __func__);
> > > > +
> > > > + tpm_emulator_cancel_cmd(tb);
> > > > +}
> > > > +
> > > > +static TPMVersion tpm_emulator_get_tpm_version(TPMBackend *tb)
> > > > +{
> > > > + TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +
> > > > + return tpm_pt->tpm_version;
> > > > +}
> > > > +
> > > > +static void tpm_emulator_block_migration(TPMEmulator *tpm_pt)
> > > > +{
> > > > + Error *err = NULL;
> > > > +
> > > > + error_setg(&tpm_pt->migration_blocker,
> > > > + "Migration disabled: TPM emulator not yet
> > > > migratable");
> > > > + migrate_add_blocker(tpm_pt->migration_blocker, &err);
> > > > + if (err) {
> > > > + error_free(err);
> > > probably better to report_err it instead
> > Ok
> > >
> > >
> > > >
> > > >
> > > > + error_free(tpm_pt->migration_blocker);
> > > > + tpm_pt->migration_blocker = NULL;
> > > and return an error code.
> > Will do
> > >
> > >
> > > >
> > > >
> > > > + }
> > > > +}
> > > > +
> > > > +static int tpm_emulator_prepare_data_fd(TPMEmulator *tpm_pt)
> > > > +{
> > > > + ptm_res res;
> > > > + Error *err = NULL;
> > > > + int fds[2] = { -1, -1 };
> > > > +
> > > > + if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds) < 0) {
> > > > + error_report("tpm-emulator: Failed to create
> > > > socketpair");
> > > > + return -1;
> > > > + }
> > > > +
> > > > + qemu_chr_fe_set_msgfds(&tpm_pt->ctrl_dev, fds + 1, 1);
> > > > +
> > > > + if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_SET_DATAFD,
> > > > &res, 0,
> > > > + sizeof(res)) || res != 0) {
> > > Yay! for making life easier and hiding a protocol detail.
> > >
> > > >
> > > >
> > > > + error_report("tpm-emulator: Failed to send
> > > > CMD_SET_DATAFD:
> > > > %s",
> > > > + strerror(errno));
> > > > + goto err_exit;
> > > > + }
> > > > +
> > > > + tpm_pt->data_ioc =
> > > > QIO_CHANNEL(qio_channel_socket_new_fd(fds[0], &err));
> > > > + if (err) {
> > > > + error_report("tpm-emulator: Failed to create io
> > > > channel :
> > > > %s",
> > > > + error_get_pretty(err));
> > > error_prepend()?
> > Ok
> > >
> > >
> > > >
> > > >
> > > > + error_free(err);
> > > > + goto err_exit;
> > > > + }
> > > close fds[1] ?
> > I guess we are not supposed to close the other end of the
> > socketpair/pipe, as it is not forked process.
> You will close this end, not the other end.
fds[1] is handed over to other end, fds[0] is used by qemu, so we
cannot close unless error case i guess.
- Amarnath
- Re: [Qemu-devel] [PATCH v7 5/8] tmp backend: Add new api to read backend TpmInfo, (continued)
- [Qemu-devel] [PATCH v7 6/8] tpm-backend: Move realloc_buffer() implementation to tpm-tis model, Amarnath Valluri, 2017/09/22
- [Qemu-devel] [PATCH v7 7/8] tpm-passthrough: move reusable code to utils, Amarnath Valluri, 2017/09/22
- [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Amarnath Valluri, 2017/09/22
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Stefan Berger, 2017/09/22
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Marc-André Lureau, 2017/09/24
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Stefan Berger, 2017/09/24
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Valluri, Amarnath, 2017/09/26
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Marc-André Lureau, 2017/09/26
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator,
Valluri, Amarnath <=
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Marc-André Lureau, 2017/09/26
- Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator, Eric Blake, 2017/09/26
- [Qemu-devel] [PATCH v8 8/8] tpm: Added support for TPM emulator, Amarnath Valluri, 2017/09/27
- Re: [Qemu-devel] [PATCH v8 8/8] tpm: Added support for TPM emulator, Marc-André Lureau, 2017/09/27