Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41120 )
Change subject: cbfs: Add verification for RO CBFS metadata hash ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/metadata_hash.h:
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... PS8, Line 14: hash above, which may be shorter than the compile-time size of struct vb2_hash! */ What's the reason for doing this as opposed to honoring the fixed length of the object itself?
https://review.coreboot.org/c/coreboot/+/41120/8/src/commonlib/bsd/include/c... PS8, Line 25: /* Must *not* appear anywhere else in coreboot code (other than the anchor). */ Wouldn't the act of verifying the magic value include the immediate value? Update: looks like we're not looking for this string in the code.
Maybe we should note that more explicitly. And if it's not supposed to be referenced anywhere in the code why provide a #define in a header file for people to obtain? For building purposes? vs runtime?
https://review.coreboot.org/c/coreboot/+/41120/8/src/include/metadata_hash.h File src/include/metadata_hash.h:
https://review.coreboot.org/c/coreboot/+/41120/8/src/include/metadata_hash.h... PS8, Line 12: struct vb2_hash *metadata_hash_get(void); could you please provide some commentary as to what this function would be used for and when?
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@36 PS8, Line 36: if (cbd != vboot_get_cbfs_boot_device()) { I'm confused by this check. This error is only true when !NO_CBFS_MCACHE as well as !recovery. I see the Kconfig dependencies, but the code would be clearer if !NO_CBFS_MCACHE as added here as well. Separately, cbfs_boot_lookup() has force_ro parameter so it's not clear to me this is a valid check w/o checking force_ro here too.
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/cbfs.c@471 PS8, Line 471: die("RO CBFS initialization error: %d", err); I'm still having a hard time grok'ing why it's ok to fill up the mcache and not fail for Chrome OS. It makes it really hard to deal w/ checking integrity of the files' contents when the metadata is only sitting on boot media which means you'll have to re-walk the whole cbfs for a missing file lookup in mcache.
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/fmap.c@37 PS8, Line 37: ENV_BOOTBLOCK ENV_INITIAL_STAGE?
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/metadata_hash.c File src/lib/metadata_hash.c:
https://review.coreboot.org/c/coreboot/+/41120/8/src/lib/metadata_hash.c@10 PS8, Line 10: struct metadata_hash_anchor metadata_hash_anchor = { why is this global? and not const?