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 <j...@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 <kw...@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 :|
