From 71a131415e2063d0e8414782b263e04bb5dc818b Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Wed, 6 May 2020 11:11:03 -0700 Subject: [PATCH] security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode This patch improves the response buffer handling for TPM 2.0. Previously we would allow any command to return no payload, but if there was a payload we would always try to unmarshal it according to the normal success response. This was sort of relying on the fact that the TPM usually returns no additional data after the header for error responses, but in practice that is not always true. It also means that commands without a response payload accidentally work by default even though we did not explicitly add unmarshallig support for them, which seems undesirable. Adding explicit unmarshalling support for TPM2_SelfTest which was only supported through this loophole before. This patch changes the behavior to always accept any amount of payload data for error responses but not unmarshal any of it. None of our use cases actually care about payload data for errors, so it seems safer to not even try to interpret it. For success responses, on the other hand, we always require support for the command to be explicitly added. This fixes a problem with the Cr50 GET_BOOT_MODE command where an error response would only return the subcommand code but no data after that. Also add support for a second, slightly different NO_SUCH_COMMAND error code that was added in Cr50 recently. Signed-off-by: Julius Werner Change-Id: Ib85032d85482d5484180be6fd105f2467f393cd2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41100 Reviewed-by: Vadim Bendebury Reviewed-by: Andrey Pronin Tested-by: build bot (Jenkins) --- src/security/tpm/tss/tcg-2.0/tss_marshaling.c | 18 ++++++++++++------ src/security/tpm/tss/vendor/cr50/cr50.c | 6 ++++-- src/security/tpm/tss/vendor/cr50/cr50.h | 1 + 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c index a229dd17ef..eff1acd2cd 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c +++ b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c @@ -587,17 +587,23 @@ struct tpm2_response *tpm_unmarshal_response(TPM_CC command, struct ibuf *ib) if (rc != 0) return NULL; - if (ibuf_remaining(ib) == 0) { - if (tpm2_static_resp.hdr.tpm_size != ibuf_nr_read(ib)) - printk(BIOS_ERR, - "%s: size mismatch in response to command %#x\n", - __func__, command); - return &tpm2_static_resp; + if (ibuf_capacity(ib) != tpm2_static_resp.hdr.tpm_size) { + printk(BIOS_ERR, + "%s: size mismatch in response to command %#x\n", + __func__, command); + return NULL; } + /* On errors, we're not sure what the TPM is returning. None of the + commands we use actually expect useful data payloads for errors, so + just ignore any data after the header. */ + if (tpm2_static_resp.hdr.tpm_code != TPM2_RC_SUCCESS) + return &tpm2_static_resp; + switch (command) { case TPM2_Startup: case TPM2_Shutdown: + case TPM2_SelfTest: break; case TPM2_GetCapability: diff --git a/src/security/tpm/tss/vendor/cr50/cr50.c b/src/security/tpm/tss/vendor/cr50/cr50.c index ae2f7c2516..d7bf48d711 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.c +++ b/src/security/tpm/tss/vendor/cr50/cr50.c @@ -89,7 +89,8 @@ uint32_t tlcl_cr50_get_tpm_mode(uint8_t *tpm_mode) return TPM_E_MUST_REBOOT; } - if (response->hdr.tpm_code == VENDOR_RC_NO_SUCH_COMMAND) { + if (response->hdr.tpm_code == VENDOR_RC_NO_SUCH_COMMAND || + response->hdr.tpm_code == VENDOR_RC_NO_SUCH_SUBCOMMAND) { /* * Explicitly inform caller when command is not supported */ @@ -119,7 +120,8 @@ uint32_t tlcl_cr50_get_boot_mode(uint8_t *boot_mode) if (!response) return TPM_E_IOERROR; - if (response->hdr.tpm_code == VENDOR_RC_NO_SUCH_COMMAND) + if (response->hdr.tpm_code == VENDOR_RC_NO_SUCH_COMMAND || + response->hdr.tpm_code == VENDOR_RC_NO_SUCH_SUBCOMMAND) /* Explicitly inform caller when command is not supported */ return TPM_E_NO_SUCH_COMMAND; diff --git a/src/security/tpm/tss/vendor/cr50/cr50.h b/src/security/tpm/tss/vendor/cr50/cr50.h index 0f91732856..e3146a421f 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.h +++ b/src/security/tpm/tss/vendor/cr50/cr50.h @@ -21,6 +21,7 @@ #define VENDOR_RC_ERR 0x00000500 enum cr50_vendor_rc { VENDOR_RC_INTERNAL_ERROR = (VENDOR_RC_ERR | 6), + VENDOR_RC_NO_SUCH_SUBCOMMAND = (VENDOR_RC_ERR | 8), VENDOR_RC_NO_SUCH_COMMAND = (VENDOR_RC_ERR | 127), };