Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35077 )
Change subject: security/vboot: Decouple measured boot from verified boot ......................................................................
Patch Set 72:
(2 comments)
I understand the approach of decopupling measured boot from vboot in order to support other use cases. What I don't understand is why we need to extend measurements from the TCPA log into the TPM. If the measurements are done before?
This is only about measurements that couldn't be written to the TPM at the time they were measured because the TPM was not up yet. The previous implementation just tried to manually reload and re-measure all those files after we set up the TPM. With this patch, we instead use the same code that runs after the TPM is up to measure them at the time they're loaded, we just don't write them to the TPM yet, and then we know at the time the TPM comes up that all those things we need to backfill are exactly what's currently in our TCPA log. It's just cleaner than trying to build that list manually, it covers more edge cases (e.g. for platforms that load other platform-specific files in the bootblock before running verstage), it will be more efficient once we get to the point where we really measure the same bytes we're loading (which I where I'm hoping to get CBFS eventually), it conveniently allows us to solve the !CONFIG_VBOOT case with the same code as well, and it still extends everything at effectively the same time and in the same order as the old implementation.
BTW it took me months to get my stuff merged in the past. I went on a vacation for a week and we don't have time to wait for another?
Come on, we've been trying to discuss this for over a month and there was never any response. Scroll up and you can see my earlier comments. I don't think anyone wants to rush anything through here, but if you just don't say *anything* that makes it impossible to make progress and you have to understand that it frustrates people. If you want to have some more time to look at this in detail just name a reasonable deadline and stick to it and I'm sure we can accommodate that. Nobody wants to have a fight over this, it's just the complete lack of willingness to communicate that sucks.
https://review.coreboot.org/c/coreboot/+/35077/72/src/security/tpm/tspi/log.... File src/security/tpm/tspi/log.c:
https://review.coreboot.org/c/coreboot/+/35077/72/src/security/tpm/tspi/log.... PS72, Line 115: int result = tlcl_extend(tce->pcr,
Please explain me why we need to extend measurements which are already extended into a TPM again by […]
Not sure what you mean, we are only extending every measurement once. Basically, there is always a time before the TPM is up and a time after the TPM is up. This function runs only once during boot, at the end of tpm_setup() (so for CONFIG_VBOOT that happens in verstage). At that point all hashes that were previously cached in the TCPA log get written out to the TPM. (Anything measured afterwards goes directly to the TPM and this function doesn't run again.)
So any file loaded before the TPM is up gets cached in the TCPA log and written out at the time the TPM comes up. For CONFIG_VBOOT, this is usually only the bootblock and verstage. Whereas the previous implementation would manually reload those stages from flash to measure them at verstage-time, this patch measures them at the time they're loaded, stores them in the TCPA log and writes them out during the verstage. So they still get written out at the same time as they used to.
https://review.coreboot.org/c/coreboot/+/35077/70/src/security/tpm/tspi/tspi... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/35077/70/src/security/tpm/tspi/tspi... PS70, Line 110: return tpm_is_setup;
The ramstage has its own init system so it's unclear if the tpm is set up..
This line is for the !CONFIG_VBOOT case where tpm_setup() is called in ramstage. In that case, this will return 0 before tpm_setup() is called and 1 after. All other (= earlier) stages will always return 0.
For the CONFIG_VBOOT case, it's always decided on line 107 and this line doesn't matter. for CONFIG_VBOOT all stages after verstage will return true from vboot_logic_executed().
I think this should behave correctly in all cases, do you have any specific concerns?