Philipp Deppenwiese 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: -Code-Review
(2 comments)
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.
We are an Open Source project right? Sometimes people are busy, vacationing or vanish for a longer time. I wouldn't call it lack of willingness to communicate. I just have to prioritize my time and the coreboot community work is currently at the end of my list. So I can see that this raised some frustration on your and persmule side. Sorry for that.
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,
Not sure what you mean, we are only extending every measurement once. […]
So we missuse the tcpa log as PCR cache. IMHO we should try to initialize the TPM in bootblock itself. Probably with less code so it can fit on size constraint platforms. I guess Eltan already did the work for that on some platforms. The bootblock then can measure itself. What we have now is a so called PCR cache. If it stays like that, please rename the tcpa log function to pcr cache and move it into the crtm. It's obviously confusing that it has nothing todo with a TCPA log and it will lead to false assumptions again.
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;
This line is for the !CONFIG_VBOOT case where tpm_setup() is called in ramstage. […]
Ack