Re: [PATCH] tpm: add backend for mssim

["Daniel P. Berrangé" <berrange@redhat.com>]

Copy'ing Markus for QAPI design feedback.

On Sat, Dec 10, 2022 at 12:10:18PM -0500, James Bottomley wrote:
> The Microsoft Simulator (mssim) is the reference emulation platform
> for the TCG TPM 2.0 specification.
> 
> https://github.com/Microsoft/ms-tpm-20-ref.git
> 
> It exports a fairly simple network socket baset protocol on two
> sockets, one for command (default 2321) and one for control (default
> 2322).  This patch adds a simple backend that can speak the mssim
> protocol over the network.  It also allows the host, and two ports to
> be specified on the qemu command line.  The benefits are twofold:
> firstly it gives us a backend that actually speaks a standard TPM
> emulation protocol instead of the linux specific TPM driver format of
> the current emulated TPM backend and secondly, using the microsoft
> protocol, the end point of the emulator can be anywhere on the
> network, facilitating the cloud use case where a central TPM service
> can be used over a control network.

What's the story with security for this ?  The patch isn't using
TLS, so talking to any emulator over anything other than localhost
looks insecure, unless I'm missing something.



> diff --git a/backends/tpm/tpm_mssim.c b/backends/tpm/tpm_mssim.c
> new file mode 100644
> index 0000000000..6864b1fbc0
> --- /dev/null
> +++ b/backends/tpm/tpm_mssim.c
> @@ -0,0 +1,266 @@
> +/*
> + * Emulator TPM driver which connects over the mssim protocol
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) 2022

Copyright by whom ?  Presumably this line should have "IBM" present
if we're going to have it at all.

> + * Author: James Bottomley <jejb@linux.ibm.com>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "qemu/sockets.h"
> +
> +#include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-tpm.h"
> +
> +#include "io/channel-socket.h"
> +
> +#include "sysemu/tpm_backend.h"
> +#include "sysemu/tpm_util.h"
> +
> +#include "qom/object.h"
> +
> +#include "tpm_int.h"
> +#include "tpm_mssim.h"
> +

> +static TPMBackend *tpm_mssim_create(QemuOpts *opts)
> +{
> +    TPMBackend *be = TPM_BACKEND(object_new(TYPE_TPM_MSSIM));
> +    TPMmssim *t = TPM_MSSIM(be);
> +    InetSocketAddress cmd_s, ctl_s;
> +    int sock;
> +    const char *host, *port, *ctrl;
> +    Error *errp = NULL;
> +
> +    host = qemu_opt_get(opts, "host");
> +    if (!host)
> +        host = "localhost";
> +    t->opts.host = g_strdup(host);
> +
> +    port = qemu_opt_get(opts, "port");
> +    if (!port)
> +        port = "2321";
> +    t->opts.port = g_strdup(port);
> +
> +    ctrl = qemu_opt_get(opts, "ctrl");
> +    if (!ctrl)
> +        ctrl = "2322";
> +    t->opts.ctrl = g_strdup(ctrl);
> +
> +    cmd_s.host = (char *)host;
> +    cmd_s.port = (char *)port;
> +
> +    ctl_s.host = (char *)host;
> +    ctl_s.port = (char *)ctrl;
> +
> +    sock = inet_connect_saddr(&cmd_s, &errp);
> +    if (sock < 0)
> +        goto fail;
> +    t->cmd_qc = QIO_CHANNEL(qio_channel_socket_new_fd(sock, &errp));
> +    if (errp)
> +        goto fail;
> +    sock = inet_connect_saddr(&ctl_s, &errp);
> +    if (sock < 0)
> +        goto fail_unref_cmd;
> +    t->ctrl_qc = QIO_CHANNEL(qio_channel_socket_new_fd(sock, &errp));
> +    if (errp)
> +        goto fail_unref_cmd;

We don't want to be using inet_connect_saddr, that's a legacy
API. All new code should be using the qio_channel_socket_connect*
family of APIs. This is trivial if the QAPI design uses SocketAddress
structs directly.

> +
> +    /* reset the TPM using a power cycle sequence, in case someone
> +     * has previously powered it up */
> +    sock = tpm_send_ctrl(t, TPM_SIGNAL_POWER_OFF, &errp);
> +    if (sock != 0)
> +        goto fail_unref;
> +    sock = tpm_send_ctrl(t, TPM_SIGNAL_POWER_ON, &errp);
> +    if (sock != 0)
> +        goto fail_unref;
> +    sock = tpm_send_ctrl(t, TPM_SIGNAL_NV_ON, &errp);
> +    if (sock != 0)
> +        goto fail_unref;
> +
> +    return be;
> + fail_unref:
> +    object_unref(OBJECT(t->ctrl_qc));
> + fail_unref_cmd:
> +    object_unref(OBJECT(t->cmd_qc));
> + fail:
> +    error_prepend(&errp, ERROR_PREFIX);
> +    error_report_err(errp);
> +    object_unref(OBJECT(be));
> +
> +    return NULL;
> +}
> +
> +static const QemuOptDesc tpm_mssim_cmdline_opts[] = {
> +    TPM_STANDARD_CMDLINE_OPTS,
> +    {
> +        .name = "host",
> +        .type = QEMU_OPT_STRING,
> +        .help = "name or IP address of host to connect to (deault localhost)",
> +    },
> +    {
> +        .name = "port",
> +        .type = QEMU_OPT_STRING,
> +        .help = "port number for standard TPM commands (default 2321)",
> +    },
> +    {
> +        .name = "ctrl",
> +        .type = QEMU_OPT_STRING,
> +        .help = "control port for TPM commands (default 2322)",
> +    },
> +};
> +
> +static void tpm_mssim_class_init(ObjectClass *klass, void *data)
> +{
> +    TPMBackendClass *cl = TPM_BACKEND_CLASS(klass);
> +
> +    cl->type = TPM_TYPE_MSSIM;
> +    cl->opts = tpm_mssim_cmdline_opts;
> +    cl->desc = "TPM mssim emulator backend driver";
> +    cl->create = tpm_mssim_create;
> +    cl->cancel_cmd = tpm_mssim_cancel_cmd;
> +    cl->get_tpm_version = tpm_mssim_get_version;
> +    cl->get_buffer_size = tpm_mssim_get_buffer_size;
> +    cl->get_tpm_options = tpm_mssim_get_opts;
> +    cl->handle_request = tpm_mssim_handle_request;
> +}


>  
> +##
> +# @TPMmssimOptions:
> +#
> +# Information for the mssim emulator connection
> +#
> +# @host: host name or IP address to connect to
> +# @port: port for the standard TPM commands
> +# @ctrl: control port for TPM state changes
> +#
> +# Since: 7.2.0
> +##
> +{ 'struct': 'TPMmssimOptions',
> +  'data': {
> +      'host': 'str',
> +      'port': 'str',
> +      'ctrl': 'str' },
> +  'if': 'CONFIG_TPM' }

We don't want to be adding new code using plain host/port combos,
as that misses extra functionality for controlling IPv4 vs IPv6
usage.

The existing 'emulator' backend references a chardev, but I'm
not especially in favour of using the chardev indirection either,
when all we should really need is a SocketAddress

IOW, from a QAPI design POV, IMHO the best practice would be

 { 'struct': 'TPMmssimOptions',
   'data': {
       'command': 'SocketAddress',
       'control': 'SocketAddress' },
   'if': 'CONFIG_TPM' }


The main wrinkle with this is that exprssing nested struct fields
with QemuOpts is a disaster zone, and -tpmdev doesn't yet support
JSON syntax.

IMHO we should just fix the latter problem, as I don't think it
ought to be too hard. Probably a cut+paste / search/replace job
on the chanmge we did for -device in:

  commit 5dacda5167560b3af8eadbce5814f60ba44b467e
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   Fri Oct 8 15:34:42 2021 +0200

    vl: Enable JSON syntax for -device

This would mean we could use plain -tpmdev for a local instance

   -tpmdev mssim,id=tpm0 \
    -device tpm-crb,tpmdev=tpm0 \

but to use a remote emulator we would use

    -tpmdev "{'backend': 'mssim', 'id': 'tpm0',
              'command': {
	         'type': 'inet',
		 'host': 'remote',
		 'port': '4455'
               },
              'control': {
	         'type': 'inet',
		 'host': 'remote',
		 'port': '4456'
               }}"

(without the whitepace/newlines, which i just used for sake of clarity)

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|