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 5:
(14 comments)
Thanks! I think this fundamentally goes into the right direction, just needs a bit of tweaking here and there.
Philipp, do you have any fundamental concerns about this? I think separating measured boot from verified boot makes sense.
https://review.coreboot.org/c/coreboot/+/35077/5/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/35077/5/src/lib/cbfs.c@330 PS5, Line 330: #if !CONFIG(VBOOT) && CONFIG(VBOOT_MEASURED_BOOT) I'm not sure this prepare() callback is a good fit for this. First of all it only runs for programs (not other kinds of CBFS files), and secondly platforms can override cbfs_master_header_locator (currently done by soc/intel/apollolake).
Why not just put this at the end of bootblock_main()?
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/tpm/Makefile.i... File src/security/tpm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/tpm/Makefile.i... PS5, Line 10: CONFIG_VBOOT Just remove the conditional linking here (make it verstage-y and postcar-y) rather than making it more complicated. For normal C files there's no extra cost for linking more files, we can just link everything we have and let the garbage collector sort it out.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/tpm/tspi/tspi.... PS5, Line 25: #if CONFIG(VBOOT) || CONFIG(VBOOT_MEASURED_BOOT) We should make vboot library primitives (like hash functions) available unconditionally, so you should just take out the #if completely.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/tpm/tspi/tspi.... PS5, Line 246: #if CONFIG(VBOOT) || CONFIG(VBOOT_MEASURED_BOOT) ...here too.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Kconfig@... PS5, Line 16: menu "vboot functionalities" I think you should move all the measured boot stuff (i.e. the relevant options from here and vboot_crtm.c) out of the vboot directory, and rename everything so that there's no more "vboot" in the names. Maybe just put it in src/security/tpm/tspi/crtm.c? (The TCPA log stuff is already there too so I think that would make sense.)
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Kconfig@... PS5, Line 24: verified voot Please don't replace perfectly fine help text with typos. ;)
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... PS5, Line 66: bootblock-y += vboot_common.c I don't see why this would need to move?
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... PS5, Line 17: ifneq ($(CONFIG_VBOOT_MEASURED_BOOT)$(CONFIG_VBOOT),) Rather than building it for both vboot and measured boot, I think we should build and link the vboot library unconditionally. Only the actual verification stuff (e.g. the src/security/vboot/* files) should be conditional on CONFIG_VBOOT.
This change (building the library unconditionally) should probably be a separate patch, and then the patch factoring out measured boot on top of that.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... PS5, Line 20: verstage-y += tpm_common.c Hmmm... no, this is not good, if you need this file for measured boot then it shouldn't be in this directory. All the files in src/security/vboot/ should only get built when CONFIG_VBOOT is active.
What do you need from there, anyway? Neither of the functions in there are required for measured boot.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... PS5, Line 59: endif # CONFIG_VBOOT_MEASURED_BOOT || CONFIG_VBOOT This is where the ifneq ($(CONFIG_VBOOT),) should start. So everything above here should be built unconditionally, and everything below here should only be built for CONFIG_VBOOT.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... PS5, Line 61: ifeq ($(CONFIG_VBOOT_MEASURED_BOOT),y) This is the stuff that should go into another directory.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/Makefile... PS5, Line 69: ifeq ($(CONFIG_VBOOT),y) (Slightly off-topic, but this ifeq seems to be pointless because CONFIG_VBOOT should always be y here.)
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/misc.h@1... PS5, Line 130: static inline int vboot_crtm_is_set(void) Does this need to be externally accessible anyway? I think you can keep this local to crtm.c.
https://review.coreboot.org/c/coreboot/+/35077/5/src/security/vboot/misc.h@1... PS5, Line 139: __PRE_RAM__ We use if (ENV_ROMSTAGE_OR_BEFORE) for this kind of stuff now (see Kyösti's recent patches).