Re: [Qemu-devel] [PATCH v3 1/6] Provide support for the CUSE TPM

[Eric Blake]

On 05/26/2015 03:33 PM, Stefan Berger wrote:
> Rather than integrating TPM functionality into QEMU directly
> using the TPM emulation of libtpms, we now integrate an external
> emulated TPM device. This device is expected to implement a Linux
> CUSE interface (CUSE = character device in userspace).
> 
> QEMU talks to the CUSE TPM using much functionality of the
> passthrough driver. For example, the TPM commands and responses
> are sent to the CUSE TPM using the read()/write() interface.
> However, some out-of-band control needs to be done using the CUSE
> TPM's ioctl's. The CUSE TPM currently defines and implements 14
> different ioctls for controlling certain life-cycle aspects of
> the emulated TPM. The ioctls can be regarded as a replacement for
> direct function calls to a TPM emulator if the TPM were to be
> directly integrated into QEMU.
> 
> One  of the ioctl's allows to get a bitmask of supported capabilities.
> Each returned bit indicates which capabilties have been implemented.

s/capabilties/capabilities/

> An include file defining the various ioctls is added to QEMU.
> 
> The CUSE TPM and associated tools can be found here:
> 
> https://github.com/stefanberger/swtpm
> 
> 
> To use the external CUSE TPM, the CUSE TPM should be started as follows:
> 
> /usr/bin/swtpm_cuse -n vtpm-test
> 
> QEMU can then be started using the following parameters:
> 
> qemu-system-x86_64 \
>       [...] \
>         -tpmdev cuse-tpm,id=tpm0,cancel-path=/dev/null,path=/dev/vtpm-test \
>         -device tpm-tis,id=tpm0,tpmdev=tpm0 \
>       [...]
> 
> 
> Signed-off-by: Stefan Berger <address@hidden>
> Cc: Eric Blake <address@hidden>
> ---

At this point, I'm only doing a high-level overview (public interface,
blatant findings) and not a fine-grained reading of the implementation.

> diff --git a/hw/tpm/tpm_ioctl.h b/hw/tpm/tpm_ioctl.h
> new file mode 100644
> index 0000000..d36e702
> --- /dev/null
> +++ b/hw/tpm/tpm_ioctl.h
> @@ -0,0 +1,178 @@
> +/*
> + * tpm_ioctl.h
> + *
> + * This file is licensed under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either version 2.1 
> of
> + * the License, or (at your option) any later version.
> + */

My understanding of copyleft (and insert the obligatory IANAL
disclaimer) is that it works by exploiting the copyright law - that is,
something cannot be [L]GPL unless there is also an assertion of
copyright ownership (I don't care who, merely that a copyright claim
exists somewhere in the file, nearby the license).


> +/*
> + * Data structure to get state blobs from the TPM. If the size of the
> + * blob exceeds the STATE_BLOB_SIZE, multiple reads with
> + * adjusted offset are necessary. The last packet is indicated by
> + * the length being smaller than the STATE_BLOB_SIZE.

If the read size is exactly STATE_BLOB_SIZE, does that result in 1
packet or 2?  Does it cause a 0-length packet to be attempted, or is it
broken into STATE_BLOB_SIZE-1 and 1?


> +
> +/* state_flags above : */
> +#define STATE_FLAG_DECRYPTED     1 /* on input:  get decrypted state */
> +#define STATE_FLAG_ENCRYPTED     2 /* on output: state is encrytped */

s/encrytped/encrypted/

> +
> +/*
> + * Data structure to set state blobs in the TPM. If the size of the
> + * blob exceeds the STATE_BLOB_SIZE, multiple 'writes' are necessary.
> + * The last packet is indicated by the length being smaller than the
> + * STATE_BLOB_SIZE.
> + */

Same question as on read about an exact STATE_BLOB_SIZE write

> +struct ptm_setstate {
> +    union {
> +        struct {
> +            uint32_t state_flags; /* may be STATE_FLAG_ENCRYPTED */
> +            uint32_t tpm_number;  /* always set to 0 */
> +            uint8_t type;         /* which blob to set */
> +            uint32_t length;
> +            uint8_t data[STATE_BLOB_SIZE];

This struct has padding blanks; is that going to matter?


> +typedef uint64_t ptmcap_t;
> +typedef struct ptmest  ptmest_t;
> +typedef struct ptmreset_est ptmreset_est_t;
> +typedef struct ptmloc  ptmloc_t;
> +typedef struct ptmhdata ptmhdata_t;

Why a change in 1 vs. 2 spaces on some of the types?

Technically, POSIX reserves the entire *_t namespace to itself, I'm a
bit worried that by doing 'typedef struct foo foo_t' we are not being
consistent with the rest of qemu, which does 'typedef struct foo foo'.


> +++ b/hw/tpm/tpm_passthrough.c

> @@ -72,12 +74,18 @@ struct TPMPassthruState {
>      bool had_startup_error;
>  
>      TPMVersion tpm_version;
> +    ptmcap_t cuse_cap; /* capabilties of the CUSE TPM */
> +    uint8_t cur_locty_number; /* last set locality */

s/capabilties/capabilities/

>  };
>  
>  typedef struct TPMPassthruState TPMPassthruState;
>  
>  #define TPM_PASSTHROUGH_DEFAULT_DEVICE "/dev/tpm0"
>  
> +#define TPM_PASSTHROUGH_USES_CUSE_TPM(tpm_pt) (tpm_pt->cuse_cap != 0)
> +
> +#define TPM_CUSE_IMPLEMENTS(tpm_tr, cap) ((tpm_pt->cuse_cap & cap) == cap)

Evaluates cap more than once, which may not be ideal.  Also
under-parenthesized in the face of arbitrary expressions for tpm_tr or cap.

Umm, how does the macro argument tpm_tr get used, and where is the macro
body tpm_pt scoped?

Better might be this (depending on your intent):
#define TPM_CUSE_IMPLEMENTS(tpm_tr, cap) \
   (((tpm_tr)->cuse_cap & (cap)) != 0)

if you know that cap will always be passed as one bit.  But if someone
intends to use the macro to test multiple bits at once, and return true
only if all of the bits are set, then living with multiple evaluation of
'cap' may be better.

> +static int tpm_passthrough_set_locality(TPMPassthruState *tpm_pt,
> +                                        uint8_t locty_number)
> +{
> +    int n;
> +    ptmloc_t loc;
> +
> +    if (TPM_PASSTHROUGH_USES_CUSE_TPM(tpm_pt)) {
> +        if (tpm_pt->cur_locty_number != locty_number) {
> +            loc.u.req.loc = locty_number;
> +            n = ioctl(tpm_pt->tpm_fd, PTM_SET_LOCALITY, &loc);
> +            if (n < 0) {
> +                error_report("tpm_cuse: could not set locality on "
> +                             "CUSE TPM: %s (%i)",
> +                             strerror(errno), errno);

Hmm, I wonder if error_setg_errno() followed by error_report_err() is
any nicer than manually calling strerror().  Probably not worth worrying
about.

On the other hand, this code is not strictly portable - passing both
errno and strerror(errno) as arguments to a function has no sequencing
point defined on whether errno is collected first or second; if it is
collected second, strerror() may have clobbered errno.  Most code
doesn't bother with printing "%s (%i)" for errors; the %s alone is
sufficient.


>  /*
> + * Gracefully shut down the external CUSE TPM
> + */
> +static void tpm_passthrough_shutdown(TPMPassthruState *tpm_pt)
> +{
> +    int n;
> +    ptmres_t res;
> +
> +    if (TPM_PASSTHROUGH_USES_CUSE_TPM(tpm_pt)) {
> +        n = ioctl(tpm_pt->tpm_fd, PTM_SHUTDOWN, &res);
> +        if (n < 0) {
> +            error_report("tpm_cuse: Could not cleanly shut down "
> +                         "the CUSE TPM: %s (%i)",
> +                         strerror(errno), errno);

Why not just 'if (ioctl(...) < 0) {' without needing 'n'?

> +        }
> +    }
> +}
> +
> +/*
> + * Probe for the CUSE TPM by sending an ioctl() requesting its
> + * capability flags.
> + */
> +static int tpm_passthrough_cuse_probe(TPMPassthruState *tpm_pt)
> +{
> +    int rc = 0;
> +    int n;
> +
> +    n = ioctl(tpm_pt->tpm_fd, PTM_GET_CAPABILITY, &tpm_pt->cuse_cap);
> +    if (n < 0) {
> +        error_report("Error: CUSE TPM was requested, but probing failed.");

Most qemu error messages intentionally do not end in period

> @@ -306,6 +472,8 @@ static void tpm_passthrough_cancel_cmd(TPMBackend *tb)
>  {
>      TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
>      int n;
> +    ptmres_t res;
> +    static int error_printed;

You're using this as a bool...


> +                } else if (res != TPM_SUCCESS) {
> +                    if (!error_printed) {
> +                        error_report("TPM error code from command "
> +                                     "cancellation of CUSE TPM: 0x%x", res);
> +                        error_printed = true;
> +                    }

...so declare it as one.


> +++ b/qapi-schema.json
> @@ -2974,10 +2974,11 @@
>  # An enumeration of TPM types
>  #
>  # @passthrough: TPM passthrough type
> +# @cuse-tpm: CUSE TPM type

Missing '(since 2.4)' designator.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature