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.

View Change

14 comments:

• File src/lib/cbfs.c:

  • Patch Set #5, 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()?

• File src/security/tpm/Makefile.inc:

  • Patch Set #5, 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.

• File src/security/tpm/tspi/tspi.c:

  • Patch Set #5, 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.

  • Patch Set #5, Line 246: #if CONFIG(VBOOT) || CONFIG(VBOOT_MEASURED_BOOT)

    ...here too.

• File src/security/vboot/Kconfig:

  • Patch Set #5, 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.)

  • Patch Set #5, Line 24: verified voot

    Please don't replace perfectly fine help text with typos. ;)

• File src/security/vboot/Makefile.inc:

  • Patch Set #5, Line 66: bootblock-y += vboot_common.c

    I don't see why this would need to move?

  • Patch Set #5, 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.

  • Patch Set #5, 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.

  • Patch Set #5, 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.

  • Patch Set #5, Line 61: ifeq ($(CONFIG_VBOOT_MEASURED_BOOT),y)

    This is the stuff that should go into another directory.

  • Patch Set #5, Line 69: ifeq ($(CONFIG_VBOOT),y)

    (Slightly off-topic, but this ifeq seems to be pointless because CONFIG_VBOOT should always be y here.)

• File src/security/vboot/misc.h:

  • Patch Set #5, 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.

  • Patch Set #5, Line 139: __PRE_RAM__

    We use if (ENV_ROMSTAGE_OR_BEFORE) for this kind of stuff now (see Kyösti's recent patches).

To view, visit change 35077. To unsubscribe, or for help writing mail filters, visit settings.