On Thu, 2023-09-28 at 07:29 +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berra...@redhat.com> writes:
>
> > On Wed, Sep 27, 2023 at 12:49:08PM -0400, James Bottomley wrote:
> > > From: James Bottomley <james.bottom...@hansenpartnership.com>
> > >
> > > 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 based 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 two sockets to be
> > > specified on the 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.
> > >
> > > The implementation does basic control commands like power off/on,
> > > but
> > > doesn't implement cancellation or startup. The former because
> > > cancellation is pretty much useless on a fast operating TPM
> > > emulator
> > > and the latter because this emulator is designed to be used with
> > > OVMF
> > > which itself does TPM startup and I wanted to validate that.
> > >
> > > To run this, simply download an emulator based on the MS
> > > specification
> > > (package ibmswtpm2 on openSUSE) and run it, then add these two
> > > lines
> > > to the qemu command and it will use the emulator.
> > >
> > > -tpmdev mssim,id=tpm0 \
> > > -device tpm-crb,tpmdev=tpm0 \
> > >
> > > to use a remote emulator replace the first line with
> > >
> > > -tpmdev
> > > "{'type':'mssim','id':'tpm0','command':{'type':inet,'host':'remot
> > > e','port':'2321'}}"
> > >
> > > tpm-tis also works as the backend.
> > >
> > > Signed-off-by: James Bottomley <j...@linux.ibm.com>
> > > Acked-by: Markus Armbruster <arm...@redhat.com>
>
> [...]
>
> > > diff --git a/backends/tpm/tpm_mssim.c b/backends/tpm/tpm_mssim.c
> > > new file mode 100644
> > > index 0000000000..b8a12dce04
> > > --- /dev/null
> > > +++ b/backends/tpm/tpm_mssim.c
> > > @@ -0,0 +1,290 @@
> > > +/*
> > > + * Emulator TPM driver which connects over the mssim protocol
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + *
> > > + * Copyright (c) 2022
> > > + * 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/runstate.h"
> > > +#include "sysemu/tpm_backend.h"
> > > +#include "sysemu/tpm_util.h"
> > > +
> > > +#include "qom/object.h"
> > > +
> > > +#include "tpm_int.h"
> > > +#include "tpm_mssim.h"
> > > +
> > > +#define ERROR_PREFIX "TPM mssim Emulator: "
> > > +
> > > +#define TYPE_TPM_MSSIM "tpm-mssim"
> > > +OBJECT_DECLARE_SIMPLE_TYPE(TPMMssim, TPM_MSSIM)
> > > +
> > > +struct TPMMssim {
> > > + TPMBackend parent;
> > > +
> > > + TPMMssimOptions opts;
> > > +
> > > + QIOChannelSocket *cmd_qc, *ctrl_qc;
> > > +};
> > > +
> > > +static int tpm_send_ctrl(TPMMssim *t, uint32_t cmd, Error
> > > **errp)
> > > +{
> > > + int ret;
> > > +
> > > + qio_channel_socket_connect_sync(t->ctrl_qc, t->opts.control,
> > > errp);
> >
> > Need to assign to 'ret' and check for failure here, otherwise the
> > next call to write_all will overwrite the useful message in 'errp'
> > with a less helpful one.
>
> No, it'll crash :)
>
> An @errp argument must point to a null pointer. If it doesn't,
> setting
> an error will trip error_setv()'s assertion.
>
> > + cmd = htonl(cmd);
> > + ret = qio_channel_write_all(QIO_CHANNEL(t->ctrl_qc),
> > + (char *)&cmd, sizeof(cmd), errp);
> > + if (ret != 0) {
> > + goto out;
> > + }
>
> qapi/error.h's big comment advises:
>
> * Receive and accumulate multiple errors (first one wins):
> * Error *err = NULL, *local_err = NULL;
> * foo(arg, &err);
> * bar(arg, &local_err);
> * error_propagate(&err, local_err);
> * if (err) {
> * handle the error...
> * }
> *
> * Do *not* "optimize" this to
> * Error *err = NULL;
> * foo(arg, &err);
> * bar(arg, &err); // WRONG!
> * if (err) {
> * handle the error...
> * }
> * because this may pass a non-null err to bar().
> *
> * Likewise, do *not*
> * Error *err = NULL;
> * if (cond1) {
> * error_setg(&err, ...);
> * }
> * if (cond2) {
> * error_setg(&err, ...); // WRONG!
> * }
> * because this may pass a non-null err to error_setg().
>
> The quoted code is like the last example, except the error_setg()
> lurk within the functions called.
So this is what I chose:
out:
/*
* need to close the channel here, but if that fails report it
* while not letting a prior failure get overwritten
*/
retc = qio_channel_close(QIO_CHANNEL(t->ctrl_qc), &local_err);
error_propagate(errp, local_err);
return retc ? retc : ret;
Hopefully that looks OK to everyone?
James
