Re: [PATCH] tpm: add backend for mssim

[James Bottomley]

On Mon, 2022-12-12 at 15:47 +0000, Daniel P. Berrangé wrote:
> 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.

Pretty much every TPM application fears interposers and should thus be
using the TPM transport security anyway. *If* this is the case, then
the transport is secure.  Note that this currently isn't the case for
the kernel use of the TPM, but I'm trying to fix that.  The standard
mssim server is too simplistic to do transport layer security, but like
everything that does this (or rather doesn't do this), you can front it
with stunnel4.

> > 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.

It can either be me or IBM, we're joint owners, that's why I thought
just author.

> > + * 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.

Heh, well I just copied this from the ssh block backend ...

I can easily find something more modern to update it.

[...]
> >  
> > +##
> > +# @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

Unfortunately chardev isn't a socket, it really is a character device
that fronts to the vtpm kernel proxy, so even if I convert mssim usage
to socket, you'll still have to keep chardev for the emulator backend.

> 
> 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)


OK, I'll look into doing this.

Regards,

James