Keith Short has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31260
Change subject: coreboot: check TPM mode on normal boot ......................................................................
coreboot: check TPM mode on normal boot
When booting into Alt OS legacy mode, the TPM is disabled before handing off control to the OS. On a reboot back to Chrome OS, we must check the TPM mode. If TPM or key-ladder is disabled, trigger a reboot of the Cr50 to restore TPM functionality.
BUG=b:121463033 BRANCH=none TEST=Built depthcharge on sarien and grunt platforms. TEST=Ran 'gsctool -a -m disable' and reboot. Verfied coreboot sends VENDOR_CC_IMMEDIATE_RESET command to Cr50 and that the Cr50 resets and then the platform boots normally. Tested-by: Keith Short keithshort@chromium.org
Change-Id: I70e012efaf1079d43890e909bc6b5015bef6835a Signed-off-by: Keith Short keithshort@chromium.org --- M src/include/elog.h M src/mainboard/google/sarien/chromeos.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/cr50_enable_update.c 8 files changed, 203 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31260/1
diff --git a/src/include/elog.h b/src/include/elog.h index 31891e0..f1d5314 100644 --- a/src/include/elog.h +++ b/src/include/elog.h @@ -223,6 +223,9 @@ #define ELOG_SLEEP_PENDING_PM1_WAKE 0x01 #define ELOG_SLEEP_PENDING_GPE0_WAKE 0x02
+/* Cr50 reset to enable TPM */ +#define ELOG_TYPE_CR50_NEED_RESET 0xb2 + struct elog_event_extended_event { u8 event_type; u32 event_complement; diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index f9e42e0..9466124 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -116,7 +116,7 @@ return 1; }
-void mainboard_cr50_update_reset(void) +void mainboard_prepare_cr50_reset(void) { #if ENV_RAMSTAGE /* Ensure system powers up after CR50 reset */ 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 f1c5a37..991e9a6 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c +++ b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c @@ -266,6 +266,10 @@ uint16_t *sub_command = command_body;
switch (*sub_command) { + case TPM2_CR50_SUB_CMD_IMMEDIATE_RESET: + rc |= obuf_write_be16(ob, sub_command[0]); + rc |= obuf_write_be16(ob, sub_command[1]); + break; case TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS: rc |= obuf_write_be16(ob, *sub_command); break; @@ -276,6 +280,18 @@ case TPM2_CR50_SUB_CMD_GET_REC_BTN: rc |= obuf_write_be16(ob, *sub_command); break; + case TPM2_CR50_SUB_CMD_TPM_MODE: + /* The Cr50 TPM_MODE command supports an optional parameter. + * When the parameter is present the Cr50 will attempt to change + * the TPM state (enable or disable) and returns the new state + * in the response. When the parameter is absent, the Cr50 + * returns the current TPM state. + * + * Coreboot currently only uses the TPM get capability and does + * not set a new TPM state with the Cr50. + */ + rc |= obuf_write_be16(ob, *sub_command); + break; default: /* Unsupported subcommand. */ printk(BIOS_WARNING, "Unsupported cr50 subcommand: 0x%04x\n", @@ -471,12 +487,16 @@ return -1;
switch (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); case TPM2_CR50_SUB_CMD_GET_REC_BTN: return ibuf_read_be8(ib, &vcr->recovery_button_state); + case TPM2_CR50_SUB_CMD_TPM_MODE: + return ibuf_read_be8(ib, &vcr->tpm_mode); default: printk(BIOS_ERR, "%s:%d - unsupported vendor command %#04x!\n", diff --git a/src/security/tpm/tss/tcg-2.0/tss_structures.h b/src/security/tpm/tss/tcg-2.0/tss_structures.h index 6952169..991cbcf 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -298,6 +298,7 @@ union { uint8_t num_restored_headers; uint8_t recovery_button_state; + uint8_t tpm_mode; }; };
diff --git a/src/security/tpm/tss/vendor/cr50/cr50.c b/src/security/tpm/tss/vendor/cr50/cr50.c index 450ad97..97954db 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.c +++ b/src/security/tpm/tss/vendor/cr50/cr50.c @@ -68,3 +68,55 @@ *recovery_button_state = response->vcr.recovery_button_state; return TPM_SUCCESS; } + +uint32_t tlcl_cr50_get_tpm_mode(uint8_t *tpm_mode, int *cr50_must_reset) +{ + struct tpm2_response *response; + uint16_t mode_command = TPM2_CR50_SUB_CMD_TPM_MODE; + *cr50_must_reset = 0; + *tpm_mode = TPM_MODE_INVALID; + + printk(BIOS_INFO, "Reading cr50 TPM mode\n"); + + response = tpm_process_command(TPM2_CR50_VENDOR_COMMAND, &mode_command); + + if (!response) + return TPM_E_INTERNAL_INCONSISTENCY; + + if (response->hdr.tpm_code == VENDOR_RC_INTERNAL_ERROR) { + /* + * The Cr50 returns VENDOR_RC_INTERNAL_ERROR iff the key ladder + * is disabled. The Cr50 requires a reboot to re-enable the key + * ladder. + */ + *cr50_must_reset = 1; + } else if (response->hdr.tpm_code) { + /* Only other error expected is VENDOR_RC_NO_SUCH_SUBCOMMAND + * if communicating with a down rev Cr50 firmware. + */ + return TPM_E_INTERNAL_INCONSISTENCY; + } else + *tpm_mode = response->vcr.tpm_mode; + + return (TPM_SUCCESS); +} + +uint32_t tlcl_cr50_immediate_reset(uint16_t timeout_ms) +{ + struct tpm2_response *response; + uint16_t reset_command_body[] = { + TPM2_CR50_SUB_CMD_IMMEDIATE_RESET, timeout_ms + }; + + /* + * Issue an immediate reset to the Cr50. + */ + printk(BIOS_INFO, "Issuing Cr50 reset\n"); + response = tpm_process_command(TPM2_CR50_VENDOR_COMMAND, + &reset_command_body); + + if (!response) + return TPM_E_INTERNAL_INCONSISTENCY; + + return TPM_SUCCESS; +} diff --git a/src/security/tpm/tss/vendor/cr50/cr50.h b/src/security/tpm/tss/vendor/cr50/cr50.h index a1ab539..94b825f 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.h +++ b/src/security/tpm/tss/vendor/cr50/cr50.h @@ -23,9 +23,32 @@ to extending generically because the marshaling code is assuming all knowledge of all commands. */ #define TPM2_CR50_VENDOR_COMMAND ((TPM_CC)(TPM_CC_VENDOR_BIT_MASK | 0)) +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET (19) #define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS (21) #define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON (24) #define TPM2_CR50_SUB_CMD_GET_REC_BTN (29) +#define TPM2_CR50_SUB_CMD_TPM_MODE (40) + +/* Cr50 vendor-specific error codes. */ +#define VENDOR_RC_ERR 0x00000500 +enum cr50_vendor_rc { + VENDOR_RC_INTERNAL_ERROR = (VENDOR_RC_ERR | 6), + VENDOR_RC_NO_SUCH_COMMAND = (VENDOR_RC_ERR | 127), +}; + +enum cr50_tpm_mode { + /* TPM is enabled, and may be set to either ENABLED or DISABLED mode. */ + TPM_MODE_ENABLED = 0, + + /* TPM is enabled, and mode may not be changed. */ + TPM_MODE_LOCKED_ENABLED = 1, + + /* TPM is disabled, and mode may not be changed. */ + TPM_MODE_LOCKED_DISABLED = 2, + + TPM_MODE_INVALID, +}; +
/** * CR50 specific tpm command to enable nvmem commits before internal timeout @@ -53,4 +76,24 @@ */ uint32_t tlcl_cr50_get_recovery_button(uint8_t *recovery_button_state);
+/** + * CR50 specific TPM command sequence to query the current TPM mode. + * + * Returns value indicates success or failure of accessing the TPM; in case of + * success the cr50_must_reset output parameter indicates if the Cr50 must be + * reset to restore full capability. On success, the tpm_mode parameter is + * set to the current TPM mode. + */ +uint32_t tlcl_cr50_get_tpm_mode(uint8_t *tpm_mode, int *cr50_must_reset); + +/** + * CR50 specific TPM command sequence to trigger an immediate reset to the Cr50 + * device after the specified timeout in milliseconds. A timeout of zero means + * "IMMEDIATE REBOOT". + * + * Returns value indicates success or failure of accessing the TPM. + */ +uint32_t tlcl_cr50_immediate_reset(uint16_t timeout_ms); + + #endif /* CR50_TSS_STRUCTURES_H_ */ diff --git a/src/vendorcode/google/chromeos/chromeos.h b/src/vendorcode/google/chromeos/chromeos.h index f7e2ae9..6831261 100644 --- a/src/vendorcode/google/chromeos/chromeos.h +++ b/src/vendorcode/google/chromeos/chromeos.h @@ -33,8 +33,11 @@ static inline void reboot_from_watchdog(void) { return; } #endif /* CONFIG_CHROMEOS */
-/* Defined as weak function in cr50_enable_update.c */ -void mainboard_cr50_update_reset(void); +/** + * Perform any platform specific actions required prior to resetting the Cr50. + * Defined as weak function in cr50_enable_update.c + */ +void mainboard_prepare_cr50_reset(void);
struct romstage_handoff;
diff --git a/src/vendorcode/google/chromeos/cr50_enable_update.c b/src/vendorcode/google/chromeos/cr50_enable_update.c index da9a16d..31aad90 100644 --- a/src/vendorcode/google/chromeos/cr50_enable_update.c +++ b/src/vendorcode/google/chromeos/cr50_enable_update.c @@ -22,8 +22,58 @@ #include <vb2_api.h> #include <security/vboot/vboot_common.h> #include <vendorcode/google/chromeos/chromeos.h> +#include <delay.h>
-void __weak mainboard_cr50_update_reset(void) {} +#define C50_RESET_DELAY_MS 1000 + +void __weak mainboard_prepare_cr50_reset(void) {} + +/** + * Check of the Cr50 TPM state requires a chip reset of the Cr50 device. + * + * Returns 0 if the Cr50 TPM state is good (or cannot be determined). Returns 1 + * if the Cr50 was reset. + */ +static int cr50_check_tpm(uint16_t timeout_ms) +{ + int ret; + int cr50_must_reset = 0; + uint8_t tpm_mode; + + ret = tlcl_cr50_get_tpm_mode(&tpm_mode, &cr50_must_reset); + + if (ret != TPM_SUCCESS) { + /* TPM command failed, continue booting. */ + printk(BIOS_ERR, + "Attempt to get CR50 TPM mode failed: %x\n", + ret); + return 0; + } + + /* If the TPM mode has been locked, a Cr50 reset is required as vboot + * may need to disable the TPM. + */ + if (tpm_mode != TPM_MODE_ENABLED) { + cr50_must_reset = 1; + } + + /* If TPM state is okay, no reset needed. */ + if (!cr50_must_reset) + return 0; + + ret = tlcl_cr50_immediate_reset(timeout_ms); + + if (ret != TPM_SUCCESS) { + /* TPM command failed, continue booting. */ + printk(BIOS_ERR, + "Attempt to reset CR50 failed: %x\n", + ret); + return 0; + } + + /* Cr50 was reset successfully */ + return 1; +}
static void enable_update(void *unused) { @@ -43,7 +93,8 @@ }
/* Reboot in 1000 ms if necessary. */ - ret = tlcl_cr50_enable_update(1000, &num_restored_headers); + ret = tlcl_cr50_enable_update(C50_RESET_DELAY_MS, + &num_restored_headers);
if (ret != TPM_SUCCESS) { printk(BIOS_ERR, "Attempt to enable CR50 update failed: %x\n", @@ -51,20 +102,37 @@ return; }
- /* If no headers were restored there is no reset forthcoming. */ - if (!num_restored_headers) - return; + if (!num_restored_headers) { + /* If no headers were restored there is no reset forthcoming due + * to a Cr50 firmware update. Also check if the Cr50 TPM mode + * requires a reset. + * + * TODO: to eliminate a TPM command during every boot, the + * TURN_UPDATE_ON command could be enhanced/replaced in the Cr50 + * firmware to perform the TPM mode/key-ladder check in addition + * to the FW version check. + */ + + /* + * If the Cr50 was not reset, continue booting. + */ + if (!cr50_check_tpm(C50_RESET_DELAY_MS)) + return; + + printk(BIOS_INFO, "Waiting for CR50 reset to enable TPM.\n"); + elog_add_event(ELOG_TYPE_CR50_NEED_RESET); + } else { + printk(BIOS_INFO, + "Waiting for CR50 reset to pick up update.\n"); + elog_add_event(ELOG_TYPE_CR50_UPDATE); + }
/* Give mainboard a chance to take action */ - mainboard_cr50_update_reset(); - - elog_add_event(ELOG_TYPE_CR50_UPDATE); + mainboard_prepare_cr50_reset();
/* clear current post code avoid chatty eventlog on subsequent boot*/ post_code(0);
- printk(BIOS_INFO, "Waiting for CR50 reset to pick up update.\n"); - if (IS_ENABLED(CONFIG_POWER_OFF_ON_CR50_UPDATE)) poweroff(); halt();
build bot (Jenkins) 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:
(2 comments)
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 98: } else else is not generally useful after a break or return
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
Keith Short 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:
Original CL is here: crrev.com/c/1454885. This incorporates Julius' comments from that CL
Keith Short 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:
(3 comments)
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 15: Comment from Julius Werner on original CL:
Should probably rename this file now, maybe just chromeos/cr50.c
We could also consider moving the whole thing to something like src/security/tpm/tss/vendor/cr50/boot_hooks.c (that directory didn't exist yet when this was added but may be a better place, because really you want to run this even if you compile without CONFIG_CHROMEOS as long as your board has a Cr50).
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 84: if (vboot_recovery_mode_enabled()) Vadim - what's the rationale for skipping the Cr50 update check when recovery mode is enabled? Is this still needed? Should we always reboot the Cr50 if the key-ladder is disabled regardless of recovery mode?
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 123: elog_add_event(ELOG_TYPE_CR50_NEED_RESET); I have a separate CL that adds the ELOG_TYPE_CR50_NEED_RESET event type into mosys.
Vadim Bendebury 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:
(2 comments)
as a general comment: adding any tpm communication to the main boot flow adds something like 8 ms to the boot time, it should be avoided unless absolutely necessary.
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 84: if (vboot_recovery_mode_enabled())
Vadim - what's the rationale for skipping the Cr50 update check when recovery mode is enabled? Is t […]
I think recovery request could be lost if we reboot, we don't want this to happen.
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 96: tlcl_cr50_enable_update isn't this command goiong to timeout if tpm is disabled?
Keith Short 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:
(1 comment)
Patch Set 1:
(2 comments)
as a general comment: adding any tpm communication to the main boot flow adds something like 8 ms to the boot time, it should be avoided unless absolutely necessary.
The VENDOR_CC_TURN_UPDATE_ON command could be modified on the Cr50 side to also automatically check the key-ladder state and reboot itself. This would eliminate the extra TPM command. But this change can't be made until 4.14, and Wilco devices will need this support before then.
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 96: tlcl_cr50_enable_update
isn't this command goiong to timeout if tpm is disabled?
Before launching the alternate OS, TPM is disabled. However the AP reboot causes the Cr50 to re-enable TPM, but leave the key ladder disabled. So this command does complete normally.
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: .
Julius Werner 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:
(11 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
This may be a matter of taste, but I think coreboot commits shouldn't contain too many Chrome OS-isms anyway. This is a justification of why this patch is needed in coreboot, features we implement in our particular payload aren't really interesting here. I'd just say that Cr50 can be put into this special disabled state for certain use cases and needs an explicit reboot to get back out of it. (Also, clarify that this is only about Cr50, because people use coreboot with other TPMs.)
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
If we are changing the names here, can we also change them to match in the Cr50 source? Otherwise i […]
This still doesn't really seem to match what you said in CL:1446146? If the key ladder is still disabled when the TPM is in this mode, I wouldn't really call it "enabled". It more like a TPM_SOMEWHAT_ON_BUT_NOT_REALLY... (maybe TPM_PARTIALLY_ENABLED or TPM_NO_KEY_LADDER? or just TPM_NEEDS_RESET, because that's really what it's trying to tell us...)
Also, then later Joel said that this mode is actually never used atm (is that still true?), so maybe then calling this TPM_MODE_UNUSED or TPM_MODE_DEPRECATED would actually make most sense. We shouldn't be suggesting that another mode exists here when Cr50 can actually never enter that.
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 84: TPM_E_INTERNAL_INCONSISTENCY I think we generally like to use TPM_E_IOERROR for this. (I think INTERNAL_INCONSISTENCY refers more to a host-side internal inconsistency in the library, although it's not quite consistently used that way.)
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?
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. […]
Agreed. Maybe best to create a new TPM_E_UNSUPPORTED_COMMAND or something like that for this (in the 0x500X error space).
https://review.coreboot.org/#/c/31260/1/src/security/tpm/tss/vendor/cr50/cr5... PS1, Line 119: return TPM_E_INTERNAL_INCONSISTENCY; IOERROR
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 37: cr50_check_tpm nit: I think cr50_reset_if_needed() or something like that would better encompass what this does when you read it in the caller.
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.
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
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 84: if (vboot_recovery_mode_enabled())
I think recovery request could be lost if we reboot, we don't want this to happen.
This is a good point, and that would happen when we reboot due to TPM_MODE as well.
I think when we boot into manual recovery mode we know that H1 just cleanly rebooted (that's what it does when you press the magic key combo, right?), so in that case it should be guaranteed that we don't need to reboot again. When we boot into non-manual recovery mode, all we do is display a "Chrome OS is missing or damaged" screen, we don't really need the TPM for that. So I think it should be fine to just skip both reboot checks here when we're in recovery mode. (It would be nice to update the comment to detail that reasoning better.)
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 136: CONFIG_POWER_OFF_ON_CR50_UPDATE nit: may wanna rename to POWER_OFF_ON_CR50_RESET now
Aaron Durbin 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:
(1 comment)
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 84: if (vboot_recovery_mode_enabled())
This is a good point, and that would happen when we reboot due to TPM_MODE as well.
I think when we boot into manual recovery mode we know that H1 just cleanly rebooted (that's what it does when you press the magic key combo,
H1 does not reboot. EC_RST_L is asserted which takes down the AP side as well. H1 sees the AP reset signal and resets the TPM state, but the H1 itself does not reboot.
right?), so in that case it should be guaranteed that we don't need to reboot again. When we boot into non-manual recovery mode, all we do is display a "Chrome OS is missing or damaged" screen, we don't really need the TPM for that. So I think it should be fine to just skip both reboot checks here when we're in recovery mode. (It would be nice to update the comment to detail that reasoning better.)
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:
(2 comments)
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
This still doesn't really seem to match what you said in CL:1446146? If the key ladder is still disa […]
I'm still rooting for something like:
0 = I2C_ENABLED_KEY_LADDER_ENABLED 2 = I2C_ENABLED_KEY_LADDER_DISABLED If we change the Cr50 code to return I2C_ENABLED_KEY_LADDER_DISABLED after an AP reset, it describes the state a bit better than returning an internal error, or just DISABLED.
I suppose Cr50 can still use 1 = I2C_DISABLED_KEY_LADDER_DISABLED internally, but there would never be any way for it to communicate this state to the outside world.
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 93: response->hdr.tpm_code
Agreed. […]
I had originally used TPM_E_NO_SUCH_COMMAND = 0x500d to keep the wording consistent.
Vadim Bendebury 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:
(1 comment)
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
I'm still rooting for something like: […]
I have not been following this patch closely, but this should be TPM_ENABLED... as the interface could be either i2c or SPI.
Julius Werner 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:
(3 comments)
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
I have not been following this patch closely, but this should be TPM_ENABLED... […]
How about TPM_MODE_ENABLED, TPM_MODE_KEYLADDER_DISABLED and TPM_MODE_FULLY_DISABLED? (Then, again, as long as Cr50 is never actually using the middle state I don't think we need to bikeshed a name for it, just call it "reserved", "deprecated" or "unused".)
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 93: response->hdr.tpm_code
I had originally used TPM_E_NO_SUCH_COMMAND = 0x500d to keep the wording consistent.
Sure, that's fine as well.
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 84: if (vboot_recovery_mode_enabled())
This is a good point, and that would happen when we reboot due to TPM_MODE as well. […]
Oh, okay. So I guess then we have the options:
1. Don't care about running manual recovery mode with keyladder disabled 2. Reboot with preserving the recovery reason
1. should theoretically be okay with what our recovery images are doing right now, I think, since we don't really update TPM state from there. But we designed it so that we'll always have the option to update TPM state if we ever find that we need to, and even though that's never come up for now it's probably not something we wanna preclude for the future.
2. shouldn't be that hard either... there's already some stuff that's essentially doing this on x86 anyway, with the whole vb2_save_recovery_reason_vbnv/vb2_clear_recovery_reason_vbnv... although that's really messy and I'd prefer not to propagate it further (coreboot shouldn't muck with vboot's stuff behind its back that much). Maybe we can instead just re-evaluate how the recovery_request is handled in vboot in general... move the point where it gets cleared to the start of kernel verification and we can stop worrying about what happens when platforms want to reboot at random points within coreboot.
Hello Simon Glass, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31260
to look at the new patch set (#2).
Change subject: coreboot: check Cr50 PM mode on normal boot ......................................................................
coreboot: check Cr50 PM mode on normal boot
Under some scenearios the key ladder on the Cr50 can get disabled. If this state is dettected, trigger a reboot of the Cr50 to restore full TPM functionality.
BUG=b:121463033 BRANCH=none TEST=Built coreboot on sarien and grunt platforms. TEST=Ran 'gsctool -a -m disable' and reboot. Verfied coreboot sends VENDOR_CC_IMMEDIATE_RESET command to Cr50 and that the Cr50 resets and then the platform boots normally. TEST=Performed Cr50 rollback to 0.0.22 which does not support the VENDOR_CC_TPM_MODE command, confirmed that platform boots normally and the coreboot log captures the unsupported command. Tested-by: Keith Short keithshort@chromium.org
Change-Id: I70e012efaf1079d43890e909bc6b5015bef6835a Signed-off-by: Keith Short keithshort@chromium.org --- M src/include/elog.h M src/mainboard/google/sarien/chromeos.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h M src/security/tpm/tss_errors.h M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/cr50_enable_update.c 9 files changed, 244 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31260/2
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
Joel Kitching 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:
(3 comments)
https://review.coreboot.org/#/c/31260/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31260/2//COMMIT_MSG@10 PS2, Line 10: dettected detected
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 91: A timeout of zero means : * "IMMEDIATE REBOOT".
Do you mean that the actual reset occurs in a deferred call only after completing the TPM command?
Yeah, which means in theory it's not quite "immediate". Without providing an argument to IMMEDIATE_RESET, it will trigger without a deferred call.
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
Existing printk calls in this file used "cr50" - so I've changed those to to "Cr50" as well. […]
Ah - just match the file with lowercase then. We seem to have a mix of cr50, Cr50, CR50 throughout different code.
Hello Simon Glass, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31260
to look at the new patch set (#3).
Change subject: coreboot: check Cr50 PM mode on normal boot ......................................................................
coreboot: check Cr50 PM mode on normal boot
Under some scenearios the key ladder on the Cr50 can get disabled. If this state is detected, trigger a reboot of the Cr50 to restore full TPM functionality.
BUG=b:121463033 BRANCH=none TEST=Built coreboot on sarien and grunt platforms. TEST=Ran 'gsctool -a -m disable' and reboot. Verfied coreboot sends VENDOR_CC_IMMEDIATE_RESET command to Cr50 and that the Cr50 resets and then the platform boots normally. TEST=Performed Cr50 rollback to 0.0.22 which does not support the VENDOR_CC_TPM_MODE command, confirmed that platform boots normally and the coreboot log captures the unsupported command. Tested-by: Keith Short keithshort@chromium.org
Change-Id: I70e012efaf1079d43890e909bc6b5015bef6835a Signed-off-by: Keith Short keithshort@chromium.org --- M src/include/elog.h M src/mainboard/google/sarien/chromeos.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h M src/security/tpm/tss_errors.h M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/cr50_enable_update.c 9 files changed, 241 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31260/3
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 3:
(2 comments)
https://review.coreboot.org/#/c/31260/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31260/2//COMMIT_MSG@10 PS2, Line 10: dettected
detected
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
Ah - just match the file with lowercase then. […]
Done
Julius Werner 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 3:
(4 comments)
https://review.coreboot.org/#/c/31260/3/src/security/tpm/tss_errors.h File src/security/tpm/tss_errors.h:
https://review.coreboot.org/#/c/31260/3/src/security/tpm/tss_errors.h@45 PS3, Line 45: #define TPM_E_NO_SUCH_COMMAND ((uint32_t)0x00005010) nit: I think the train for keeping this in sync with the vboot version has long since left the station, so might as well use 500e here.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 69: cr50_must_reset = 1; If this is never supposed to happen under normal circumstances, you should print a warning here.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 96: if (vboot_recovery_mode_enabled()) I don't think the problem here is solved yet? We want to make sure we don't boot in recovery mode in a state where NVRAM changes won't be committed. Maybe that doesn't need to be solved in this CL, but it should be fixed before any board supporting this is shipped, so please at least file a bug about it.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 127: * to the FW version check. nit: Can you please file a bug for this and assign it to someone to make sure we won't forget it?
Hello Simon Glass, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31260
to look at the new patch set (#4).
Change subject: coreboot: check Cr50 PM mode on normal boot ......................................................................
coreboot: check Cr50 PM mode on normal boot
Under some scenarios the key ladder on the Cr50 can get disabled. If this state is detected, trigger a reboot of the Cr50 to restore full TPM functionality.
BUG=b:121463033 BRANCH=none TEST=Built coreboot on sarien and grunt platforms. TEST=Ran 'gsctool -a -m disable' and reboot. Verified coreboot sends VENDOR_CC_IMMEDIATE_RESET command to Cr50 and that the Cr50 resets and then the platform boots normally. TEST=Performed Cr50 rollback to 0.0.22 which does not support the VENDOR_CC_TPM_MODE command, confirmed that platform boots normally and the coreboot log captures the unsupported command. Tested-by: Keith Short keithshort@chromium.org
Change-Id: I70e012efaf1079d43890e909bc6b5015bef6835a Signed-off-by: Keith Short keithshort@chromium.org --- M src/include/elog.h M src/mainboard/google/sarien/chromeos.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h M src/security/tpm/tss_errors.h M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/cr50_enable_update.c 9 files changed, 247 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31260/4
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 4:
(7 comments)
https://review.coreboot.org/#/c/31260/3/src/security/tpm/tss_errors.h File src/security/tpm/tss_errors.h:
https://review.coreboot.org/#/c/31260/3/src/security/tpm/tss_errors.h@45 PS3, Line 45: #define TPM_E_NO_SUCH_COMMAND ((uint32_t)0x00005010)
nit: I think the train for keeping this in sync with the vboot version has long since left the stati […]
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 84: if (vboot_recovery_mode_enabled())
Oh, okay. So I guess then we have the options: […]
Filed bug http://b/124303781 and assigned to me.
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 96: tlcl_cr50_enable_update
Before launching the alternate OS, TPM is disabled. […]
Done
https://review.coreboot.org/#/c/31260/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 123: elog_add_event(ELOG_TYPE_CR50_NEED_RESET);
I have a separate CL that adds the ELOG_TYPE_CR50_NEED_RESET event type into mosys.
Done: https://chromium-review.googlesource.com/c/chromiumos/platform/mosys/+/14650...
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 69: cr50_must_reset = 1;
If this is never supposed to happen under normal circumstances, you should print a warning here.
Done
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 96: if (vboot_recovery_mode_enabled())
I don't think the problem here is solved yet? We want to make sure we don't boot in recovery mode in […]
Filed bug http://b/124303781 and assigned to me.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 127: * to the FW version check.
nit: Can you please file a bug for this and assign it to someone to make sure we won't forget it?
Opened bug http://b/124304447, and assigned to me.
Julius Werner 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 4: Code-Review+2
Thanks, I think this looks good now.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31260 )
Change subject: coreboot: check Cr50 PM mode on normal boot ......................................................................
coreboot: check Cr50 PM mode on normal boot
Under some scenarios the key ladder on the Cr50 can get disabled. If this state is detected, trigger a reboot of the Cr50 to restore full TPM functionality.
BUG=b:121463033 BRANCH=none TEST=Built coreboot on sarien and grunt platforms. TEST=Ran 'gsctool -a -m disable' and reboot. Verified coreboot sends VENDOR_CC_IMMEDIATE_RESET command to Cr50 and that the Cr50 resets and then the platform boots normally. TEST=Performed Cr50 rollback to 0.0.22 which does not support the VENDOR_CC_TPM_MODE command, confirmed that platform boots normally and the coreboot log captures the unsupported command. Tested-by: Keith Short keithshort@chromium.org
Change-Id: I70e012efaf1079d43890e909bc6b5015bef6835a Signed-off-by: Keith Short keithshort@chromium.org Reviewed-on: https://review.coreboot.org/c/31260 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/include/elog.h M src/mainboard/google/sarien/chromeos.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h M src/security/tpm/tss/vendor/cr50/cr50.c M src/security/tpm/tss/vendor/cr50/cr50.h M src/security/tpm/tss_errors.h M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/cr50_enable_update.c 9 files changed, 247 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/include/elog.h b/src/include/elog.h index 31891e0..f1d5314 100644 --- a/src/include/elog.h +++ b/src/include/elog.h @@ -223,6 +223,9 @@ #define ELOG_SLEEP_PENDING_PM1_WAKE 0x01 #define ELOG_SLEEP_PENDING_GPE0_WAKE 0x02
+/* Cr50 reset to enable TPM */ +#define ELOG_TYPE_CR50_NEED_RESET 0xb2 + struct elog_event_extended_event { u8 event_type; u32 event_complement; diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index 4cd6e16..1e363fd 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -115,7 +115,7 @@ return 1; }
-void mainboard_cr50_update_reset(void) +void mainboard_prepare_cr50_reset(void) { #if ENV_RAMSTAGE /* Ensure system powers up after CR50 reset */ 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 f1c5a37..62bc6a9 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c +++ b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c @@ -266,6 +266,14 @@ uint16_t *sub_command = command_body;
switch (*sub_command) { + case TPM2_CR50_SUB_CMD_IMMEDIATE_RESET: + /* The 16-bit timeout parameter is optional for the + * IMMEDIATE_RESET command. However in coreboot, the timeout + * parameter must be specified. + */ + rc |= obuf_write_be16(ob, sub_command[0]); + rc |= obuf_write_be16(ob, sub_command[1]); + break; case TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS: rc |= obuf_write_be16(ob, *sub_command); break; @@ -276,6 +284,18 @@ case TPM2_CR50_SUB_CMD_GET_REC_BTN: rc |= obuf_write_be16(ob, *sub_command); break; + case TPM2_CR50_SUB_CMD_TPM_MODE: + /* The Cr50 TPM_MODE command supports an optional parameter. + * When the parameter is present the Cr50 will attempt to change + * the TPM state (enable or disable) and returns the new state + * in the response. When the parameter is absent, the Cr50 + * returns the current TPM state. + * + * coreboot currently only uses the TPM get capability and does + * not set a new TPM state with the Cr50. + */ + rc |= obuf_write_be16(ob, *sub_command); + break; default: /* Unsupported subcommand. */ printk(BIOS_WARNING, "Unsupported cr50 subcommand: 0x%04x\n", @@ -471,12 +491,16 @@ return -1;
switch (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); case TPM2_CR50_SUB_CMD_GET_REC_BTN: return ibuf_read_be8(ib, &vcr->recovery_button_state); + case TPM2_CR50_SUB_CMD_TPM_MODE: + return ibuf_read_be8(ib, &vcr->tpm_mode); default: printk(BIOS_ERR, "%s:%d - unsupported vendor command %#04x!\n", diff --git a/src/security/tpm/tss/tcg-2.0/tss_structures.h b/src/security/tpm/tss/tcg-2.0/tss_structures.h index 6952169..991cbcf 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -298,6 +298,7 @@ union { uint8_t num_restored_headers; uint8_t recovery_button_state; + uint8_t tpm_mode; }; };
diff --git a/src/security/tpm/tss/vendor/cr50/cr50.c b/src/security/tpm/tss/vendor/cr50/cr50.c index 450ad97..1522ce6 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.c +++ b/src/security/tpm/tss/vendor/cr50/cr50.c @@ -26,7 +26,7 @@ if (response == NULL || (response && response->hdr.tpm_code)) { if (response) printk(BIOS_INFO, "%s: failed %x\n", __func__, - response->hdr.tpm_code); + response->hdr.tpm_code); else printk(BIOS_INFO, "%s: failed\n", __func__); return TPM_E_IOERROR; @@ -47,7 +47,7 @@ response = tpm_process_command(TPM2_CR50_VENDOR_COMMAND, command_body);
if (!response || response->hdr.tpm_code) - return TPM_E_INTERNAL_INCONSISTENCY; + return TPM_E_IOERROR;
*num_restored_headers = response->vcr.num_restored_headers; return TPM_SUCCESS; @@ -63,8 +63,67 @@ response = tpm_process_command(TPM2_CR50_VENDOR_COMMAND, &sub_command);
if (!response || response->hdr.tpm_code) - return TPM_E_INTERNAL_INCONSISTENCY; + return TPM_E_IOERROR;
*recovery_button_state = response->vcr.recovery_button_state; return TPM_SUCCESS; } + +uint32_t tlcl_cr50_get_tpm_mode(uint8_t *tpm_mode) +{ + struct tpm2_response *response; + uint16_t mode_command = TPM2_CR50_SUB_CMD_TPM_MODE; + *tpm_mode = TPM_MODE_INVALID; + + printk(BIOS_INFO, "Reading cr50 TPM mode\n"); + + response = tpm_process_command(TPM2_CR50_VENDOR_COMMAND, &mode_command); + + if (!response) + return TPM_E_IOERROR; + + if (response->hdr.tpm_code == VENDOR_RC_INTERNAL_ERROR) { + /* + * The Cr50 returns VENDOR_RC_INTERNAL_ERROR iff the key ladder + * is disabled. The Cr50 requires a reboot to re-enable the key + * ladder. + */ + return TPM_E_MUST_REBOOT; + } + + if (response->hdr.tpm_code == VENDOR_RC_NO_SUCH_COMMAND) { + /* + * Explicitly inform caller when command is not supported + */ + return TPM_E_NO_SUCH_COMMAND; + } + + if (response->hdr.tpm_code) { + /* Unexpected return code from Cr50 */ + return TPM_E_IOERROR; + } + + /* TPM command completed without error */ + *tpm_mode = response->vcr.tpm_mode; + + return TPM_SUCCESS; +} + +uint32_t tlcl_cr50_immediate_reset(uint16_t timeout_ms) +{ + struct tpm2_response *response; + uint16_t reset_command_body[] = { + TPM2_CR50_SUB_CMD_IMMEDIATE_RESET, timeout_ms}; + + /* + * Issue an immediate reset to the Cr50. + */ + printk(BIOS_INFO, "Issuing cr50 reset\n"); + response = tpm_process_command(TPM2_CR50_VENDOR_COMMAND, + &reset_command_body); + + if (!response) + return TPM_E_IOERROR; + + return TPM_SUCCESS; +} diff --git a/src/security/tpm/tss/vendor/cr50/cr50.h b/src/security/tpm/tss/vendor/cr50/cr50.h index a1ab539..6a160e0 100644 --- a/src/security/tpm/tss/vendor/cr50/cr50.h +++ b/src/security/tpm/tss/vendor/cr50/cr50.h @@ -23,9 +23,35 @@ to extending generically because the marshaling code is assuming all knowledge of all commands. */ #define TPM2_CR50_VENDOR_COMMAND ((TPM_CC)(TPM_CC_VENDOR_BIT_MASK | 0)) +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET (19) #define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS (21) #define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON (24) #define TPM2_CR50_SUB_CMD_GET_REC_BTN (29) +#define TPM2_CR50_SUB_CMD_TPM_MODE (40) + +/* Cr50 vendor-specific error codes. */ +#define VENDOR_RC_ERR 0x00000500 +enum cr50_vendor_rc { + VENDOR_RC_INTERNAL_ERROR = (VENDOR_RC_ERR | 6), + VENDOR_RC_NO_SUCH_COMMAND = (VENDOR_RC_ERR | 127), +}; + +enum cr50_tpm_mode { + /* + * Default state: TPM is enabled, and may be set to either + * TPM_MODE_ENABLED or TPM_MODE_DISABLED. + */ + TPM_MODE_ENABLED_TENTATIVE = 0, + + /* TPM is enabled, and mode may not be changed. */ + TPM_MODE_ENABLED = 1, + + /* TPM is disabled, and mode may not be changed. */ + TPM_MODE_DISABLED = 2, + + TPM_MODE_INVALID, +}; +
/** * CR50 specific tpm command to enable nvmem commits before internal timeout @@ -53,4 +79,26 @@ */ uint32_t tlcl_cr50_get_recovery_button(uint8_t *recovery_button_state);
+/** + * CR50 specific TPM command sequence to query the current TPM mode. + * + * Returns TPM_SUCCESS if TPM mode command completed, the Cr50 does not need a + * reboot, and the tpm_mode parameter is set to the current TPM mode. + * Returns TPM_E_MUST_REBOOT if TPM mode command completed, but the Cr50 + * requires a reboot. + * Returns TPM_E_NO_SUCH_COMMAND if the Cr50 does not support the command. + * Other returns value indicate a failure accessing the TPM. + */ +uint32_t tlcl_cr50_get_tpm_mode(uint8_t *tpm_mode); + +/** + * CR50 specific TPM command sequence to trigger an immediate reset to the Cr50 + * device after the specified timeout in milliseconds. A timeout of zero means + * "IMMEDIATE REBOOT". + * + * Return value indicates success or failure of accessing the TPM. + */ +uint32_t tlcl_cr50_immediate_reset(uint16_t timeout_ms); + + #endif /* CR50_TSS_STRUCTURES_H_ */ diff --git a/src/security/tpm/tss_errors.h b/src/security/tpm/tss_errors.h index 316661c..ed6fc3d 100644 --- a/src/security/tpm/tss_errors.h +++ b/src/security/tpm/tss_errors.h @@ -42,5 +42,6 @@ #define TPM_E_NV_DEFINED ((uint32_t)0x0000500b) /* vboot local */ #define TPM_E_INVALID_ARG ((uint32_t)0x0000500c) #define TPM_E_HASH_ERROR ((uint32_t)0x0000500d) +#define TPM_E_NO_SUCH_COMMAND ((uint32_t)0x0000500e)
#endif /* TSS_ERRORS_H_ */ diff --git a/src/vendorcode/google/chromeos/chromeos.h b/src/vendorcode/google/chromeos/chromeos.h index f7e2ae9..6831261 100644 --- a/src/vendorcode/google/chromeos/chromeos.h +++ b/src/vendorcode/google/chromeos/chromeos.h @@ -33,8 +33,11 @@ static inline void reboot_from_watchdog(void) { return; } #endif /* CONFIG_CHROMEOS */
-/* Defined as weak function in cr50_enable_update.c */ -void mainboard_cr50_update_reset(void); +/** + * Perform any platform specific actions required prior to resetting the Cr50. + * Defined as weak function in cr50_enable_update.c + */ +void mainboard_prepare_cr50_reset(void);
struct romstage_handoff;
diff --git a/src/vendorcode/google/chromeos/cr50_enable_update.c b/src/vendorcode/google/chromeos/cr50_enable_update.c index da9a16d..660fe2e 100644 --- a/src/vendorcode/google/chromeos/cr50_enable_update.c +++ b/src/vendorcode/google/chromeos/cr50_enable_update.c @@ -23,7 +23,75 @@ #include <security/vboot/vboot_common.h> #include <vendorcode/google/chromeos/chromeos.h>
-void __weak mainboard_cr50_update_reset(void) {} +#define C50_RESET_DELAY_MS 1000 + +void __weak mainboard_prepare_cr50_reset(void) {} + +/** + * Check if the Cr50 TPM state requires a chip reset of the Cr50 device. + * + * Returns 0 if the Cr50 TPM state is good or if the TPM_MODE command is + * unsupported. Returns 1 if the Cr50 was reset. + */ +static int cr50_reset_if_needed(uint16_t timeout_ms) +{ + int ret; + int cr50_must_reset = 0; + uint8_t tpm_mode; + + ret = tlcl_cr50_get_tpm_mode(&tpm_mode); + + if (ret == TPM_E_NO_SUCH_COMMAND) { + printk(BIOS_INFO, + "Cr50 does not support TPM mode command\n"); + /* Older Cr50 firmware, assume no Cr50 reset is required */ + return 0; + } + + if (ret == TPM_E_MUST_REBOOT) { + /* + * Cr50 indicated a reboot is required to restore TPM + * functionality. + */ + cr50_must_reset = 1; + } else if (ret != TPM_SUCCESS) { + /* TPM command failed, continue booting. */ + printk(BIOS_ERR, + "ERROR: Attempt to get CR50 TPM mode failed: %x\n", ret); + return 0; + } + + /* If the TPM mode is not enabled-tentative, then the TPM mode is locked + * and cannot be changed. Perform a Cr50 reset because vboot may need + * to disable TPM as part of booting an untrusted OS. + * + * This is not an expected state, as the Cr50 always sets the TPM mode + * to TPM_MODE_ENABLED_TENTATIVE during any TPM reset action. + */ + if (tpm_mode != TPM_MODE_ENABLED_TENTATIVE) { + printk(BIOS_NOTICE, + "NOTICE: Unexpected Cr50 TPM mode (%d). " + "A Cr50 reset is required.\n", tpm_mode); + cr50_must_reset = 1; + } + + /* If TPM state is okay, no reset needed. */ + if (!cr50_must_reset) + return 0; + + ret = tlcl_cr50_immediate_reset(timeout_ms); + + if (ret != TPM_SUCCESS) { + /* TPM command failed, continue booting. */ + printk(BIOS_ERR, + "ERROR: Attempt to reset CR50 failed: %x\n", + ret); + return 0; + } + + /* Cr50 is about to be reset, caller needs to prepare */ + return 1; +}
static void enable_update(void *unused) { @@ -37,34 +105,54 @@ ret = tlcl_lib_init();
if (ret != VB2_SUCCESS) { - printk(BIOS_ERR, "tlcl_lib_init() failed for CR50 update: %x\n", - ret); + printk(BIOS_ERR, + "ERROR: tlcl_lib_init() failed for CR50 update: %x\n", + ret); return; }
/* Reboot in 1000 ms if necessary. */ - ret = tlcl_cr50_enable_update(1000, &num_restored_headers); + ret = tlcl_cr50_enable_update(C50_RESET_DELAY_MS, + &num_restored_headers);
if (ret != TPM_SUCCESS) { - printk(BIOS_ERR, "Attempt to enable CR50 update failed: %x\n", - ret); + printk(BIOS_ERR, + "ERROR: Attempt to enable CR50 update failed: %x\n", + ret); return; }
- /* If no headers were restored there is no reset forthcoming. */ - if (!num_restored_headers) - return; + if (!num_restored_headers) { + /* If no headers were restored there is no reset forthcoming due + * to a Cr50 firmware update. Also check if the Cr50 TPM mode + * requires a reset. + * + * TODO: to eliminate a TPM command during every boot, the + * TURN_UPDATE_ON command could be enhanced/replaced in the Cr50 + * firmware to perform the TPM mode/key-ladder check in addition + * to the FW version check. + */ + + /* + * If the Cr50 was not reset, continue booting. + */ + if (!cr50_reset_if_needed(C50_RESET_DELAY_MS)) + return; + + printk(BIOS_INFO, "Waiting for CR50 reset to enable TPM.\n"); + elog_add_event(ELOG_TYPE_CR50_NEED_RESET); + } else { + printk(BIOS_INFO, + "Waiting for CR50 reset to pick up update.\n"); + elog_add_event(ELOG_TYPE_CR50_UPDATE); + }
/* Give mainboard a chance to take action */ - mainboard_cr50_update_reset(); - - elog_add_event(ELOG_TYPE_CR50_UPDATE); + mainboard_prepare_cr50_reset();
/* clear current post code avoid chatty eventlog on subsequent boot*/ post_code(0);
- printk(BIOS_INFO, "Waiting for CR50 reset to pick up update.\n"); - if (IS_ENABLED(CONFIG_POWER_OFF_ON_CR50_UPDATE)) poweroff(); halt();
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Keith Short (Code Review)
-
Patrick Georgi (Code Review)
-
Vadim Bendebury (Code Review)