Attention is currently required from: Christian Walter, Filip Lewiński, Yu-Ping Wu.
Julius Werner has posted comments on this change by Filip Lewiński. ( https://review.coreboot.org/c/coreboot/+/82695?usp=email )
Change subject: security: Allow vboot when INTEL_TXT enabled ......................................................................
Patch Set 9:
(1 comment)
File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/82695/comment/7d545581_4796212c?usp... : PS5, Line 62: if (CONFIG(TPM_MEASURED_BOOT_INIT_BOOTBLOCK) && !CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) {
So it sounds like we're going with the "always have MEASURED_BOOT initialize the TPM and then just […]
Sorry, I'm still not really following to be honest. So you're worried about the rare edge cases where the vboot code that calls `tpm_setup()` does extra stuff if it doesn't return success? But I still don't see what that has to do with STARTS_IN_BOOTBLOCK. You'd should have that same problem for STARTS_IN_ROMSTAGE as well if you initialize the TPM early and skip the vboot code for it.
I don't think any of these cases are really worth worrying about. The ones where it returns TPM_CB_MUST_REBOOT should really have been handled in the non-vboot instances of `tpm_setup()` already anyway, they're only in vboot because vboot wants to keep state for these reboots to avoid an endless reboot loop if something is severely broken. In practice this only really happens when the TPM is misconfigured in a weird way and can be resolved with a manual reset anyway.
The other case sets a recovery reason in vboot, but that's not a big deal because it would go into recovery mode from not reading correct secdata anyway. The only difference that makes is the reason code. And the SETUP_HIBERNATE_ON_ERR thing I honestly don't remember what that's there for, but it was a rare bug on Chromeboxes so if you're not using a Chromebox it probably doesn't matter.
-
Julius Werner (Code Review)