Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31260 )
Change subject: coreboot: check TPM mode on normal boot ......................................................................
Patch Set 1:
(17 comments)
https://review.coreboot.org/#/c/31260/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31260/1//COMMIT_MSG@9 PS1, Line 9: into Alt OS legacy mode an untrusted OS or UEFI binary
https://review.coreboot.org/#/c/31260/1//COMMIT_MSG@10 PS1, Line 10: to the OS [remove]
https://review.coreboot.org/#/c/31260/1//COMMIT_MSG@11 PS1, Line 11: key-ladder key ladder
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/tcg-2.0/tss_mar... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/tcg-2.0/tss_mar... PS1, Line 269: case TPM2_CR50_SUB_CMD_IMMEDIATE_RESET: We may want to also include a comment here about how IMMEDIATE_RESET's parameter is also optional, and how we are not supporting the calling the command with no parameter.
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... File src/security/tpm/tss/vendor/cr50/cr50.h:
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 40: ENABLED or DISABLED LOCKED_?
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 44: TPM_MODE_LOCKED_ENABLED If we are changing the names here, can we also change them to match in the Cr50 source? Otherwise it's going to get confusing pretty fast.
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 82: Returns Return
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 84: . On success, , and
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 91: A timeout of zero means : * "IMMEDIATE REBOOT". Sort of, but not really. See the Cr50 source of the immediate_reset command.
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 94: Returns Return
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... File src/security/tpm/tss/vendor/cr50/cr50.c:
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 79: cr50 Cr50
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 93: response->hdr.tpm_code I think it would be safer to specifically check the VENDOR_RC_NO_SUCH_COMMAND value here. If there is ever any other error value returned by the TPM, it should not be taken to mean "no such command" and allow the system to continue booting as normal.
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 98: else Even on a one-line statement, include brackets if there are brackets used before in an if/else if/else.
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 101: return (TPM_SUCCESS);
return is not a function, parentheses are not required
No parens.
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 32: of if
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 34: cannot be determined I think this should only be "if the command is unsupported".
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 74: .