From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44412) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxNvB-0000Ph-DP for qemu-devel@nongnu.org; Tue, 26 May 2015 19:06:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxNv8-0006sr-5X for qemu-devel@nongnu.org; Tue, 26 May 2015 19:06:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58553) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxNv7-0006sX-Tl for qemu-devel@nongnu.org; Tue, 26 May 2015 19:06:02 -0400 Message-ID: <5564FC54.6070604@redhat.com> Date: Tue, 26 May 2015 17:05:56 -0600 From: Eric Blake MIME-Version: 1.0 References: <1432676024-1046793-1-git-send-email-stefanb@linux.vnet.ibm.com> <1432676024-1046793-2-git-send-email-stefanb@linux.vnet.ibm.com> In-Reply-To: <1432676024-1046793-2-git-send-email-stefanb@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8AaIaDDHMT9twfSgNUj0CBiG4S9X72OJb" Subject: Re: [Qemu-devel] [PATCH v3 1/6] Provide support for the CUSE TPM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger , mst@redhat.com, qemu-devel@nongnu.org, imammedo@redhat.com Cc: quan.xu@intel.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8AaIaDDHMT9twfSgNUj0CBiG4S9X72OJb Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 =3D character device in userspace). >=20 > 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. >=20 > 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. >=20 > The CUSE TPM and associated tools can be found here: >=20 > https://github.com/stefanberger/swtpm >=20 >=20 > To use the external CUSE TPM, the CUSE TPM should be started as follows= : >=20 > /usr/bin/swtpm_cuse -n vtpm-test >=20 > QEMU can then be started using the following parameters: >=20 > qemu-system-x86_64 \ > [...] \ > -tpmdev cuse-tpm,id=3Dtpm0,cancel-path=3D/dev/null,path=3D/dev/= vtpm-test \ > -device tpm-tis,id=3Dtpm0,tpmdev=3Dtpm0 \ > [...] >=20 >=20 > Signed-off-by: Stefan Berger > Cc: Eric Blake > --- 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 Pub= lic > + * License as published by the Free Software Foundation; either versio= n 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; > =20 > TPMVersion tpm_version; > + ptmcap_t cuse_cap; /* capabilties of the CUSE TPM */ > + uint8_t cur_locty_number; /* last set locality */ s/capabilties/capabilities/ > }; > =20 > typedef struct TPMPassthruState TPMPassthruState; > =20 > #define TPM_PASSTHROUGH_DEFAULT_DEVICE "/dev/tpm0" > =20 > +#define TPM_PASSTHROUGH_USES_CUSE_TPM(tpm_pt) (tpm_pt->cuse_cap !=3D 0= ) > + > +#define TPM_CUSE_IMPLEMENTS(tpm_tr, cap) ((tpm_pt->cuse_cap & cap) =3D= =3D cap) Evaluates cap more than once, which may not be ideal. Also under-parenthesized in the face of arbitrary expressions for tpm_tr or ca= p. 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)) !=3D 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 !=3D locty_number) { > + loc.u.req.loc =3D locty_number; > + n =3D 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 =3D 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 =3D 0; > + int n; > + > + n =3D ioctl(tpm_pt->tpm_fd, PTM_GET_CAPABILITY, &tpm_pt->cuse_cap)= ; > + if (n < 0) { > + error_report("Error: CUSE TPM was requested, but probing faile= d."); 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 =3D TPM_PASSTHROUGH(tb); > int n; > + ptmres_t res; > + static int error_printed; You're using this as a bool... > + } else if (res !=3D TPM_SUCCESS) { > + if (!error_printed) { > + error_report("TPM error code from command " > + "cancellation of CUSE TPM: 0x%x",= res); > + error_printed =3D true; > + } =2E..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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --8AaIaDDHMT9twfSgNUj0CBiG4S9X72OJb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVZPxUAAoJEKeha0olJ0NqiGAIAKf0/e7S7yL4TI9fo5Zsj+2b 3mjAj8OLIstSe/ostZ6TxJ8pVigMYiHCjG7vU4okx31vvopa7ALafVDedFji2YpR Tz3AdXj9qi2WHAPxrIC1blE1kH/5QD29wbTA0fxnft7qyoGHupqjrPiVByQgMIVC yqyo8D3EHi38qRdsdam0suYHmpu9gVhkRTc+fk/h/cujROchWFcMja/ZzCB8cM0T DITAiLS+fhiIgif9DBQnP14ZrXs24EBkoSkgf+MU1I45DhIfP+JOuZZpYLm1AjX6 yiX7jahzv3QJFErzlv+nnYRun26FZdlarEcEr4FKKGDyEXn6ZBtCOs4n6z9zjHo= =zHCD -----END PGP SIGNATURE----- --8AaIaDDHMT9twfSgNUj0CBiG4S9X72OJb--