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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 05/26/2015 07:05 PM, Eric Blake wrote:
> 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 <stefanb@linux.vnet.ibm.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> ---
> At this point, I'm only doing a high-level overview (public interface,
> blatant findings) and not a fine-grained reading of the implementation.


Thanks anyway.


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

I added the copyright now, which was obviously missing.


>
>> +/*
>> + * 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?


It would be 2 packets, the 2nd one having 0-length.


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

Same here. 2 packets.

>> +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?


The problem here could be a 64bit variable that would allign differently 
on a 32bit machine versus a 64bit machine or a 32bit executable running 
on a 64bit machine.At least there are no 64bit variables here, so it 
would be ok. However, we can still make the type member 32bit.


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


So remove the _t entirely?


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

The usage so far asks for whether a certain set of capabilities are 
_all_ implemented and for this the
evaluation above is good in call cases. I'll add the additional 
parenthesis, though.


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

Ok.

>
>
>>   /*
>> + * 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'?

Thought it was a coding style requirement .. but it isn't.


>
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * 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

Removed.

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

Ok.


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

Definitely.

Thanks!

    Stefan