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.