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