Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29547 )
Change subject: security/vboot: Add measured boot mode ......................................................................
Patch Set 70:
(2 comments)
Patch Set 70:
(2 comments)
LGTM from a code perspective now, but the whitespace and line length issues still need to be fixed. If you want to fixup whitespace that's fine, but please do it in a separate CL. Do not mix feature changes with so many unrelated cleanups.
The problem Philipp faces here is the decision of using clang-format. We made this decision some time ago and he is just following. With switching to clang-format you will one day hit exactly this case: One touches some code and have clang-format active. Now clang-format steps in and touches some formatting. One can either separate this now into a "code-only" change and a "this is what clang-format has done" change but is it something we really want? What we see here should be visible more and more in the future and is just the result of the decision enabling clang-format.
Back to the patch itself: We have tested the latest patch set (70) on mc_bdx1 and mc_apl5 and it behaves as intended. I vote for bring this patch in. We all know that this will not be the final state of this experimental feature, it will be improved. And this can and will be done in follow-up patches. Having this patch for further several month in this state does only increase the effort of maintain it.
https://review.coreboot.org/#/c/29547/70/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/29547/70/src/lib/prog_loaders.c@77 PS70, Line 77: Is this really what clang-format gives you? Looks weird as there is the same space left in the upper line.
https://review.coreboot.org/#/c/29547/70/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/70/src/security/vboot/vboot_crtm.c@135 PS70, Line 135: pcr_index = TPM_CRTM_PCR;
Actually... […]
I think the intention was to provide a way to separate execution code (stages, payload, fsp, microcode_blob, ...) and "other" data measurements into different PCRs. For example, Siemens boards use a dataset in cbfs which contains logistic data of the mainboard. One can think of a case where a part of this dataset needs to be updated in the field. If we would have this dataset measured into PCR2, then this update would lead to an inconsistent TPM state or would require pre-attestation in order to have a trustworthy state after the update. While this would make sense for code updates it does not make sense for this mentioned dataset.
-
Werner Zeh (Code Review)