Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31260 )
Change subject: coreboot: check Cr50 PM mode on normal boot ......................................................................
Patch Set 2:
(22 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
This may be a matter of taste, but I think coreboot commits shouldn't contain too many Chrome OS-ism […]
Done
https://review.coreboot.org/#/c/31260/1//COMMIT_MSG@10 PS1, Line 10: to the OS
[remove]
Done
https://review.coreboot.org/#/c/31260/1//COMMIT_MSG@11 PS1, Line 11: key-ladder
key ladder
Done
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, a […]
Done
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 44: TPM_MODE_LOCKED_ENABLED
How about TPM_MODE_ENABLED, TPM_MODE_KEYLADDER_DISABLED and TPM_MODE_FULLY_DISABLED? (Then, again, a […]
The Cr50 TPM states returned by VENDOR_CC_TPM_MODE, are: 0 = TPM_MODE_ENABLED_TENTATIVE: TPM is enabled, and may be changed using "gsctool -m [enable|disable]" 1 = TPM_MODE_ENABLED: Entered by running "gsctool -m enable". TPM is enabled, but cannot be changed again by gsctool (command fails with VENDOR_RC_NOT_ALLOWED). 2 = TPM_MODE_DISABLED: Entered by running "gsctool -m disable". TPM is disabled and the key-ladder is disabled. No TPM communication from AP is possible in this state.
The TPM mode always reverts to TPM_MODE_ENABLED_TENTATIVE(0) on a Cr50 reboot, or if the TPM is reset (which occurs on an AP reboot). The key-ladder state is not changed by a TPM reset.
If the key-ladder is disabled, then VENDOR_CC_TPM_MODE returns with the error VENDOR_RC_INTERNAL_ERROR and does not provide the TPM mode back to the AP in the command response.
So on a normal boot the only two expected results for sending the VENDOR_CC_TPM_MODE command are: 1. Command completes successfully, TPM mode is TPM_MODE_ENABLED_TENTATIVE(0) 2. Command fails with VENDOR_RC_INTERNAL_ERROR
It shouldn't be possible for the TPM mode to be TPM_MODE_ENABLED(1) during boot. If "gsctool" is used on the previous boot to set the mode to TPM_MODE_ENABLED(1), the state will be cleared by the AP reboot/TPM reset. However if this state is found, we should reboot the Cr50 because it is an invalid state.
So it's probably best to set the enum names in coreboot to match the Cr50 firmware and not change the Cr50 firmware at this time. Ideally, we want to combine the TPM mode check with the existing TURN_UPDATE_ON command, but that change would need to wait until the 4.14 Cr50 firwmare release.
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 82: Returns
Return
Done
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 84: . On success,
, and
Done
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.
Do you mean that the actual reset occurs in a deferred call only after completing the TPM command?
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 94: Returns
Return
Done
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
Existing printk calls in this file used "cr50" - so I've changed those to to "Cr50" as well. Are there any scripts that parse the raw serial log that would be impacted by changing this?
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 84: TPM_E_INTERNAL_INCONSISTENCY
I think we generally like to use TPM_E_IOERROR for this. […]
Done
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 92: *cr50_must_reset = 1;
Why not just return this as TPM_E_MUST_REBOOT?
Done
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 93: response->hdr.tpm_code
Sure, that's fine as well.
I used TPM_E_NO_SUCH_COMMAND = 0x5010. Error code 0x5000-0x500F have all been used in vboot's tss_constants.h
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/el […]
Done
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 98: } else
else is not generally useful after a break or return
Done
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 101: return (TPM_SUCCESS);
No parens.
Done
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 119: return TPM_E_INTERNAL_INCONSISTENCY;
IOERROR
Done
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
Done
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".
Done
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 37: cr50_check_tpm
nit: I think cr50_reset_if_needed() or something like that would better encompass what this does whe […]
Done
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 48: Attempt
Please prefix all "bad" error strings with "ERROR: " for visibility.
I've done that with the errors in this file. Could printk() be enhanced to automatically add this if the msg_level is BIOS_ERR? This would ensure uniformity, and save some code space.
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 74:
nit: technically "is about to be reset", if it was already done we wouldn't be here anymore
Done