Julius Werner 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 9:
(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?
The length of struct vb2_hash (and the value of VB2_MAX_DIGEST_SIZE) may change based on whether any `#define VB2_SUPPORT_<algo>` macros are defined when including <vb2_sha.h> (see https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs...), and I expect it might change in the future if support for different algorithms with longer hashes is added, so I am careful to not rely on it in any serialized structure. I'll expand this comment a bit to make that clearer.
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 […]
I do use it in src/lib/metadata_hash.c to define the anchor, it's just important that it not be used anywhere else in coreboot. It's used in cbfstool to find the anchor (that's why I'm defining it here in commonlib so that both can use the same definition).
I expanded the comment a bit and put DO_NOT_USE in the name to make it more visible.
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?
Done
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. […]
TOCTOU_SAFETY implies !NO_CBFS_MCACHE via Kconfig so I'm not checking it explicitly again.
This check is basically a "are we looking at the RO CBFS" check -- I know it looks a bit weird but that was the best way I found to do it (I can't just look at force_ro, because even for !force_ro we may still be looking at the RO CBFS in early stages).
When we are looking at the RO CBFS and the cache is full, then we cannot continue because we may not be in the bootblock anymore and other stages do not have access to the metadata hash anchor. I basically decided to force TOCTOU_SAFETY to depend on !NO_CBFS_MCACHE (and !NO_FMAP_CACHE) so I could avoid the problem of having to carry the metadata anchor itself forward into other stages. I think that's okay because the RO mcache either overflows right away or not at all -- you don't have that problem that it may overflow later with a post-ship update, so I'm basically saying people who need TOCTOU safety have to ensure their RO cache is big enough from the get go. (For RW we won't have that problem because the RW metadata hash will be part of vb2_context and available in all RW stages. See also the help text for the TOCTOU_SAFETY option in Kconfig.)
Actually, I think I can also just check this right when the mcache is built (where you put your other comment), maybe that's cleaner?
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. […]
See above -- if TOCTOU_SAFETY is enabled, then an overflow of the RO mcache is basically a fatal error. Moving that check down here now.
For configurations without TOCTOU_SAFETY (and we don't intend to use it in Chrome OS), the metadata only needs to be checked once. cbfs_mcache_build() still checks the whole metadata even if the cache was full halfway through, and it prioritizes CB_CBFS_HASH_MISMATCH over CB_CBFS_CACHE_FULL -- so if we get here and we get CACHE_FULL, we know the metadata for the whole CBFS is valid. Since we don't care about TOCTOU attacks we can later during the same boot just re-read it and trust it if we're trying to look up a file beyond what we fit in our cache. (Note that you don't exactly rewalk the whole CBFS in that case, just up to the file you're trying to find, since you're not checking the metadata hash again at that time -- and cbfs_walk() is smart enough to abort early and to avoid reading extra attribute data in that case.)
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?
Right, thanks.
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?
The const goes back to the discussion in https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... -- the argument to cbfs_walk() cannot be const, so if I made this struct const I'd have to cast it away at some point.
With static I think I was worried that might interact weirdly with the __attribute__((section)) somehow, but when trying it out now it seem to work fine. So okay, added.