vtpmmgr TPM 2.0 support is incomplete. There is no code to save the tpm2 keys generated by the vtpmmgr, so it's impossible to restore vtpm state with tpm2. The vtpmmgr also issues TPM 1.2 commands to the TPM 2.0 hardware which naturally fails. Dag reported this [1][2], and I independently re-discovered it. I have not fixed the above issues. These are some fixes I made while investigating tpm2 support. At a minimum, "docs: Warn about incomplete vtpmmgr TPM 2.0 support" should be applied to warn others. This is useful for debugging: vtpmmgr: Print error code to aid debugging This fixes vtpmmgr output (also noted by Dag [3]): stubom: newlib: Enable C99 formats for %z This gives more flexibility if you are already using the TPM2 hardware: vtpmmgr: Allow specifying srk_handle for TPM2 These are some changes to unload keys from the TPM hardware (so they are not still loaded for anything that runs afterwards): vtpmmgr: Move vtpmmgr_shutdown vtpmmgr: Flush transient keys on shutdown vtpmmgr: Flush all transient keys vtpmmgr: Shutdown more gracefully This lets vtpms initialize their random pools: vtpmmgr: Support GetRandom passthrough on TPM 2.0 New in v2: TPM2_GetRandom fix per Samuel: vtpmmgr: Remove bogus cast from TPM2_GetRandom Change ":" to "=": vtpmmgr: Fix owner_auth & srk_auth parsing Follow on from comments from Samuel vtpmmgr: Check req_len before unpacking command Fix for vtpm emulator to work with Linux 5.4 vtpm: Correct timeout units and command duration Changes in v2: Added R-by & Ack-by to 1-3,5-8 Updated #4 to use srk_handle= Updated #7 commit message Updated #9 per Samuel Added #10-13 [1] https://lore.kernel.org/xen-devel/8285393.eUs1EhXEQl@eseries.newtech.fi/ [2] https://lore.kernel.org/xen-devel/1615731.eyaQ0j4tC5@eseries.newtech.fi/ [3] https://lore.kernel.org/xen-devel/3151252.0ZAaMuH7Fy@dag.newtech.fi/ Jason Andryuk (13): docs: Warn about incomplete vtpmmgr TPM 2.0 support vtpmmgr: Print error code to aid debugging stubom: newlib: Enable C99 formats for %z vtpmmgr: Allow specifying srk_handle for TPM2 vtpmmgr: Move vtpmmgr_shutdown vtpmmgr: Flush transient keys on shutdown vtpmmgr: Flush all transient keys vtpmmgr: Shutdown more gracefully vtpmmgr: Support GetRandom passthrough on TPM 2.0 vtpmmgr: Remove bogus cast from TPM2_GetRandom vtpmmgr: Fix owner_auth & srk_auth parsing vtpmmgr: Check req_len before unpacking command vtpm: Correct timeout units and command duration docs/man/xen-vtpmmgr.7.pod | 18 +++++++ stubdom/Makefile | 4 +- stubdom/vtpm-command-duration.patch | 52 +++++++++++++++++++ stubdom/vtpm-microsecond-duration.patch | 52 +++++++++++++++++++ stubdom/vtpmmgr/init.c | 57 +++++++++++++-------- stubdom/vtpmmgr/marshal.h | 15 ++++++ stubdom/vtpmmgr/tpm.c | 2 +- stubdom/vtpmmgr/tpm2.c | 15 ++++-- stubdom/vtpmmgr/vtpm_cmd_handler.c | 67 ++++++++++++++++++++++++- stubdom/vtpmmgr/vtpmmgr.c | 12 ++++- 10 files changed, 266 insertions(+), 28 deletions(-) create mode 100644 stubdom/vtpm-command-duration.patch create mode 100644 stubdom/vtpm-microsecond-duration.patch -- 2.30.2
The vtpmmgr TPM 2.0 support is incomplete. Add a warning about that to the documentation so others don't have to work through discovering it is broken. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> --- docs/man/xen-vtpmmgr.7.pod | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod index af825a7ffe..875dcce508 100644 --- a/docs/man/xen-vtpmmgr.7.pod +++ b/docs/man/xen-vtpmmgr.7.pod @@ -222,6 +222,17 @@ XSM label, not the kernel. =head1 Appendix B: vtpmmgr on TPM 2.0 +=head2 WARNING: Incomplete - cannot persist data + +TPM 2.0 support for vTPM manager is incomplete. There is no support for +persisting an encryption key, so vTPM manager regenerates primary and secondary +key handles each boot. + +Also, the vTPM manger group command implementation hardcodes TPM 1.2 commands. +This means running manage-vtpmmgr.pl fails when the TPM 2.0 hardware rejects +the TPM 1.2 commands. vTPM manager with TPM 2.0 cannot create groups and +therefore cannot persist vTPM contents. + =head2 Manager disk image setup: The vTPM Manager requires a disk image to store its encrypted data. The image -- 2.30.2
tpm_get_error_name returns "Unknown Error Code" when an error string is not defined. In that case, we should print the Error Code so it can be looked up offline. tpm_get_error_name returns a const string, so just have the two callers always print the error code so it is always available. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- stubdom/vtpmmgr/tpm.c | 2 +- stubdom/vtpmmgr/tpm2.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/stubdom/vtpmmgr/tpm.c b/stubdom/vtpmmgr/tpm.c index 779cddd64e..83b2bc16b2 100644 --- a/stubdom/vtpmmgr/tpm.c +++ b/stubdom/vtpmmgr/tpm.c @@ -109,7 +109,7 @@ UINT32 rsp_status; \ UNPACK_OUT(TPM_RSP_HEADER, &rsp_tag, &rsp_len, &rsp_status); \ if (rsp_status != TPM_SUCCESS) { \ - vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(rsp_status)); \ + vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(rsp_status), rsp_status); \ status = rsp_status; \ goto abort_egress; \ } \ diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c index c9f1016ab5..655e6d164c 100644 --- a/stubdom/vtpmmgr/tpm2.c +++ b/stubdom/vtpmmgr/tpm2.c @@ -126,7 +126,7 @@ ptr = unpack_TPM_RSP_HEADER(ptr, \ &(tag), &(paramSize), &(status));\ if ((status) != TPM_SUCCESS){ \ - vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(status));\ + vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(status), (status));\ goto abort_egress;\ }\ } while(0) -- 2.30.2
vtpmmgr was changed to print size_t with the %z modifier, but newlib isn't compiled with %z support. So you get output like: root seal: zu; sector of 13: zu root: zu v=zu itree: 36; sector of 112: zu group: zu v=zu id=zu md=zu group seal: zu; 5 in parent: zu; sector of 13: zu vtpm: zu+zu; sector of 48: zu Enable the C99 formats in newlib so vtpmmgr prints the numeric values. Fixes 9379af08ccc0 "stubdom: vtpmmgr: Correctly format size_t with %z when printing." Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- stubdom/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stubdom/Makefile b/stubdom/Makefile index 90d9ffcd9f..c6de5f68ae 100644 --- a/stubdom/Makefile +++ b/stubdom/Makefile @@ -105,7 +105,7 @@ cross-newlib: $(NEWLIB_STAMPFILE) $(NEWLIB_STAMPFILE): mk-headers-$(XEN_TARGET_ARCH) newlib-$(NEWLIB_VERSION) mkdir -p newlib-$(XEN_TARGET_ARCH) ( cd newlib-$(XEN_TARGET_ARCH) && \ - CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --disable-multilib && \ + CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --enable-newlib-io-c99-formats --disable-multilib && \ $(MAKE) DESTDIR= && \ $(MAKE) DESTDIR= install ) -- 2.30.2
Bypass taking ownership of the TPM2 if an srk_handle is specified. This srk_handle must be usable with Null auth for the time being. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- v2: Use "=" seperator --- docs/man/xen-vtpmmgr.7.pod | 7 +++++++ stubdom/vtpmmgr/init.c | 11 ++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod index 875dcce508..3286954568 100644 --- a/docs/man/xen-vtpmmgr.7.pod +++ b/docs/man/xen-vtpmmgr.7.pod @@ -92,6 +92,13 @@ Valid arguments: =over 4 +=item srk_handle=<HANDLE> + +Specify a srk_handle for TPM 2.0. TPM 2.0 uses a key hierarchy, and +this allow specifying the parent handle for vtpmmgr to create its own +key under. Using this option bypasses vtpmmgr trying to take ownership +of the TPM. + =item owner_auth=<AUTHSPEC> =item srk_auth=<AUTHSPEC> diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c index 1506735051..130e4f4bf6 100644 --- a/stubdom/vtpmmgr/init.c +++ b/stubdom/vtpmmgr/init.c @@ -302,6 +302,11 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts) goto err_invalid; } } + else if(!strncmp(argv[i], "srk_handle=", 11)) { + if(sscanf(argv[i] + 11, "%x", &vtpm_globals.srk_handle) != 1) { + goto err_invalid; + } + } else if(!strncmp(argv[i], "tpmdriver=", 10)) { if(!strcmp(argv[i] + 10, "tpm_tis")) { opts->tpmdriver = TPMDRV_TPM_TIS; @@ -586,7 +591,11 @@ TPM_RESULT vtpmmgr2_create(void) { TPM_RESULT status = TPM_SUCCESS; - TPMTRYRETURN(tpm2_take_ownership()); + if ( vtpm_globals.srk_handle == 0 ) { + TPMTRYRETURN(tpm2_take_ownership()); + } else { + tpm2_AuthArea_ctor(NULL, 0, &vtpm_globals.srk_auth_area); + } /* create SK */ TPM2_Create_Params_out out; -- 2.30.2
Reposition vtpmmgr_shutdown so it can call flush_tpm2 without a forward declaration. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- stubdom/vtpmmgr/init.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c index 130e4f4bf6..decf8e8b4d 100644 --- a/stubdom/vtpmmgr/init.c +++ b/stubdom/vtpmmgr/init.c @@ -503,20 +503,6 @@ egress: return status; } -void vtpmmgr_shutdown(void) -{ - /* Cleanup TPM resources */ - TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle); - - /* Close tpmback */ - shutdown_tpmback(); - - /* Close tpmfront/tpm_tis */ - close(vtpm_globals.tpm_fd); - - vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n"); -} - /* TPM 2.0 */ static void tpm2_AuthArea_ctor(const char *authValue, UINT32 authLen, @@ -797,3 +783,17 @@ abort_egress: egress: return status; } + +void vtpmmgr_shutdown(void) +{ + /* Cleanup TPM resources */ + TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle); + + /* Close tpmback */ + shutdown_tpmback(); + + /* Close tpmfront/tpm_tis */ + close(vtpm_globals.tpm_fd); + + vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n"); +} -- 2.30.2
Remove our key so it isn't left in the TPM for someone to come along after vtpmmgr shutsdown. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- stubdom/vtpmmgr/init.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c index decf8e8b4d..56b4be85b3 100644 --- a/stubdom/vtpmmgr/init.c +++ b/stubdom/vtpmmgr/init.c @@ -792,6 +792,14 @@ void vtpmmgr_shutdown(void) /* Close tpmback */ shutdown_tpmback(); + if (hw_is_tpm2()) { + /* Blow away all stale handles left in the tpm*/ + if (flush_tpm2() != TPM_SUCCESS) { + vtpmlogerror(VTPM_LOG_TPM, + "TPM2_FlushResources failed, continuing shutdown..\n"); + } + } + /* Close tpmfront/tpm_tis */ close(vtpm_globals.tpm_fd); -- 2.30.2
We're only flushing 2 transients, but there are 3 handles. Use <= to also flush the third handle since TRANSIENT_LAST is inclusive The number of transient handles/keys is hardware dependent, so this should query for the limit. And assignment of handles is assumed to be sequential from the minimum. That may not be guaranteed, but seems okay with my tpm2. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- v2 add "since TRANSIENT_LAST is inclusive" to commit message. --- stubdom/vtpmmgr/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c index 56b4be85b3..4ae34a4fcb 100644 --- a/stubdom/vtpmmgr/init.c +++ b/stubdom/vtpmmgr/init.c @@ -656,7 +656,7 @@ static TPM_RC flush_tpm2(void) { int i; - for (i = TRANSIENT_FIRST; i < TRANSIENT_LAST; i++) + for (i = TRANSIENT_FIRST; i <= TRANSIENT_LAST; i++) TPM2_FlushContext(i); return TPM_SUCCESS; -- 2.30.2
vtpmmgr uses the default, weak app_shutdown, which immediately calls the shutdown hypercall. This short circuits the vtpmmgr clean up logic. We need to perform the clean up to actually Flush our key out of the tpm. Setting do_shutdown is one step in that direction, but vtpmmgr will most likely be waiting in tpmback_req_any. We need to call shutdown_tpmback to cancel the wait inside tpmback and perform the shutdown. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibaut@ens-lyon.org> --- stubdom/vtpmmgr/vtpmmgr.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/stubdom/vtpmmgr/vtpmmgr.c b/stubdom/vtpmmgr/vtpmmgr.c index 9fddaa24f8..46ea018921 100644 --- a/stubdom/vtpmmgr/vtpmmgr.c +++ b/stubdom/vtpmmgr/vtpmmgr.c @@ -67,11 +67,21 @@ int hw_is_tpm2(void) return (hardware_version.hw_version == TPM2_HARDWARE) ? 1 : 0; } +static int do_shutdown; + +void app_shutdown(unsigned int reason) +{ + printk("Shutdown requested: %d\n", reason); + do_shutdown = 1; + + shutdown_tpmback(); +} + void main_loop(void) { tpmcmd_t* tpmcmd; uint8_t respbuf[TCPA_MAX_BUFFER_LENGTH]; - while(1) { + while (!do_shutdown) { /* Wait for requests from a vtpm */ vtpmloginfo(VTPM_LOG_VTPM, "Waiting for commands from vTPM's:\n"); if((tpmcmd = tpmback_req_any()) == NULL) { -- 2.30.2
GetRandom passthrough currently fails when using vtpmmgr with a hardware TPM 2.0. vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30) When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2 TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command. Besides the differing ordinal, the TPM 1.2 uses 32bit sizes for the request and response (vs. 16bit for TPM2). Place the random output directly into the tpmcmd->resp and build the packet around it. This avoids bouncing through an extra buffer, but the header has to be written after grabbing the random bytes so we have the number of bytes to include in the size. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- v2: Add bounds and size checks Whitespace fixup --- stubdom/vtpmmgr/marshal.h | 15 ++++++++ stubdom/vtpmmgr/vtpm_cmd_handler.c | 61 +++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h index dce19c6439..f1037a7976 100644 --- a/stubdom/vtpmmgr/marshal.h +++ b/stubdom/vtpmmgr/marshal.h @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) { return rv; } +static +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) { + int rv = 0; + rv += sizeof_UINT16(ptr); + rv += sizeof_UINT32(ptr); + rv += sizeof_UINT32(ptr); + return rv; +} + static inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr, TPM_TAG tag, @@ -920,8 +929,14 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max, unpack3_UINT32(ptr, pos, max, ord); } +static +inline int sizeof_TPM_RQU_GetRandom(BYTE* ptr) { + return sizeof_TPM_RQU_HEADER(ptr) + sizeof_UINT32(ptr); +} + #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r) #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r) #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r) +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p) #endif diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c index 2ac14fae77..c879b24c13 100644 --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c @@ -47,6 +47,7 @@ #include "vtpm_disk.h" #include "vtpmmgr.h" #include "tpm.h" +#include "tpm2.h" #include "tpmrsa.h" #include "tcg.h" #include "mgmt_authority.h" @@ -772,6 +773,64 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq) return 1; } +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque, + tpmcmd_t* tpmcmd) +{ + TPM_RESULT status = TPM_SUCCESS; + TPM_TAG tag; + UINT32 size; + const int max_rand_size = TCPA_MAX_BUFFER_LENGTH - + sizeof_TPM_RQU_GetRandom(tpmcmd->req); + UINT32 rand_offset; + UINT32 rand_size; + TPM_COMMAND_CODE ord; + BYTE *p; + + if (tpmcmd->req_len != sizeof_TPM_RQU_GetRandom(tpmcmd->req)) { + status = TPM_BAD_PARAMETER; + tag = TPM_TAG_RQU_COMMAND; + goto abort_egress; + } + + p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); + + if (!hw_is_tpm2()) { + size = TCPA_MAX_BUFFER_LENGTH; + TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len, + tpmcmd->resp, &size)); + tpmcmd->resp_len = size; + + return TPM_SUCCESS; + } + + /* TPM_GetRandom req: <header><uint32 num bytes> */ + unpack_UINT32(p, &rand_size); + + /* Returning fewer bytes is acceptable per the spec. */ + if (rand_size > max_rand_size) + rand_size = max_rand_size; + + /* Call TPM2_GetRandom but return a TPM_GetRandom response. */ + /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */ + rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) + + sizeof_UINT32(tpmcmd->resp); + + TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset)); + + p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND, + rand_offset + rand_size, status); + p = pack_UINT32(p, rand_size); + tpmcmd->resp_len = rand_offset + rand_size; + + return status; + +abort_egress: + tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE; + pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status); + + return status; +} + TPM_RESULT vtpmmgr_handle_cmd( struct tpm_opaque *opaque, tpmcmd_t* tpmcmd) @@ -842,7 +901,7 @@ TPM_RESULT vtpmmgr_handle_cmd( switch(ord) { case TPM_ORD_GetRandom: vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); - break; + return vtpmmgr_handle_getrandom(opaque, tpmcmd); case TPM_ORD_PcrRead: vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n"); // Quotes also need to be restricted to hide PCR values -- 2.30.2
The UINT32 <-> UINT16 casting in TPM2_GetRandom is incorrect. Use a local UINT16 as needed for the TPM hardware command and assign the result. Suggested-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- stubdom/vtpmmgr/tpm2.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c index 655e6d164c..ebd06eac74 100644 --- a/stubdom/vtpmmgr/tpm2.c +++ b/stubdom/vtpmmgr/tpm2.c @@ -427,15 +427,22 @@ abort_egress: TPM_RC TPM2_GetRandom(UINT32 * bytesRequested, BYTE * randomBytes) { + UINT16 bytesReq; TPM_BEGIN(TPM_ST_NO_SESSIONS, TPM_CC_GetRandom); - ptr = pack_UINT16(ptr, (UINT16)*bytesRequested); + if (*bytesRequested > UINT16_MAX) + bytesReq = UINT16_MAX; + else + bytesReq = *bytesRequested; + + ptr = pack_UINT16(ptr, bytesReq); TPM_TRANSMIT(); TPM_UNPACK_VERIFY(); - ptr = unpack_UINT16(ptr, (UINT16 *)bytesRequested); - ptr = unpack_TPM_BUFFER(ptr, randomBytes, *bytesRequested); + ptr = unpack_UINT16(ptr, &bytesReq); + *bytesRequested = bytesReq; + ptr = unpack_TPM_BUFFER(ptr, randomBytes, bytesReq); abort_egress: return status; -- 2.30.2
Argument parsing only matches to before ':' and then the string with leading ':' is passed to parse_auth_string which fails to parse. Extend the length to include the seperator in the match. While here, switch the seperator to "=". The man page documented "=" and the other tpm.* arguments already use "=". Since it didn't work before, we don't need to worry about backwards compatibility. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- stubdom/vtpmmgr/init.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c index 4ae34a4fcb..62dc5994de 100644 --- a/stubdom/vtpmmgr/init.c +++ b/stubdom/vtpmmgr/init.c @@ -289,16 +289,16 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts) memcpy(vtpm_globals.srk_auth, WELLKNOWN_AUTH, sizeof(TPM_AUTHDATA)); for(i = 1; i < argc; ++i) { - if(!strncmp(argv[i], "owner_auth:", 10)) { - if((rc = parse_auth_string(argv[i] + 10, vtpm_globals.owner_auth)) < 0) { + if(!strncmp(argv[i], "owner_auth=", 11)) { + if((rc = parse_auth_string(argv[i] + 11, vtpm_globals.owner_auth)) < 0) { goto err_invalid; } if(rc == 1) { opts->gen_owner_auth = 1; } } - else if(!strncmp(argv[i], "srk_auth:", 8)) { - if((rc = parse_auth_string(argv[i] + 8, vtpm_globals.srk_auth)) != 0) { + else if(!strncmp(argv[i], "srk_auth=", 9)) { + if((rc = parse_auth_string(argv[i] + 9, vtpm_globals.srk_auth)) != 0) { goto err_invalid; } } -- 2.30.2
vtpm_handle_cmd doesn't ensure there is enough space before unpacking the req buffer. Add a minimum size check. Called functions will have to do their own checking if they need more data from the request. The error case is tricky since abort_egress wants to rely with a corresponding tag. Just hardcode TPM_TAG_RQU_COMMAND since the vtpm is sending in malformed commands in the first place. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- stubdom/vtpmmgr/vtpm_cmd_handler.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c index c879b24c13..5586be6997 100644 --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c @@ -840,6 +840,12 @@ TPM_RESULT vtpmmgr_handle_cmd( UINT32 size; TPM_COMMAND_CODE ord; + if (tpmcmd->req_len < sizeof_TPM_RQU_HEADER(tpmcmd->req)) { + status = TPM_BAD_PARAMETER; + tag = TPM_TAG_RQU_COMMAND; + goto abort_egress; + } + unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); -- 2.30.2
Add two patches: vtpm-microsecond-duration.patch fixes the units for timeouts and command durations. vtpm-command-duration.patch increases the timeout linux uses to allow commands to succeed. Linux works around low timeouts, but not low durations. The second patch allows commands to complete that often timeout with the lower command durations. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- stubdom/Makefile | 2 + stubdom/vtpm-command-duration.patch | 52 +++++++++++++++++++++++++ stubdom/vtpm-microsecond-duration.patch | 52 +++++++++++++++++++++++++ 3 files changed, 106 insertions(+) create mode 100644 stubdom/vtpm-command-duration.patch create mode 100644 stubdom/vtpm-microsecond-duration.patch diff --git a/stubdom/Makefile b/stubdom/Makefile index c6de5f68ae..06aa69d8bc 100644 --- a/stubdom/Makefile +++ b/stubdom/Makefile @@ -239,6 +239,8 @@ tpm_emulator-$(XEN_TARGET_ARCH): tpm_emulator-$(TPMEMU_VERSION).tar.gz patch -d $@ -p1 < vtpm-implicit-fallthrough.patch patch -d $@ -p1 < vtpm_TPM_ChangeAuthAsymFinish.patch patch -d $@ -p1 < vtpm_extern.patch + patch -d $@ -p1 < vtpm-microsecond-duration.patch + patch -d $@ -p1 < vtpm-command-duration.patch mkdir $@/build cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) -Wno-declaration-after-statement" touch $@ diff --git a/stubdom/vtpm-command-duration.patch b/stubdom/vtpm-command-duration.patch new file mode 100644 index 0000000000..6fdf2fc9be --- /dev/null +++ b/stubdom/vtpm-command-duration.patch @@ -0,0 +1,52 @@ +From e7c976b5864e7d2649292d90ea60d5aea091a990 Mon Sep 17 00:00:00 2001 +From: Jason Andryuk <jandryuk@gmail.com> +Date: Sun, 14 Mar 2021 12:46:34 -0400 +Subject: [PATCH 2/2] Increase command durations + +Wth Linux 5.4 xen-tpmfront and a Xen vtpm-stubdom, xen-tpmfront was +failing commands with -ETIME: +tpm tpm0: tpm_try_transmit: send(): error-62 + +The vtpm was returning the data, but it was after the duration timeout +in vtpm_send. Linux may have started being more stringent about timing? + +The vtpm-stubdom has a little delay since it writes its disk before +returning the response. + +Anyway, the durations are rather low. When they were 1/10/1000 before +converting to microseconds, Linux showed all three durations rounded to +10000. Update them with values from a physical TPM1.2. These were +taken from a WEC which was software downgraded from a TPM2 to a TPM1.2. +They might be excessive, but I'd rather have a command succeed than +return -ETIME. + +An IFX physical TPM1.2 uses: +1000000 +1500000 +150000000 + +Signed-off-by: Jason Andryuk <jandryuk@gmail.com> +--- + tpm/tpm_data.c | 6 +++--- + 1 file changed, 3 insertions(+), 3 deletions(-) + +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c +index bebaf10..844afca 100644 +--- a/tpm/tpm_data.c ++++ b/tpm/tpm_data.c +@@ -71,9 +71,9 @@ static void init_timeouts(void) + tpmData.permanent.data.tis_timeouts[1] = 2000000; + tpmData.permanent.data.tis_timeouts[2] = 750000; + tpmData.permanent.data.tis_timeouts[3] = 750000; +- tpmData.permanent.data.cmd_durations[0] = 1000; +- tpmData.permanent.data.cmd_durations[1] = 10000; +- tpmData.permanent.data.cmd_durations[2] = 1000000; ++ tpmData.permanent.data.cmd_durations[0] = 3000000; ++ tpmData.permanent.data.cmd_durations[1] = 3000000; ++ tpmData.permanent.data.cmd_durations[2] = 600000000; + } + + void tpm_init_data(void) +-- +2.30.2 + diff --git a/stubdom/vtpm-microsecond-duration.patch b/stubdom/vtpm-microsecond-duration.patch new file mode 100644 index 0000000000..7a906e72c5 --- /dev/null +++ b/stubdom/vtpm-microsecond-duration.patch @@ -0,0 +1,52 @@ +From 5a510e0afd7c288e3f0fb3523ec749ba1366ad61 Mon Sep 17 00:00:00 2001 +From: Jason Andryuk <jandryuk@gmail.com> +Date: Sun, 14 Mar 2021 12:42:10 -0400 +Subject: [PATCH 1/2] Use microseconds for timeouts and durations + +The timeout and duration fields should be in microseconds according to +the spec. + +TPM_CAP_PROP_TIS_TIMEOUT: +A 4 element array of UINT32 values each denoting the timeout value in +microseconds for the following in this order: + +TPM_CAP_PROP_DURATION: +A 3 element array of UINT32 values each denoting the duration value in +microseconds of the duration of the three classes of commands: + +Linux will scale the timeouts up by 1000, but not the durations. Change +the units for both sets as appropriate. + +Signed-off-by: Jason Andryuk <jandryuk@gmail.com> +--- + tpm/tpm_data.c | 14 +++++++------- + 1 file changed, 7 insertions(+), 7 deletions(-) + +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c +index a3a79ef..bebaf10 100644 +--- a/tpm/tpm_data.c ++++ b/tpm/tpm_data.c +@@ -67,13 +67,13 @@ static void init_nv_storage(void) + static void init_timeouts(void) + { + /* for the timeouts we use the PC platform defaults */ +- tpmData.permanent.data.tis_timeouts[0] = 750; +- tpmData.permanent.data.tis_timeouts[1] = 2000; +- tpmData.permanent.data.tis_timeouts[2] = 750; +- tpmData.permanent.data.tis_timeouts[3] = 750; +- tpmData.permanent.data.cmd_durations[0] = 1; +- tpmData.permanent.data.cmd_durations[1] = 10; +- tpmData.permanent.data.cmd_durations[2] = 1000; ++ tpmData.permanent.data.tis_timeouts[0] = 750000; ++ tpmData.permanent.data.tis_timeouts[1] = 2000000; ++ tpmData.permanent.data.tis_timeouts[2] = 750000; ++ tpmData.permanent.data.tis_timeouts[3] = 750000; ++ tpmData.permanent.data.cmd_durations[0] = 1000; ++ tpmData.permanent.data.cmd_durations[1] = 10000; ++ tpmData.permanent.data.cmd_durations[2] = 1000000; + } + + void tpm_init_data(void) +-- +2.30.2 + -- 2.30.2
On Thu, May 6, 2021 at 10:00 AM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> vtpmmgr uses the default, weak app_shutdown, which immediately calls the
> shutdown hypercall. This short circuits the vtpmmgr clean up logic. We
> need to perform the clean up to actually Flush our key out of the tpm.
>
> Setting do_shutdown is one step in that direction, but vtpmmgr will most
> likely be waiting in tpmback_req_any. We need to call shutdown_tpmback
> to cancel the wait inside tpmback and perform the shutdown.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> Reviewed-by: Samuel Thibault <samuel.thibaut@ens-lyon.org>
Whoops, this should be "Reviewed-by: Samuel Thibault
<samuel.thibault@ens-lyon.org>". The above is missing an "l" in the
last name.
-Jason
Jason Andryuk, le jeu. 06 mai 2021 09:59:14 -0400, a ecrit: > Bypass taking ownership of the TPM2 if an srk_handle is specified. > > This srk_handle must be usable with Null auth for the time being. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > v2: Use "=" seperator > --- > docs/man/xen-vtpmmgr.7.pod | 7 +++++++ > stubdom/vtpmmgr/init.c | 11 ++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod > index 875dcce508..3286954568 100644 > --- a/docs/man/xen-vtpmmgr.7.pod > +++ b/docs/man/xen-vtpmmgr.7.pod > @@ -92,6 +92,13 @@ Valid arguments: > > =over 4 > > +=item srk_handle=<HANDLE> > + > +Specify a srk_handle for TPM 2.0. TPM 2.0 uses a key hierarchy, and > +this allow specifying the parent handle for vtpmmgr to create its own > +key under. Using this option bypasses vtpmmgr trying to take ownership > +of the TPM. > + > =item owner_auth=<AUTHSPEC> > > =item srk_auth=<AUTHSPEC> > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index 1506735051..130e4f4bf6 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -302,6 +302,11 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts) > goto err_invalid; > } > } > + else if(!strncmp(argv[i], "srk_handle=", 11)) { > + if(sscanf(argv[i] + 11, "%x", &vtpm_globals.srk_handle) != 1) { > + goto err_invalid; > + } > + } > else if(!strncmp(argv[i], "tpmdriver=", 10)) { > if(!strcmp(argv[i] + 10, "tpm_tis")) { > opts->tpmdriver = TPMDRV_TPM_TIS; > @@ -586,7 +591,11 @@ TPM_RESULT vtpmmgr2_create(void) > { > TPM_RESULT status = TPM_SUCCESS; > > - TPMTRYRETURN(tpm2_take_ownership()); > + if ( vtpm_globals.srk_handle == 0 ) { > + TPMTRYRETURN(tpm2_take_ownership()); > + } else { > + tpm2_AuthArea_ctor(NULL, 0, &vtpm_globals.srk_auth_area); > + } > > /* create SK */ > TPM2_Create_Params_out out; > -- > 2.30.2 > -- Samuel "...[Linux's] capacity to talk via any medium except smoke signals." (By Dr. Greg Wettstein, Roger Maris Cancer Center)
Jason Andryuk, le jeu. 06 mai 2021 09:59:19 -0400, a ecrit: > GetRandom passthrough currently fails when using vtpmmgr with a hardware > TPM 2.0. > vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom > vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30) > > When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2 > TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command. Besides the > differing ordinal, the TPM 1.2 uses 32bit sizes for the request and > response (vs. 16bit for TPM2). > > Place the random output directly into the tpmcmd->resp and build the > packet around it. This avoids bouncing through an extra buffer, but the > header has to be written after grabbing the random bytes so we have the > number of bytes to include in the size. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > v2: > Add bounds and size checks > Whitespace fixup > --- > stubdom/vtpmmgr/marshal.h | 15 ++++++++ > stubdom/vtpmmgr/vtpm_cmd_handler.c | 61 +++++++++++++++++++++++++++++- > 2 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h > index dce19c6439..f1037a7976 100644 > --- a/stubdom/vtpmmgr/marshal.h > +++ b/stubdom/vtpmmgr/marshal.h > @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) { > return rv; > } > > +static > +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) { > + int rv = 0; > + rv += sizeof_UINT16(ptr); > + rv += sizeof_UINT32(ptr); > + rv += sizeof_UINT32(ptr); > + return rv; > +} > + > static > inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr, > TPM_TAG tag, > @@ -920,8 +929,14 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max, > unpack3_UINT32(ptr, pos, max, ord); > } > > +static > +inline int sizeof_TPM_RQU_GetRandom(BYTE* ptr) { > + return sizeof_TPM_RQU_HEADER(ptr) + sizeof_UINT32(ptr); > +} > + > #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r) > #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r) > #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r) > +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p) > > #endif > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c > index 2ac14fae77..c879b24c13 100644 > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c > @@ -47,6 +47,7 @@ > #include "vtpm_disk.h" > #include "vtpmmgr.h" > #include "tpm.h" > +#include "tpm2.h" > #include "tpmrsa.h" > #include "tcg.h" > #include "mgmt_authority.h" > @@ -772,6 +773,64 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq) > return 1; > } > > +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque, > + tpmcmd_t* tpmcmd) > +{ > + TPM_RESULT status = TPM_SUCCESS; > + TPM_TAG tag; > + UINT32 size; > + const int max_rand_size = TCPA_MAX_BUFFER_LENGTH - > + sizeof_TPM_RQU_GetRandom(tpmcmd->req); > + UINT32 rand_offset; > + UINT32 rand_size; > + TPM_COMMAND_CODE ord; > + BYTE *p; > + > + if (tpmcmd->req_len != sizeof_TPM_RQU_GetRandom(tpmcmd->req)) { > + status = TPM_BAD_PARAMETER; > + tag = TPM_TAG_RQU_COMMAND; > + goto abort_egress; > + } > + > + p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); > + > + if (!hw_is_tpm2()) { > + size = TCPA_MAX_BUFFER_LENGTH; > + TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len, > + tpmcmd->resp, &size)); > + tpmcmd->resp_len = size; > + > + return TPM_SUCCESS; > + } > + > + /* TPM_GetRandom req: <header><uint32 num bytes> */ > + unpack_UINT32(p, &rand_size); > + > + /* Returning fewer bytes is acceptable per the spec. */ > + if (rand_size > max_rand_size) > + rand_size = max_rand_size; > + > + /* Call TPM2_GetRandom but return a TPM_GetRandom response. */ > + /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */ > + rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) + > + sizeof_UINT32(tpmcmd->resp); > + > + TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset)); > + > + p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND, > + rand_offset + rand_size, status); > + p = pack_UINT32(p, rand_size); > + tpmcmd->resp_len = rand_offset + rand_size; > + > + return status; > + > +abort_egress: > + tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE; > + pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status); > + > + return status; > +} > + > TPM_RESULT vtpmmgr_handle_cmd( > struct tpm_opaque *opaque, > tpmcmd_t* tpmcmd) > @@ -842,7 +901,7 @@ TPM_RESULT vtpmmgr_handle_cmd( > switch(ord) { > case TPM_ORD_GetRandom: > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); > - break; > + return vtpmmgr_handle_getrandom(opaque, tpmcmd); > case TPM_ORD_PcrRead: > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n"); > // Quotes also need to be restricted to hide PCR values > -- > 2.30.2 > -- Samuel <i8b4uUnderground> d-_-b <BonyNoMore> how u make that inverted b? <BonyNoMore> wait <BonyNoMore> never mind
Jason Andryuk, le jeu. 06 mai 2021 09:59:20 -0400, a ecrit: > The UINT32 <-> UINT16 casting in TPM2_GetRandom is incorrect. Use a > local UINT16 as needed for the TPM hardware command and assign the > result. > > Suggested-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > stubdom/vtpmmgr/tpm2.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c > index 655e6d164c..ebd06eac74 100644 > --- a/stubdom/vtpmmgr/tpm2.c > +++ b/stubdom/vtpmmgr/tpm2.c > @@ -427,15 +427,22 @@ abort_egress: > > TPM_RC TPM2_GetRandom(UINT32 * bytesRequested, BYTE * randomBytes) > { > + UINT16 bytesReq; > TPM_BEGIN(TPM_ST_NO_SESSIONS, TPM_CC_GetRandom); > > - ptr = pack_UINT16(ptr, (UINT16)*bytesRequested); > + if (*bytesRequested > UINT16_MAX) > + bytesReq = UINT16_MAX; > + else > + bytesReq = *bytesRequested; > + > + ptr = pack_UINT16(ptr, bytesReq); > > TPM_TRANSMIT(); > TPM_UNPACK_VERIFY(); > > - ptr = unpack_UINT16(ptr, (UINT16 *)bytesRequested); > - ptr = unpack_TPM_BUFFER(ptr, randomBytes, *bytesRequested); > + ptr = unpack_UINT16(ptr, &bytesReq); > + *bytesRequested = bytesReq; > + ptr = unpack_TPM_BUFFER(ptr, randomBytes, bytesReq); > > abort_egress: > return status; > -- > 2.30.2 > -- Samuel <N> (* If you have a precise idea of the intended use of the following code, please <N> write to Eduardo.Gimenez@inria.fr and ask for the prize :-) <N> -- Eduardo (11/8/97) *) -+- N sur #ens-mim - et c'était un des développeurs -+-
Jason Andryuk, le jeu. 06 mai 2021 09:59:21 -0400, a ecrit: > Argument parsing only matches to before ':' and then the string with > leading ':' is passed to parse_auth_string which fails to parse. Extend > the length to include the seperator in the match. > > While here, switch the seperator to "=". The man page documented "=" > and the other tpm.* arguments already use "=". Since it didn't work > before, we don't need to worry about backwards compatibility. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > stubdom/vtpmmgr/init.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index 4ae34a4fcb..62dc5994de 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -289,16 +289,16 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts) > memcpy(vtpm_globals.srk_auth, WELLKNOWN_AUTH, sizeof(TPM_AUTHDATA)); > > for(i = 1; i < argc; ++i) { > - if(!strncmp(argv[i], "owner_auth:", 10)) { > - if((rc = parse_auth_string(argv[i] + 10, vtpm_globals.owner_auth)) < 0) { > + if(!strncmp(argv[i], "owner_auth=", 11)) { > + if((rc = parse_auth_string(argv[i] + 11, vtpm_globals.owner_auth)) < 0) { > goto err_invalid; > } > if(rc == 1) { > opts->gen_owner_auth = 1; > } > } > - else if(!strncmp(argv[i], "srk_auth:", 8)) { > - if((rc = parse_auth_string(argv[i] + 8, vtpm_globals.srk_auth)) != 0) { > + else if(!strncmp(argv[i], "srk_auth=", 9)) { > + if((rc = parse_auth_string(argv[i] + 9, vtpm_globals.srk_auth)) != 0) { > goto err_invalid; > } > } > -- > 2.30.2 > -- Samuel c> ah (on trouve fluide glacial sur le net, ou il faut aller dans le monde reel ?) s> dans le monde reel c> zut
Jason Andryuk, le jeu. 06 mai 2021 09:59:22 -0400, a ecrit: > vtpm_handle_cmd doesn't ensure there is enough space before unpacking > the req buffer. Add a minimum size check. Called functions will have > to do their own checking if they need more data from the request. > > The error case is tricky since abort_egress wants to rely with a > corresponding tag. Just hardcode TPM_TAG_RQU_COMMAND since the vtpm is > sending in malformed commands in the first place. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > stubdom/vtpmmgr/vtpm_cmd_handler.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c > index c879b24c13..5586be6997 100644 > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c > @@ -840,6 +840,12 @@ TPM_RESULT vtpmmgr_handle_cmd( > UINT32 size; > TPM_COMMAND_CODE ord; > > + if (tpmcmd->req_len < sizeof_TPM_RQU_HEADER(tpmcmd->req)) { > + status = TPM_BAD_PARAMETER; > + tag = TPM_TAG_RQU_COMMAND; > + goto abort_egress; > + } > + > unpack_TPM_RQU_HEADER(tpmcmd->req, > &tag, &size, &ord); > > -- > 2.30.2 > -- Samuel j'etais en train de nettoyer ma souris et le coup est parti... -+- s sur #ens-mim - et en plus c vrai... -+-
Jason Andryuk, le jeu. 06 mai 2021 09:59:23 -0400, a ecrit: > Add two patches: > vtpm-microsecond-duration.patch fixes the units for timeouts and command > durations. > vtpm-command-duration.patch increases the timeout linux uses to allow > commands to succeed. > > Linux works around low timeouts, but not low durations. The second > patch allows commands to complete that often timeout with the lower > command durations. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > stubdom/Makefile | 2 + > stubdom/vtpm-command-duration.patch | 52 +++++++++++++++++++++++++ > stubdom/vtpm-microsecond-duration.patch | 52 +++++++++++++++++++++++++ > 3 files changed, 106 insertions(+) > create mode 100644 stubdom/vtpm-command-duration.patch > create mode 100644 stubdom/vtpm-microsecond-duration.patch > > diff --git a/stubdom/Makefile b/stubdom/Makefile > index c6de5f68ae..06aa69d8bc 100644 > --- a/stubdom/Makefile > +++ b/stubdom/Makefile > @@ -239,6 +239,8 @@ tpm_emulator-$(XEN_TARGET_ARCH): tpm_emulator-$(TPMEMU_VERSION).tar.gz > patch -d $@ -p1 < vtpm-implicit-fallthrough.patch > patch -d $@ -p1 < vtpm_TPM_ChangeAuthAsymFinish.patch > patch -d $@ -p1 < vtpm_extern.patch > + patch -d $@ -p1 < vtpm-microsecond-duration.patch > + patch -d $@ -p1 < vtpm-command-duration.patch > mkdir $@/build > cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) -Wno-declaration-after-statement" > touch $@ > diff --git a/stubdom/vtpm-command-duration.patch b/stubdom/vtpm-command-duration.patch > new file mode 100644 > index 0000000000..6fdf2fc9be > --- /dev/null > +++ b/stubdom/vtpm-command-duration.patch > @@ -0,0 +1,52 @@ > +From e7c976b5864e7d2649292d90ea60d5aea091a990 Mon Sep 17 00:00:00 2001 > +From: Jason Andryuk <jandryuk@gmail.com> > +Date: Sun, 14 Mar 2021 12:46:34 -0400 > +Subject: [PATCH 2/2] Increase command durations > + > +Wth Linux 5.4 xen-tpmfront and a Xen vtpm-stubdom, xen-tpmfront was > +failing commands with -ETIME: > +tpm tpm0: tpm_try_transmit: send(): error-62 > + > +The vtpm was returning the data, but it was after the duration timeout > +in vtpm_send. Linux may have started being more stringent about timing? > + > +The vtpm-stubdom has a little delay since it writes its disk before > +returning the response. > + > +Anyway, the durations are rather low. When they were 1/10/1000 before > +converting to microseconds, Linux showed all three durations rounded to > +10000. Update them with values from a physical TPM1.2. These were > +taken from a WEC which was software downgraded from a TPM2 to a TPM1.2. > +They might be excessive, but I'd rather have a command succeed than > +return -ETIME. > + > +An IFX physical TPM1.2 uses: > +1000000 > +1500000 > +150000000 > + > +Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > +--- > + tpm/tpm_data.c | 6 +++--- > + 1 file changed, 3 insertions(+), 3 deletions(-) > + > +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c > +index bebaf10..844afca 100644 > +--- a/tpm/tpm_data.c > ++++ b/tpm/tpm_data.c > +@@ -71,9 +71,9 @@ static void init_timeouts(void) > + tpmData.permanent.data.tis_timeouts[1] = 2000000; > + tpmData.permanent.data.tis_timeouts[2] = 750000; > + tpmData.permanent.data.tis_timeouts[3] = 750000; > +- tpmData.permanent.data.cmd_durations[0] = 1000; > +- tpmData.permanent.data.cmd_durations[1] = 10000; > +- tpmData.permanent.data.cmd_durations[2] = 1000000; > ++ tpmData.permanent.data.cmd_durations[0] = 3000000; > ++ tpmData.permanent.data.cmd_durations[1] = 3000000; > ++ tpmData.permanent.data.cmd_durations[2] = 600000000; > + } > + > + void tpm_init_data(void) > +-- > +2.30.2 > + > diff --git a/stubdom/vtpm-microsecond-duration.patch b/stubdom/vtpm-microsecond-duration.patch > new file mode 100644 > index 0000000000..7a906e72c5 > --- /dev/null > +++ b/stubdom/vtpm-microsecond-duration.patch > @@ -0,0 +1,52 @@ > +From 5a510e0afd7c288e3f0fb3523ec749ba1366ad61 Mon Sep 17 00:00:00 2001 > +From: Jason Andryuk <jandryuk@gmail.com> > +Date: Sun, 14 Mar 2021 12:42:10 -0400 > +Subject: [PATCH 1/2] Use microseconds for timeouts and durations > + > +The timeout and duration fields should be in microseconds according to > +the spec. > + > +TPM_CAP_PROP_TIS_TIMEOUT: > +A 4 element array of UINT32 values each denoting the timeout value in > +microseconds for the following in this order: > + > +TPM_CAP_PROP_DURATION: > +A 3 element array of UINT32 values each denoting the duration value in > +microseconds of the duration of the three classes of commands: > + > +Linux will scale the timeouts up by 1000, but not the durations. Change > +the units for both sets as appropriate. > + > +Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > +--- > + tpm/tpm_data.c | 14 +++++++------- > + 1 file changed, 7 insertions(+), 7 deletions(-) > + > +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c > +index a3a79ef..bebaf10 100644 > +--- a/tpm/tpm_data.c > ++++ b/tpm/tpm_data.c > +@@ -67,13 +67,13 @@ static void init_nv_storage(void) > + static void init_timeouts(void) > + { > + /* for the timeouts we use the PC platform defaults */ > +- tpmData.permanent.data.tis_timeouts[0] = 750; > +- tpmData.permanent.data.tis_timeouts[1] = 2000; > +- tpmData.permanent.data.tis_timeouts[2] = 750; > +- tpmData.permanent.data.tis_timeouts[3] = 750; > +- tpmData.permanent.data.cmd_durations[0] = 1; > +- tpmData.permanent.data.cmd_durations[1] = 10; > +- tpmData.permanent.data.cmd_durations[2] = 1000; > ++ tpmData.permanent.data.tis_timeouts[0] = 750000; > ++ tpmData.permanent.data.tis_timeouts[1] = 2000000; > ++ tpmData.permanent.data.tis_timeouts[2] = 750000; > ++ tpmData.permanent.data.tis_timeouts[3] = 750000; > ++ tpmData.permanent.data.cmd_durations[0] = 1000; > ++ tpmData.permanent.data.cmd_durations[1] = 10000; > ++ tpmData.permanent.data.cmd_durations[2] = 1000000; > + } > + > + void tpm_init_data(void) > +-- > +2.30.2 > + > -- > 2.30.2 > -- Samuel Pour un père, autant mourir que de faire plein de calculs et pas s'occuper de son fils -+- y sur #ens-mim - sombres histoires de zombies -+-
On 5/6/21 9:59 AM, Jason Andryuk wrote: > Reposition vtpmmgr_shutdown so it can call flush_tpm2 without a forward > declaration. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/init.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index 130e4f4bf6..decf8e8b4d 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -503,20 +503,6 @@ egress: > return status; > } > > -void vtpmmgr_shutdown(void) > -{ > - /* Cleanup TPM resources */ > - TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle); > - > - /* Close tpmback */ > - shutdown_tpmback(); > - > - /* Close tpmfront/tpm_tis */ > - close(vtpm_globals.tpm_fd); > - > - vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n"); > -} > - > /* TPM 2.0 */ > > static void tpm2_AuthArea_ctor(const char *authValue, UINT32 authLen, > @@ -797,3 +783,17 @@ abort_egress: > egress: > return status; > } > + > +void vtpmmgr_shutdown(void) > +{ > + /* Cleanup TPM resources */ > + TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle); > + > + /* Close tpmback */ > + shutdown_tpmback(); > + > + /* Close tpmfront/tpm_tis */ > + close(vtpm_globals.tpm_fd); > + > + vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n"); > +} >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > Bypass taking ownership of the TPM2 if an srk_handle is specified. > > This srk_handle must be usable with Null auth for the time being. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > v2: Use "=" seperator > --- > docs/man/xen-vtpmmgr.7.pod | 7 +++++++ > stubdom/vtpmmgr/init.c | 11 ++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod > index 875dcce508..3286954568 100644 > --- a/docs/man/xen-vtpmmgr.7.pod > +++ b/docs/man/xen-vtpmmgr.7.pod > @@ -92,6 +92,13 @@ Valid arguments: > > =over 4 > > +=item srk_handle=<HANDLE> > + > +Specify a srk_handle for TPM 2.0. TPM 2.0 uses a key hierarchy, and > +this allow specifying the parent handle for vtpmmgr to create its own > +key under. Using this option bypasses vtpmmgr trying to take ownership > +of the TPM. > + > =item owner_auth=<AUTHSPEC> > > =item srk_auth=<AUTHSPEC> > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index 1506735051..130e4f4bf6 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -302,6 +302,11 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts) > goto err_invalid; > } > } > + else if(!strncmp(argv[i], "srk_handle=", 11)) { > + if(sscanf(argv[i] + 11, "%x", &vtpm_globals.srk_handle) != 1) { > + goto err_invalid; > + } > + } > else if(!strncmp(argv[i], "tpmdriver=", 10)) { > if(!strcmp(argv[i] + 10, "tpm_tis")) { > opts->tpmdriver = TPMDRV_TPM_TIS; > @@ -586,7 +591,11 @@ TPM_RESULT vtpmmgr2_create(void) > { > TPM_RESULT status = TPM_SUCCESS; > > - TPMTRYRETURN(tpm2_take_ownership()); > + if ( vtpm_globals.srk_handle == 0 ) { > + TPMTRYRETURN(tpm2_take_ownership()); > + } else { > + tpm2_AuthArea_ctor(NULL, 0, &vtpm_globals.srk_auth_area); > + } > > /* create SK */ > TPM2_Create_Params_out out; >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > Remove our key so it isn't left in the TPM for someone to come along > after vtpmmgr shutsdown. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/init.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index decf8e8b4d..56b4be85b3 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -792,6 +792,14 @@ void vtpmmgr_shutdown(void) > /* Close tpmback */ > shutdown_tpmback(); > > + if (hw_is_tpm2()) { > + /* Blow away all stale handles left in the tpm*/ > + if (flush_tpm2() != TPM_SUCCESS) { > + vtpmlogerror(VTPM_LOG_TPM, > + "TPM2_FlushResources failed, continuing shutdown..\n"); > + } > + } > + > /* Close tpmfront/tpm_tis */ > close(vtpm_globals.tpm_fd); > >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > We're only flushing 2 transients, but there are 3 handles. Use <= to also > flush the third handle since TRANSIENT_LAST is inclusive > > The number of transient handles/keys is hardware dependent, so this > should query for the limit. And assignment of handles is assumed to be > sequential from the minimum. That may not be guaranteed, but seems okay > with my tpm2. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > v2 add "since TRANSIENT_LAST is inclusive" to commit message. > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index 56b4be85b3..4ae34a4fcb 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -656,7 +656,7 @@ static TPM_RC flush_tpm2(void) > { > int i; > > - for (i = TRANSIENT_FIRST; i < TRANSIENT_LAST; i++) > + for (i = TRANSIENT_FIRST; i <= TRANSIENT_LAST; i++) > TPM2_FlushContext(i); > > return TPM_SUCCESS; >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > vtpmmgr uses the default, weak app_shutdown, which immediately calls the > shutdown hypercall. This short circuits the vtpmmgr clean up logic. We > need to perform the clean up to actually Flush our key out of the tpm. > > Setting do_shutdown is one step in that direction, but vtpmmgr will most > likely be waiting in tpmback_req_any. We need to call shutdown_tpmback > to cancel the wait inside tpmback and perform the shutdown. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > Reviewed-by: Samuel Thibault <samuel.thibaut@ens-lyon.org> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/vtpmmgr.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/stubdom/vtpmmgr/vtpmmgr.c b/stubdom/vtpmmgr/vtpmmgr.c > index 9fddaa24f8..46ea018921 100644 > --- a/stubdom/vtpmmgr/vtpmmgr.c > +++ b/stubdom/vtpmmgr/vtpmmgr.c > @@ -67,11 +67,21 @@ int hw_is_tpm2(void) > return (hardware_version.hw_version == TPM2_HARDWARE) ? 1 : 0; > } > > +static int do_shutdown; > + > +void app_shutdown(unsigned int reason) > +{ > + printk("Shutdown requested: %d\n", reason); > + do_shutdown = 1; > + > + shutdown_tpmback(); > +} > + > void main_loop(void) { > tpmcmd_t* tpmcmd; > uint8_t respbuf[TCPA_MAX_BUFFER_LENGTH]; > > - while(1) { > + while (!do_shutdown) { > /* Wait for requests from a vtpm */ > vtpmloginfo(VTPM_LOG_VTPM, "Waiting for commands from vTPM's:\n"); > if((tpmcmd = tpmback_req_any()) == NULL) { >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > GetRandom passthrough currently fails when using vtpmmgr with a hardware > TPM 2.0. > vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom > vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30) > > When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2 > TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command. Besides the > differing ordinal, the TPM 1.2 uses 32bit sizes for the request and > response (vs. 16bit for TPM2). > > Place the random output directly into the tpmcmd->resp and build the > packet around it. This avoids bouncing through an extra buffer, but the > header has to be written after grabbing the random bytes so we have the > number of bytes to include in the size. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > --- > v2: > Add bounds and size checks > Whitespace fixup > --- Reviewed by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/marshal.h | 15 ++++++++ > stubdom/vtpmmgr/vtpm_cmd_handler.c | 61 +++++++++++++++++++++++++++++- > 2 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h > index dce19c6439..f1037a7976 100644 > --- a/stubdom/vtpmmgr/marshal.h > +++ b/stubdom/vtpmmgr/marshal.h > @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) { > return rv; > } > > +static > +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) { > + int rv = 0; > + rv += sizeof_UINT16(ptr); > + rv += sizeof_UINT32(ptr); > + rv += sizeof_UINT32(ptr); > + return rv; > +} > + > static > inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr, > TPM_TAG tag, > @@ -920,8 +929,14 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max, > unpack3_UINT32(ptr, pos, max, ord); > } > > +static > +inline int sizeof_TPM_RQU_GetRandom(BYTE* ptr) { > + return sizeof_TPM_RQU_HEADER(ptr) + sizeof_UINT32(ptr); > +} > + > #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r) > #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r) > #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r) > +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p) > > #endif > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c > index 2ac14fae77..c879b24c13 100644 > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c > @@ -47,6 +47,7 @@ > #include "vtpm_disk.h" > #include "vtpmmgr.h" > #include "tpm.h" > +#include "tpm2.h" > #include "tpmrsa.h" > #include "tcg.h" > #include "mgmt_authority.h" > @@ -772,6 +773,64 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq) > return 1; > } > > +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque, > + tpmcmd_t* tpmcmd) > +{ > + TPM_RESULT status = TPM_SUCCESS; > + TPM_TAG tag; > + UINT32 size; > + const int max_rand_size = TCPA_MAX_BUFFER_LENGTH - > + sizeof_TPM_RQU_GetRandom(tpmcmd->req); > + UINT32 rand_offset; > + UINT32 rand_size; > + TPM_COMMAND_CODE ord; > + BYTE *p; > + > + if (tpmcmd->req_len != sizeof_TPM_RQU_GetRandom(tpmcmd->req)) { > + status = TPM_BAD_PARAMETER; > + tag = TPM_TAG_RQU_COMMAND; > + goto abort_egress; > + } > + > + p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); > + > + if (!hw_is_tpm2()) { > + size = TCPA_MAX_BUFFER_LENGTH; > + TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len, > + tpmcmd->resp, &size)); > + tpmcmd->resp_len = size; > + > + return TPM_SUCCESS; > + } > + > + /* TPM_GetRandom req: <header><uint32 num bytes> */ > + unpack_UINT32(p, &rand_size); > + > + /* Returning fewer bytes is acceptable per the spec. */ > + if (rand_size > max_rand_size) > + rand_size = max_rand_size; > + > + /* Call TPM2_GetRandom but return a TPM_GetRandom response. */ > + /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */ > + rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) + > + sizeof_UINT32(tpmcmd->resp); > + > + TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset)); > + > + p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND, > + rand_offset + rand_size, status); > + p = pack_UINT32(p, rand_size); > + tpmcmd->resp_len = rand_offset + rand_size; > + > + return status; > + > +abort_egress: > + tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE; > + pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status); > + > + return status; > +} > + > TPM_RESULT vtpmmgr_handle_cmd( > struct tpm_opaque *opaque, > tpmcmd_t* tpmcmd) > @@ -842,7 +901,7 @@ TPM_RESULT vtpmmgr_handle_cmd( > switch(ord) { > case TPM_ORD_GetRandom: > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); > - break; > + return vtpmmgr_handle_getrandom(opaque, tpmcmd); > case TPM_ORD_PcrRead: > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n"); > // Quotes also need to be restricted to hide PCR values >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > The UINT32 <-> UINT16 casting in TPM2_GetRandom is incorrect. Use a > local UINT16 as needed for the TPM hardware command and assign the > result. > > Suggested-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/tpm2.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c > index 655e6d164c..ebd06eac74 100644 > --- a/stubdom/vtpmmgr/tpm2.c > +++ b/stubdom/vtpmmgr/tpm2.c > @@ -427,15 +427,22 @@ abort_egress: > > TPM_RC TPM2_GetRandom(UINT32 * bytesRequested, BYTE * randomBytes) > { > + UINT16 bytesReq; > TPM_BEGIN(TPM_ST_NO_SESSIONS, TPM_CC_GetRandom); > > - ptr = pack_UINT16(ptr, (UINT16)*bytesRequested); > + if (*bytesRequested > UINT16_MAX) > + bytesReq = UINT16_MAX; > + else > + bytesReq = *bytesRequested; > + > + ptr = pack_UINT16(ptr, bytesReq); > > TPM_TRANSMIT(); > TPM_UNPACK_VERIFY(); > > - ptr = unpack_UINT16(ptr, (UINT16 *)bytesRequested); > - ptr = unpack_TPM_BUFFER(ptr, randomBytes, *bytesRequested); > + ptr = unpack_UINT16(ptr, &bytesReq); > + *bytesRequested = bytesReq; > + ptr = unpack_TPM_BUFFER(ptr, randomBytes, bytesReq); > > abort_egress: > return status; >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > Argument parsing only matches to before ':' and then the string with > leading ':' is passed to parse_auth_string which fails to parse. Extend > the length to include the seperator in the match. > > While here, switch the seperator to "=". The man page documented "=" > and the other tpm.* arguments already use "=". Since it didn't work > before, we don't need to worry about backwards compatibility. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/init.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c > index 4ae34a4fcb..62dc5994de 100644 > --- a/stubdom/vtpmmgr/init.c > +++ b/stubdom/vtpmmgr/init.c > @@ -289,16 +289,16 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts) > memcpy(vtpm_globals.srk_auth, WELLKNOWN_AUTH, sizeof(TPM_AUTHDATA)); > > for(i = 1; i < argc; ++i) { > - if(!strncmp(argv[i], "owner_auth:", 10)) { > - if((rc = parse_auth_string(argv[i] + 10, vtpm_globals.owner_auth)) < 0) { > + if(!strncmp(argv[i], "owner_auth=", 11)) { > + if((rc = parse_auth_string(argv[i] + 11, vtpm_globals.owner_auth)) < 0) { > goto err_invalid; > } > if(rc == 1) { > opts->gen_owner_auth = 1; > } > } > - else if(!strncmp(argv[i], "srk_auth:", 8)) { > - if((rc = parse_auth_string(argv[i] + 8, vtpm_globals.srk_auth)) != 0) { > + else if(!strncmp(argv[i], "srk_auth=", 9)) { > + if((rc = parse_auth_string(argv[i] + 9, vtpm_globals.srk_auth)) != 0) { > goto err_invalid; > } > } >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > vtpm_handle_cmd doesn't ensure there is enough space before unpacking > the req buffer. Add a minimum size check. Called functions will have > to do their own checking if they need more data from the request. > > The error case is tricky since abort_egress wants to rely with a > corresponding tag. Just hardcode TPM_TAG_RQU_COMMAND since the vtpm is > sending in malformed commands in the first place. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/vtpmmgr/vtpm_cmd_handler.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c > index c879b24c13..5586be6997 100644 > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c > @@ -840,6 +840,12 @@ TPM_RESULT vtpmmgr_handle_cmd( > UINT32 size; > TPM_COMMAND_CODE ord; > > + if (tpmcmd->req_len < sizeof_TPM_RQU_HEADER(tpmcmd->req)) { > + status = TPM_BAD_PARAMETER; > + tag = TPM_TAG_RQU_COMMAND; > + goto abort_egress; > + } > + > unpack_TPM_RQU_HEADER(tpmcmd->req, > &tag, &size, &ord); > >
On 5/6/21 9:59 AM, Jason Andryuk wrote: > Add two patches: > vtpm-microsecond-duration.patch fixes the units for timeouts and command > durations. > vtpm-command-duration.patch increases the timeout linux uses to allow > commands to succeed. > > Linux works around low timeouts, but not low durations. The second > patch allows commands to complete that often timeout with the lower > command durations. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > stubdom/Makefile | 2 + > stubdom/vtpm-command-duration.patch | 52 +++++++++++++++++++++++++ > stubdom/vtpm-microsecond-duration.patch | 52 +++++++++++++++++++++++++ > 3 files changed, 106 insertions(+) > create mode 100644 stubdom/vtpm-command-duration.patch > create mode 100644 stubdom/vtpm-microsecond-duration.patch > > diff --git a/stubdom/Makefile b/stubdom/Makefile > index c6de5f68ae..06aa69d8bc 100644 > --- a/stubdom/Makefile > +++ b/stubdom/Makefile > @@ -239,6 +239,8 @@ tpm_emulator-$(XEN_TARGET_ARCH): tpm_emulator-$(TPMEMU_VERSION).tar.gz > patch -d $@ -p1 < vtpm-implicit-fallthrough.patch > patch -d $@ -p1 < vtpm_TPM_ChangeAuthAsymFinish.patch > patch -d $@ -p1 < vtpm_extern.patch > + patch -d $@ -p1 < vtpm-microsecond-duration.patch > + patch -d $@ -p1 < vtpm-command-duration.patch > mkdir $@/build > cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) -Wno-declaration-after-statement" > touch $@ > diff --git a/stubdom/vtpm-command-duration.patch b/stubdom/vtpm-command-duration.patch > new file mode 100644 > index 0000000000..6fdf2fc9be > --- /dev/null > +++ b/stubdom/vtpm-command-duration.patch > @@ -0,0 +1,52 @@ > +From e7c976b5864e7d2649292d90ea60d5aea091a990 Mon Sep 17 00:00:00 2001 > +From: Jason Andryuk <jandryuk@gmail.com> > +Date: Sun, 14 Mar 2021 12:46:34 -0400 > +Subject: [PATCH 2/2] Increase command durations > + > +Wth Linux 5.4 xen-tpmfront and a Xen vtpm-stubdom, xen-tpmfront was > +failing commands with -ETIME: > +tpm tpm0: tpm_try_transmit: send(): error-62 > + > +The vtpm was returning the data, but it was after the duration timeout > +in vtpm_send. Linux may have started being more stringent about timing? > + > +The vtpm-stubdom has a little delay since it writes its disk before > +returning the response. > + > +Anyway, the durations are rather low. When they were 1/10/1000 before > +converting to microseconds, Linux showed all three durations rounded to > +10000. Update them with values from a physical TPM1.2. These were > +taken from a WEC which was software downgraded from a TPM2 to a TPM1.2. > +They might be excessive, but I'd rather have a command succeed than > +return -ETIME. > + > +An IFX physical TPM1.2 uses: > +1000000 > +1500000 > +150000000 > + > +Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > +--- > + tpm/tpm_data.c | 6 +++--- > + 1 file changed, 3 insertions(+), 3 deletions(-) > + > +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c > +index bebaf10..844afca 100644 > +--- a/tpm/tpm_data.c > ++++ b/tpm/tpm_data.c > +@@ -71,9 +71,9 @@ static void init_timeouts(void) > + tpmData.permanent.data.tis_timeouts[1] = 2000000; > + tpmData.permanent.data.tis_timeouts[2] = 750000; > + tpmData.permanent.data.tis_timeouts[3] = 750000; > +- tpmData.permanent.data.cmd_durations[0] = 1000; > +- tpmData.permanent.data.cmd_durations[1] = 10000; > +- tpmData.permanent.data.cmd_durations[2] = 1000000; > ++ tpmData.permanent.data.cmd_durations[0] = 3000000; > ++ tpmData.permanent.data.cmd_durations[1] = 3000000; > ++ tpmData.permanent.data.cmd_durations[2] = 600000000; > + } > + > + void tpm_init_data(void) > +-- > +2.30.2 > + > diff --git a/stubdom/vtpm-microsecond-duration.patch b/stubdom/vtpm-microsecond-duration.patch > new file mode 100644 > index 0000000000..7a906e72c5 > --- /dev/null > +++ b/stubdom/vtpm-microsecond-duration.patch > @@ -0,0 +1,52 @@ > +From 5a510e0afd7c288e3f0fb3523ec749ba1366ad61 Mon Sep 17 00:00:00 2001 > +From: Jason Andryuk <jandryuk@gmail.com> > +Date: Sun, 14 Mar 2021 12:42:10 -0400 > +Subject: [PATCH 1/2] Use microseconds for timeouts and durations > + > +The timeout and duration fields should be in microseconds according to > +the spec. > + > +TPM_CAP_PROP_TIS_TIMEOUT: > +A 4 element array of UINT32 values each denoting the timeout value in > +microseconds for the following in this order: > + > +TPM_CAP_PROP_DURATION: > +A 3 element array of UINT32 values each denoting the duration value in > +microseconds of the duration of the three classes of commands: > + > +Linux will scale the timeouts up by 1000, but not the durations. Change > +the units for both sets as appropriate. > + > +Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > +--- > + tpm/tpm_data.c | 14 +++++++------- > + 1 file changed, 7 insertions(+), 7 deletions(-) > + > +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c > +index a3a79ef..bebaf10 100644 > +--- a/tpm/tpm_data.c > ++++ b/tpm/tpm_data.c > +@@ -67,13 +67,13 @@ static void init_nv_storage(void) > + static void init_timeouts(void) > + { > + /* for the timeouts we use the PC platform defaults */ > +- tpmData.permanent.data.tis_timeouts[0] = 750; > +- tpmData.permanent.data.tis_timeouts[1] = 2000; > +- tpmData.permanent.data.tis_timeouts[2] = 750; > +- tpmData.permanent.data.tis_timeouts[3] = 750; > +- tpmData.permanent.data.cmd_durations[0] = 1; > +- tpmData.permanent.data.cmd_durations[1] = 10; > +- tpmData.permanent.data.cmd_durations[2] = 1000; > ++ tpmData.permanent.data.tis_timeouts[0] = 750000; > ++ tpmData.permanent.data.tis_timeouts[1] = 2000000; > ++ tpmData.permanent.data.tis_timeouts[2] = 750000; > ++ tpmData.permanent.data.tis_timeouts[3] = 750000; > ++ tpmData.permanent.data.cmd_durations[0] = 1000; > ++ tpmData.permanent.data.cmd_durations[1] = 10000; > ++ tpmData.permanent.data.cmd_durations[2] = 1000000; > + } > + > + void tpm_init_data(void) > +-- > +2.30.2 > + >