Hello Vadim Bendebury, Andrey Pronin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41100
to review the following change.
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
security: tcg-2.0: Improve error response handling, fix Cr50 boot mode
This patch imrpoves the response buffer handling for some TPM 2.0 commands in error cases. Previously we would just allow any command to return no payload -- with this patch, only commands with errors are accepted without payload as a default, for other commands the command-specific unmarshalling code decides the expected response length. Also explicitly confirm that the amount of data received matches the response header. For vendor commands, we additionally allow errors to have an empty payload after the subcommand code.
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 jwerner@chromium.org Change-Id: Ib85032d85482d5484180be6fd105f2467f393cd2 --- M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h 3 files changed, 29 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41100/1
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 a229dd1..ce0ee5e 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c +++ b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c @@ -546,29 +546,33 @@ return 0; }
-static int unmarshal_vendor_command(struct ibuf *ib, - struct vendor_command_response *vcr) +static int unmarshal_vendor_command(struct ibuf *ib, struct tpm2_response *resp) { - if (ibuf_read_be16(ib, &vcr->vc_subcommand)) + if (ibuf_read_be16(ib, &resp->vcr.vc_subcommand)) return -1;
- switch (vcr->vc_subcommand) { + /* On errors, some subcommands do not return a payload. */ + if (resp->hdr.tpm_code != TPM2_RC_SUCCESS && + ibuf_nr_read(ib) == resp->hdr.tpm_size) + return 0; + + switch (resp->vcr.vc_subcommand) { case TPM2_CR50_SUB_CMD_IMMEDIATE_RESET: break; case TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS: break; case TPM2_CR50_SUB_CMD_TURN_UPDATE_ON: - return ibuf_read_be8(ib, &vcr->num_restored_headers); + return ibuf_read_be8(ib, &resp->vcr.num_restored_headers); case TPM2_CR50_SUB_CMD_GET_REC_BTN: - return ibuf_read_be8(ib, &vcr->recovery_button_state); + return ibuf_read_be8(ib, &resp->vcr.recovery_button_state); case TPM2_CR50_SUB_CMD_TPM_MODE: - return ibuf_read_be8(ib, &vcr->tpm_mode); + return ibuf_read_be8(ib, &resp->vcr.tpm_mode); case TPM2_CR50_SUB_CMD_GET_BOOT_MODE: - return ibuf_read_be8(ib, &vcr->boot_mode); + return ibuf_read_be8(ib, &resp->vcr.boot_mode); default: printk(BIOS_ERR, "%s:%d - unsupported vendor command %#04x!\n", - __func__, __LINE__, vcr->vc_subcommand); + __func__, __LINE__, resp->vcr.vc_subcommand); return -1; }
@@ -587,14 +591,18 @@ 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_remaining(ib) != tpm2_static_resp.hdr.tpm_size - 10) { + printk(BIOS_ERR, + "%s: size mismatch in response to command %#x\n", + __func__, command); + return NULL; }
+ /* On errors, some commands do not return a payload. */ + if (tpm2_static_resp.hdr.tpm_code != TPM2_RC_SUCCESS && + ibuf_remaining(ib) == 0) + return &tpm2_static_resp; + switch (command) { case TPM2_Startup: case TPM2_Shutdown: @@ -620,7 +628,7 @@ break;
case TPM2_CR50_VENDOR_COMMAND: - rc |= unmarshal_vendor_command(ib, &tpm2_static_resp.vcr); + rc |= unmarshal_vendor_command(ib, &tpm2_static_resp); break;
default: diff --git a/src/security/tpm/tss/vendor/cr50/cr50.c b/src/security/tpm/tss/vendor/cr50/cr50.c index ae2f7c2..d7bf48d 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.c +++ b/src/security/tpm/tss/vendor/cr50/cr50.c @@ -89,7 +89,8 @@ 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 @@ 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 0f91732..e3146a4 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), };