Hello Furquan Shaikh, Patrick Georgi, Angel Pons, Arthur Heymans,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48482
to review the following change.
Change subject: cbfs: Allow mcache to be found after the first lookup ......................................................................
cbfs: Allow mcache to be found after the first lookup
This patch addresses the same problem as CB:48429, but hopefully this time correctly. Since the mcache is not guaranteed to be available on the first CBFS lookup for some special cases, we can no longer treat it as a one-time fire-and-forget initialization. Instead, we test cbd->mcache_size to check if the mcache has been initialized yet, and keep trying on every lookup if we don't find it the first time.
Since the mcache is a hard requirement for TOCTOU safety, also make it more clear in Kconfig that configurations known to do CBFS accesses before CBMEM init are incompatbile with that, and make sure we die() rather than do something unsafe if there's a case that Kconfig didn't catch.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I4e01e9a9905f7dcba14eaf05168495201ed5de60 --- M src/lib/Kconfig.cbfs_verification M src/lib/cbfs.c 2 files changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/48482/1
diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification index 3499345..4e2ed8c 100644 --- a/src/lib/Kconfig.cbfs_verification +++ b/src/lib/Kconfig.cbfs_verification @@ -19,6 +19,7 @@ depends on CBFS_VERIFICATION depends on !NO_FMAP_CACHE depends on !NO_CBFS_MCACHE + depends on !USE_OPTION_TABLE && !FSP_CAR # Known to access CBFS before CBMEM init help Work in progress. Not actually TOCTOU safe yet. Do not use.
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index d275505..a1b6338 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -27,7 +27,7 @@
size_t data_offset; cb_err_t err = CB_CBFS_CACHE_FULL; - if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM) + if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM && cbd->mcache_size) err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size, name, mdata, &data_offset); if (err == CB_CBFS_CACHE_FULL) { @@ -35,6 +35,8 @@ if (CONFIG(TOCTOU_SAFETY)) { if (ENV_SMM) /* Cannot provide TOCTOU safety for SMM */ dead_code(); + if (!cbd->mcache_size) + die("Cannot access CBFS TOCTOU-safely in " ENV_STRING " before CBMEM init!\n"); /* We can only reach this for the RW CBFS -- an mcache overflow in the RO CBFS would have been caught when building the mcache in cbfs_get_boot_device(). @@ -408,6 +410,9 @@ if (CONFIG(NO_CBFS_MCACHE) || ENV_SMM) return;
+ if (cbd->mcache_size) + return; + const struct cbmem_entry *entry; if (cbmem_possibly_online() && (entry = cbmem_entry_find(id))) { @@ -465,14 +470,17 @@ return rw; }
+ /* In rare cases post-RAM stages may run this before cbmem_initalize(), + so we can't lock in the result of find_mcache() and should keep + trying every time until an mcache is found. */ + cbfs_boot_device_find_mcache(&ro, CBMEM_ID_CBFS_RO_MCACHE); + if (region_device_sz(&ro.rdev)) return &ro;
if (fmap_locate_area_as_rdev("COREBOOT", &ro.rdev)) die("Cannot locate primary CBFS");
- cbfs_boot_device_find_mcache(&ro, CBMEM_ID_CBFS_RO_MCACHE); - if (ENV_INITIAL_STAGE) { cb_err_t err = cbfs_init_boot_device(&ro, metadata_hash_get()); if (err == CB_CBFS_HASH_MISMATCH)