Re: [PATCH v7 2/2] tpm: add backend for mssim

[James Bottomley <jejb@linux.ibm.com>]

On Thu, 2023-09-28 at 07:29 +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Sep 27, 2023 at 12:49:08PM -0400, James Bottomley wrote:
> > > From: James Bottomley <James.Bottomley@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 <jejb@linux.ibm.com>
> > > Acked-by: Markus Armbruster <armbru@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 <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/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