Change in ...coreboot[master]: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cb...
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32532
Change subject: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook()
vboot_measure_cbfs_hook() is included when CONFIG_VBOOT_MEASURED_BOOT is enabled, but this function is defined a 0 in vboot_crtm.h using ENV_
Use same ENV_ for vboot_measure_cbfs_hook() as used in vboot_crtm.h. is_runtime_data() is not used when vboot_measure_cbfs_hook() is disabled, so use same conditions for this function also.
BUG=NA TEST=Build Google Banon and Google Cyan
Change-Id: Ic62c18db09c119dfb85340a6b7f36bfd148aaa45 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/vboot/vboot_crtm.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32532/1
diff --git a/src/security/vboot/vboot_crtm.c b/src/security/vboot/vboot_crtm.c index e4266b2..199bab6 100644 --- a/src/security/vboot/vboot_crtm.c +++ b/src/security/vboot/vboot_crtm.c @@ -139,6 +139,7 @@ return VB2_SUCCESS; }
+#if !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM static bool is_runtime_data(const char *name) { const char *whitelist = CONFIG_VBOOT_MEASURED_BOOT_RUNTIME_DATA; @@ -193,3 +194,4 @@
return tpm_measure_region(&rdev, pcr_index, tcpa_metadata); } +#endif
Hello Aaron Durbin, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32532
to look at the new patch set (#2).
Change subject: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook()
vboot_measure_cbfs_hook() is included when CONFIG_VBOOT_MEASURED_BOOT is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
Use same ENV_ for vboot_measure_cbfs_hook() as used in vboot_crtm.h. is_runtime_data() is not used when vboot_measure_cbfs_hook() is disabled, so use same conditions for this function also.
BUG=NA TEST=Build Google Banon and Google Cyan
Change-Id: Ic62c18db09c119dfb85340a6b7f36bfd148aaa45 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/vboot/vboot_crtm.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32532/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32532/2/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/32532/2/src/security/vboot/vboot_crtm.c@142 PS2, Line 142: #if !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM Don't you also need CONFIG(VBOOT_MEASURED_BOOT) just like it is done in vboot_crtm.h?
In fact, can we just add a #define VBOOT_USE_MEASURE_CBFS_HOOK or something similar that defines the same check(CONFIG(VBOOT_MEASURED_BOOT) && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) and then use it at runtime where vboot_measure_cbfs_hook is called:
if (VBOOT_USE_MEASURE_CBFS_HOOK) if (vboot_measure_cbfs_hook(fh, name)) return -1;
Does that work?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32532/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32532/2//COMMIT_MSG@10 PS2, Line 10: is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_ Do we need all the ENV_ checks in the header? There's already a vboot_logic_executed() check in the function itself which should be compile-time decided to be false for those stages. We should try to reduce the tangle of #ifdefs where possible, so I'd say just remove the ENV_ checks from the header (it should only check for the Kconfig) and leave the function as is.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
Patch Set 2:
(2 comments)
Patch Set 2:
(1 comment)
I suggest using CONFIG() at function call: if (CONFIG(VBOOT_MEASURED_BOOT) if (vboot_measure_cbfs_hook(fh, name)) return -1;
and removing #if/#else/#endif condition in vboot_crtm.h
https://review.coreboot.org/#/c/32532/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32532/2//COMMIT_MSG@10 PS2, Line 10: is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
Do we need all the ENV_ checks in the header? There's already a vboot_logic_executed() check in the […]
One or the other way conditions in vboot_crtm.h and vboot_crtm.c should be in sync.
I could not find the info why defines for vboot_measure_cbfs_hook() are used.
https://review.coreboot.org/#/c/32532/2/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/32532/2/src/security/vboot/vboot_crtm.c@142 PS2, Line 142: #if !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM
Don't you also need CONFIG(VBOOT_MEASURED_BOOT) just like it is done in vboot_crtm.h? […]
The VBOOT_MEASURED_BOOT is already used in makefile.inc to include this file.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.c: Use ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32532/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32532/2//COMMIT_MSG@10 PS2, Line 10: is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
One or the other way conditions in vboot_crtm.h and vboot_crtm.c should be in sync. […]
vboot_crtm.c is currently guarded by the Kconfig via the Makefile, and nothing else. That's why I'm saying you should take the ENV_ checks out of the header, then they'll be in sync.
Yes, putting an explicit if (...) in the place where it's called would also work and comes out to the same. That's a matter of taste and both approaches are common in coreboot code. (Arguably, the way it's currently done with an empty stub in the header is more common for optional features like this, e.g. timestamps use the same thing. I think the point is to use up less lines in the calling code so that an optional feature doesn't dominate the main code flow that much.)
Hello Aaron Durbin, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32532
to look at the new patch set (#3).
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ conditions for vboot_measure_cbfs_hook() ......................................................................
security/vboot/vboot_crtm.h: Remove ENV_ conditions for vboot_measure_cbfs_hook()
vboot_measure_cbfs_hook() is included when CONFIG_VBOOT_MEASURED_BOOT is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
Remove ENV_ for vboot_measure_cbfs_hook() function definition.
BUG=NA TEST=Build Google Banon and Google Cyan
Change-Id: Ic62c18db09c119dfb85340a6b7f36bfd148aaa45 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/vboot/vboot_crtm.h 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32532/3
Hello Aaron Durbin, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32532
to look at the new patch set (#4).
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook()
vboot_measure_cbfs_hook() is included when CONFIG_VBOOT_MEASURED_BOOT is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
Remove ENV_ for vboot_measure_cbfs_hook() function definition.
BUG=NA TEST=Build Google Banon and Google Cyan
Change-Id: Ic62c18db09c119dfb85340a6b7f36bfd148aaa45 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/vboot/vboot_crtm.h 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32532/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 5: Code-Review+2
LGTM but I think you need to rebase to get Jenkins to re-test this.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 6:
Patch Set 5: Code-Review+2
LGTM but I think you need to rebase to get Jenkins to re-test this.
Did rebase, but got error on Google Aleena 'undefined reference to `vboot_measure_cbfs_hook' Will check.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 5: Code-Review+2
LGTM but I think you need to rebase to get Jenkins to re-test this.
Did rebase, but got error on Google Aleena 'undefined reference to `vboot_measure_cbfs_hook' Will check.
The error 'undefined vboot_measure_cbfs_hook' will occur when both VBOOT and VBOOT_MEASURED_BOOT configs are enabled. The function definition of vboot_measure_cbfs_hook() is enabled, but the voot_ctrm.c is not included in bootblock. (I did a quick test adding vboot_crtm.c for bootblock, but this result in new build errors missing support in bootblock)
It is possible to check which .config is used by JENKINS for patch set 5?
I suggest revert to patch set 1? (adding _ENV to .c file) This will solve the original build issue and having a working JENKINS.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 6:
The error 'undefined vboot_measure_cbfs_hook' will occur when both VBOOT and VBOOT_MEASURED_BOOT configs are enabled. The function definition of vboot_measure_cbfs_hook() is enabled, but the voot_ctrm.c is not included in bootblock. (I did a quick test adding vboot_crtm.c for bootblock, but this result in new build errors missing support in bootblock)
I think this is the right solution, but it turned out our dead code elimination wasn't quite as efficient as I hoped yet. Please rebase onto my three patches ending in CB:32716, I think then it should work.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 7:
You still need to add vboot_crtm.c to the bootblock.
Hello Aaron Durbin, Julius Werner, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32532
to look at the new patch set (#8).
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook()
vboot_measure_cbfs_hook() is included when CONFIG_VBOOT_MEASURED_BOOT is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
Remove ENV_ for vboot_measure_cbfs_hook() function definition. This function is added to bootblock stage also.
BUG=NA TEST=Build Google Banon and Google Cyan
Change-Id: Ic62c18db09c119dfb85340a6b7f36bfd148aaa45 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/vboot/Makefile.inc M src/security/vboot/vboot_crtm.h 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32532/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 8: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 8: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32532 )
Change subject: security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook() ......................................................................
security/vboot/vboot_crtm.h: Remove ENV_ for vboot_measure_cbfs_hook()
vboot_measure_cbfs_hook() is included when CONFIG_VBOOT_MEASURED_BOOT is enabled, but this function is defined as 0 in vboot_crtm.h using ENV_
Remove ENV_ for vboot_measure_cbfs_hook() function definition. This function is added to bootblock stage also.
BUG=NA TEST=Build Google Banon and Google Cyan
Change-Id: Ic62c18db09c119dfb85340a6b7f36bfd148aaa45 Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32532 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Makefile.inc M src/security/vboot/vboot_crtm.h 2 files changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Julius Werner: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 6d2096d..9ce724e 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -70,6 +70,7 @@ postcar-y += vboot_common.c
ifeq ($(CONFIG_VBOOT_MEASURED_BOOT),y) +bootblock-y += vboot_crtm.c verstage-y += vboot_crtm.c romstage-y += vboot_crtm.c ramstage-y += vboot_crtm.c diff --git a/src/security/vboot/vboot_crtm.h b/src/security/vboot/vboot_crtm.h index e675edb..64cb4f2 100644 --- a/src/security/vboot/vboot_crtm.h +++ b/src/security/vboot/vboot_crtm.h @@ -46,8 +46,7 @@ */ uint32_t vboot_init_crtm(void);
-#if (CONFIG(VBOOT_MEASURED_BOOT) && \ -!ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) +#if CONFIG(VBOOT_MEASURED_BOOT) /* * Measures cbfs data via hook (cbfs) * fh is the cbfs file handle to measure
-
Frans Hendriks (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)