Change in coreboot[master]: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode
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), };
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size we shouldn't require this. some subcommands may decide to return more data on error
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 591: if (tpm2_static_resp.hdr.tpm_size != ibuf_nr_read(ib)) why not keep this check, just without the above line, will allow to avoid using the hardcoded header size?
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
we shouldn't require this. […]
If I understand this correctly this just checks that the header size field matches the packet size, which should be correct, payload or no payload.
But this could be checked before this function is invoked.
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
If I understand this correctly this just checks that the header size field matches the packet size, […]
No, it checks that the amount that we've read by this point is equal to the packet size. I.e. there are no additional payload bytes left after subcommand.
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 554: /* On errors, some subcommands do not return a payload. */ in fact, I'd even move this check before ibuf_read_be16(ib, &resp->vcr.vc_subcommand). if the RC says 'error', by default don't assume anything about the format or size of the payload. if we ever have subcommands, for which we are interested in parsing payloads in case of errors, we can add those special cases. but the default is: don't assume that the first two bytes is the suubcommand (or that you have 2 bytes) also.
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 591: if (tpm2_static_resp.hdr.tpm_size != ibuf_nr_read(ib))
why not keep this check, just without the above line, will allow to avoid using the hardcoded header […]
ah, I misunderstood what ibuf_nr_read(ib) does
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 594: (ibuf_remaining(ib) != tpm2_static_resp.hdr.tpm_size - 10) to avoid the hardcoded header size, how about the check
if ((ibuf_remaining(ib) + ibuf_nr_read(ib)) != tpm2_static_resp.hdr.tpm_size) { ... }
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
No, it checks that the amount that we've read by this point is equal to the packet size. I.e. […]
got it, but I think the size match should be checked before this function is invoked.
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
got it, but I think the size match should be checked before this function is invoked.
and the header return code should also be examined before this function is invoked, so there is no need to change the prototype to include the entire response instead of the VC payload.
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
and the header return code should also be examined before this function is invoked, so there is no n […]
examining it before the function is what I originally had in mind to minimize the patch.
but passing it here 1) keeps tpm_unmarshal_response generic as it should be, and encapsulates vendor-command-specific logic in this function - lower risk to miss this logic during some future change. 2) allows to add special cases (when we do want to get some info from the payload for vendor command error) there. though I suspect we never need those special cases.
bottomline: no strong preferences from my side. the choice is between a smaller patch and better separation of concerns.
another approach could be applying this logic (don't look at the payload for errors) to all commands, not just vendor commands. but that's a bigger change to the overall approach, and easier to break with future changes than this vendor-command-specific behavior for errors.
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
examining it before the function is what I originally had in mind to minimize the patch. […]
but calling unmarshal_vendor_command() is a separate case in the calling routine - I think it is perfectly logical to check the return code there and not proceed.
If and when we start caring about the payload we could change that, for now I'd rather keep the patch cleaner, and keep header and payload unmarshalling separate.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
but calling unmarshal_vendor_command() is a separate case in the calling routine - I think it is per […]
So the basic policy should be: whenever there's an error code, no matter what the command (vendor or standard), just accept any size but don't unmarshal anything?
Not sure I really like this "the TPM is allowed to return anything, anywhere" policy... but okay. For now as far as I can tell we never need to access any fields on errors, so we can do that.
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
So the basic policy should be: whenever there's an error code, no matter what the command (vendor or […]
- The proper size would be verified for all responses in line 594; - the 'do not care about details if there is an error return code' would apply only to the vendor command responses, the check would be added above line 631.
Hello build bot (Jenkins), Vadim Bendebury, Andrey Pronin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41100
to look at the new patch set (#2).
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
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.
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 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, 16 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41100/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
- The proper size would be verified for all responses in line 594; […]
Why would we make a difference between vendor commands and standard commands? I think vendor commands should follow the standard commands in how they are specified -- so either we say that the contents of the response payload are clearly specified for all commands (standard and vendor) and we can do some more specific processing like in this patch set, or we say that it's not exactly clear how the error response looks for any of them and we don't interpret it at all (like I did in Patch Set 2 now).
Hello build bot (Jenkins), Vadim Bendebury, Andrey Pronin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41100
to look at the new patch set (#3).
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
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.
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 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, 17 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41100/3
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... PS2, Line 590: - 10 I would still rather avoid using bare numbers, what is wrong with comparing the size of the packet with the header size field value?
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... PS2, Line 597: 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. sure, this is fine with me too, the policy could be applied across the board.
I was not sure if coreboot cared about error details for other commands, but come to think of it all we report in the log is the error code from the header.
Hello build bot (Jenkins), Vadim Bendebury, Andrey Pronin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41100
to look at the new patch set (#4).
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
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.
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 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, 18 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41100/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 591: if (tpm2_static_resp.hdr.tpm_size != ibuf_nr_read(ib))
ah, I misunderstood what ibuf_nr_read(ib) does
Ack
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 554: /* On errors, some subcommands do not return a payload. */
in fact, I'd even move this check before ibuf_read_be16(ib, &resp->vcr.vc_subcommand). […]
Ack
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
Why would we make a difference between vendor commands and standard commands? I think vendor command […]
Ack
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 594: (ibuf_remaining(ib) != tpm2_static_resp.hdr.tpm_size - 10)
to avoid the hardcoded header size, how about the check […]
Oh, missed this earlier... yeah, that's even better. Actually, looks like we have ibuf_capacity() to get that in one go.
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... PS2, Line 590: - 10
I would still rather avoid using bare numbers, what is wrong with comparing the size of the packet w […]
Done
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... PS2, Line 597: 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.
sure, this is fine with me too, the policy could be applied across the board. […]
Ack
Hello build bot (Jenkins), Vadim Bendebury, Andrey Pronin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41100
to look at the new patch set (#5).
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
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.
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 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, 17 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41100/5
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... PS5, Line 606: case TPM2_SelfTest: where does this come from?
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... PS5, Line 606: case TPM2_SelfTest:
where does this come from?
looks like it is because successful SelfTest response has empty payload and we were capturing it with `if (ibuf_remaining(ib) == 0)` case before. we (correctly) dropped the assumption that any empty payload response is ok (it's not for the success case), but now have to explicitly list all handled commands to avoid the error in the default case in this switch.
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... PS5, Line 606: case TPM2_SelfTest:
looks like it is because successful SelfTest response has empty payload and we were capturing it wit […]
ah,makes sense. Are there any other commands which would fall into this category?
Julius, please mention this addition in the patch description.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/5/src/security/tpm/tss/tcg-2.... PS5, Line 606: case TPM2_SelfTest:
ah,makes sense. Are there any other commands which would fall into this category? […]
Done
Hello build bot (Jenkins), Vadim Bendebury, Andrey Pronin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41100
to look at the new patch set (#6).
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
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 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, 17 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41100/6
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 6: Code-Review+2
do you want to build another image for Doug to test the most recent patch?
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
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 jwerner@chromium.org Change-Id: Ib85032d85482d5484180be6fd105f2467f393cd2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41100 Reviewed-by: Vadim Bendebury vbendeb@chromium.org Reviewed-by: Andrey Pronin apronin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- 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, 17 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Vadim Bendebury: Looks good to me, approved Andrey Pronin: Looks good to me, but someone else must approve
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..eff1acd 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 @@ 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 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), };
-
Andrey Pronin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Vadim Bendebury (Code Review)