From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42884) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxQX3-0007on-Ek for qemu-devel@nongnu.org; Tue, 26 May 2015 21:53:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxQWy-0007kk-Ci for qemu-devel@nongnu.org; Tue, 26 May 2015 21:53:21 -0400 Received: from e35.co.us.ibm.com ([32.97.110.153]:55755) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxQWy-0007k2-31 for qemu-devel@nongnu.org; Tue, 26 May 2015 21:53:16 -0400 Received: from /spool/local by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 26 May 2015 19:53:14 -0600 Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id A01F03E4003B for ; Tue, 26 May 2015 19:53:11 -0600 (MDT) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t4R1qwDl36503768 for ; Tue, 26 May 2015 18:52:58 -0700 Received: from d03av01.boulder.ibm.com (localhost [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t4R1rBi3006368 for ; Tue, 26 May 2015 19:53:11 -0600 Message-ID: <55652386.6010307@linux.vnet.ibm.com> Date: Tue, 26 May 2015 21:53:10 -0400 From: Stefan Berger 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> <5564FC54.6070604@redhat.com> In-Reply-To: <5564FC54.6070604@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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: Eric Blake , mst@redhat.com, qemu-devel@nongnu.org, imammedo@redhat.com Cc: quan.xu@intel.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 >> 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. 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