Change in coreboot[master]: cbfs: Move more stuff into cbfs_boot_lookup()

[Julius Werner (Code Review)]

Hello Aaron Durbin,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/39327

to review the following change.

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
---
M src/lib/cbfs.c
M src/security/vboot/vboot_crtm.c
M src/security/vboot/vboot_crtm.h
3 files changed, 34 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/39327/1

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 39db97a..ef29299 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -31,40 +31,47 @@
 #include <security/vboot/vboot_crtm.h>
 #include <security/vboot/vboot_common.h>

-static cb_err_t cbfs_boot_lookup(const struct cbfs_boot_device *cbd,
-		const char *name, union cbfs_mdata *mdata, size_t *data_offset)
+static cb_err_t cbfs_boot_lookup(struct region_device *rdev, const char *name,
+		union cbfs_mdata *mdata, size_t *data_offset, bool force_ro)
 {
+	cb_err_t err;
+	const struct cbfs_boot_device *cbd = cbfs_get_boot_device(force_ro);
+	if (!cbd || rdev_chain_full(rdev, &cbd->rdev))
+		return CB_ERR;
+
    if (CONFIG(NO_CBFS_MCACHE))
-		return cbfs_lookup(&cbd->rdev, name, mdata, data_offset, NULL);
+		err = cbfs_lookup(rdev, name, mdata, data_offset, NULL);
    else
-		return cbfs_mcache_lookup(&cbd->rdev,
-					  cbd->mcache, cbd->mcache_size,
-					  name, mdata, data_offset, NULL);
+		err = cbfs_mcache_lookup(rdev, cbd->mcache, cbd->mcache_size,
+					 name, mdata, data_offset, NULL);
+
+	if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro &&
+	    err == CB_CBFS_NOT_FOUND) {
+		printk(BIOS_INFO, "CBFS: Fall back to RO region for %s\n",
+		       name);
+		return cbfs_boot_lookup(rdev, name, mdata, data_offset, true);
+	}
+	if (err)
+		return err;
+
+	if (vboot_measure_cbfs_hook(rdev, name, be32toh(mdata->h.type)))
+		return CB_ERR;
+
+	return CB_SUCCESS;
 }

int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type)
 {
-	const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false);
-	if (!cbd)
-		return -1;
-
+	struct region_device rdev;
    union cbfs_mdata mdata;
    size_t data_offset;
-	cb_err_t err = cbfs_boot_lookup(cbd, name, &mdata, &data_offset);

-	if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && err == CB_CBFS_NOT_FOUND) {
-		printk(BIOS_INFO, "CBFS: Fall back to RO region for %s\n",
-		       name);
-		if (!(cbd = cbfs_get_boot_device(true)))
-			return -1;
-		err = cbfs_boot_lookup(cbd, name, &mdata, &data_offset);
-	}
-	if (err)
+	if (cbfs_boot_lookup(&rdev, name, &mdata, &data_offset, false))
    	return -1;

size_t msize = be32toh(mdata.h.offset);
-	if (rdev_chain(&fh->metadata, &cbd->rdev, data_offset - msize, msize) ||
-	    rdev_chain(&fh->data, &cbd->rdev, data_offset, be32toh(mdata.h.len)))
+	if (rdev_chain(&fh->metadata, &rdev, data_offset - msize, msize) ||
+	    rdev_chain(&fh->data, &rdev, data_offset, be32toh(mdata.h.len)))
    	return -1;
    if (type) {
    	if (!*type)
@@ -73,9 +80,6 @@
    		return -1;
    }

-	if (vboot_measure_cbfs_hook(fh, name))
-		return -1;
-
    return 0;
 }

diff --git a/src/security/vboot/vboot_crtm.c b/src/security/vboot/vboot_crtm.c
index f68ab0a4..75c5662 100644
--- a/src/security/vboot/vboot_crtm.c
+++ b/src/security/vboot/vboot_crtm.c
@@ -157,19 +157,16 @@
    return false;
 }

-uint32_t vboot_measure_cbfs_hook(struct cbfsf *fh, const char *name)
+uint32_t vboot_measure_cbfs_hook(struct region_device *rdev, const char *name,
+				 uint32_t cbfs_type)
 {
    uint32_t pcr_index;
-	uint32_t cbfs_type;
    struct region_device rdev;
    char tcpa_metadata[TCPA_PCR_HASH_NAME];

if (!vboot_logic_executed())
    	return 0;

-	cbfsf_file_type(fh, &cbfs_type);
-	cbfs_file_data(&rdev, fh);
-
    switch (cbfs_type) {
    case CBFS_TYPE_MRC:
    case CBFS_TYPE_MRC_CACHE:
diff --git a/src/security/vboot/vboot_crtm.h b/src/security/vboot/vboot_crtm.h
index 64cb4f2..8e6c076 100644
--- a/src/security/vboot/vboot_crtm.h
+++ b/src/security/vboot/vboot_crtm.h
@@ -49,13 +49,14 @@
 #if CONFIG(VBOOT_MEASURED_BOOT)
 /*
  * Measures cbfs data via hook (cbfs)
- * fh is the cbfs file handle to measure
+ * rdev covers the file data (not metadata)
  * return 0 if successful, else an error
  */
-uint32_t vboot_measure_cbfs_hook(struct cbfsf *fh, const char *name);
+uint32_t vboot_measure_cbfs_hook(struct region_device *rdev, const char *name,
+				 uint32_t cbfs_type);

#else
-#define vboot_measure_cbfs_hook(fh, name) 0
+#define vboot_measure_cbfs_hook(rdev, name, cbfs_type) 0
 #endif

#endif /* __VBOOT_VBOOT_CRTM_H__ */

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39327

to look at the new patch set (#2).

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
---
M src/lib/cbfs.c
M src/security/vboot/vboot_crtm.c
M src/security/vboot/vboot_crtm.h
3 files changed, 39 insertions(+), 33 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/39327/2

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39327

to look at the new patch set (#3).

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
---
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
3 files changed, 42 insertions(+), 34 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/39327/3

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@37 
PS3, Line 37: 	    err == CB_CBFS_NOT_FOUND) {
same line like before?

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@62 
PS3, Line 62: 	const struct region_device *bootdev = boot_device_ro();
Why is this boot_device_ro()? Below you are using rdev_relative_offset() with bootdev and the result of cbfs_boot_lookup() where cbfs_get_boot_device() may return something different. You'll need to match those by passing in &bootdev to cbfs_boot_lookup() it can properly fill out the pointer.

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.h:

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
PS3, Line 48: struct region_device *rde
const

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
PS3, Line 109: rdev
variable naming conflict.

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39327

to look at the new patch set (#4).

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Also export the function externally. Since it is a low-level API and
most code should use the higher-level loading or mapping functions
instead, put it into a new <cbfs_private.h> to raise the mental barrier
for using this API (this will make more sense once cbfs_boot_locate() is
removed from <cbfs.h>).

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
---
M src/include/boot_device.h
M src/include/cbfs.h
A src/include/cbfs_private.h
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
6 files changed, 76 insertions(+), 39 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/39327/4

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 4:

(4 comments)

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@37 
PS3, Line 37: 	    err == CB_CBFS_NOT_FOUND) {
> same line like before?

I honestly don't know what the deal is with the line lengths right now... when it was voted on they said they would reformat the whole codebase to 96 and now a year or whatever it was later we're still stuck on this ugly mix of mostly 80 and a little 96 everywhere. :(

Anyway, for now my policy has always been to keep changes to files at 80 where the rest of the file is still at 80 for consistency, and write whole new files with 96. Not sure what everyone else is doing anymore.

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@62 
PS3, Line 62: 	const struct region_device *bootdev = boot_device_ro();
> Why is this boot_device_ro()? Below you are using rdev_relative_offset() with bootdev and the result […]

I'm using the assumption here that boot_device_ro() is always the root rdev for anything cbfs_get_boot_device() could return. I'm using the same assumption in https://review.coreboot.org/c/coreboot/+/39304/4/src/lib/cbfs.c#96 -- oh okay, I see you didn't like it there either. ;)

I think it's fine though, because both for the RO and RW use case in cbfs_get_boot_device() we eventually end up making a fmap_locate_area_as_rdev() (which calls boot_device_ro_subregion()). Everything in the CBFS stack is just about reading, we never write, it never calls fmap_locate_area_as_rdev_rw(). So even for the split boot device platforms this should be fine.

I have added some comments to document this in the declarations of struct cbfs_boot_device and boot_device_ro(), does that help?

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.h:

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
PS3, Line 48: struct region_device *rde
> const

Done

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/security/tpm/tspi/crtm.... 
PS3, Line 109: rdev
> variable naming conflict.

Whoops, sorry, looks like I left this half-finished somehow. Done.

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39327

to look at the new patch set (#6).

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Also export the function externally. Since it is a low-level API and
most code should use the higher-level loading or mapping functions
instead, put it into a new <cbfs_private.h> to raise the mental barrier
for using this API (this will make more sense once cbfs_boot_locate() is
removed from <cbfs.h>).

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
---
M src/include/boot_device.h
M src/include/cbfs.h
A src/include/cbfs_private.h
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
6 files changed, 76 insertions(+), 39 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/39327/6

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@62 
PS3, Line 62: 	const struct region_device *bootdev = boot_device_ro();
> I'm using the assumption here that boot_device_ro() is always the root rdev for anything cbfs_get_bo […]

It does help some. I think that instead of calling boot_device_ro() here you can do the following as it's the exact semantic you desire.

bootdev = rdev_root(&fh->data);

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@62 
PS3, Line 62: 	const struct region_device *bootdev = boot_device_ro();
> It does help some. […]

rdev_root() is currently not exported, it's an implementation detail of the region device API. I could change that and use it here but that would still only solve this case, not the one in CB:39304, so I would still like to establish that boot_device_ro() can always be relied upon to be the root device for all CBFS files anyway.

My long term goal is to get rid of this function anyway and replace it by direct use of cbfs_boot_lookup() everywhere. Can I just leave the code like this for now and eventually I'll delete it anyway?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@62 
PS3, Line 62: 	const struct region_device *bootdev = boot_device_ro();
> rdev_root() is currently not exported, it's an implementation detail of the region device API. […]

I missed that it was static to the compilation unit. It looks so out of place and wrong, but we can leave it as-is.

I did want to hear about how we ensure the mdata is always from the metadata cache. That will be important that we get correct. Right now, that's obviously not the case.

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39327

to look at the new patch set (#8).

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Also export the function externally. Since it is a low-level API and
most code should use the higher-level loading or mapping functions
instead, put it into a new <cbfs_private.h> to raise the mental barrier
for using this API (this will make more sense once cbfs_boot_locate() is
removed from <cbfs.h>).

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
---
M src/include/boot_device.h
M src/include/cbfs.h
A src/include/cbfs_private.h
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
6 files changed, 74 insertions(+), 41 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/39327/8

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c@62 
PS3, Line 62: 	const struct region_device *bootdev = boot_device_ro();
> I did want to hear about how we ensure the mdata is always from the metadata cache. That will be important that we get correct. Right now, that's obviously not the case.

Right, this function basically doesn't fully use the mcache because I'm trying to get rid of it anyway and didn't spend too much time one it.

Actually, I could also just put a whole union cbfs_mdata into struct cbfsf and then point the rdev at the struct itself, that would both match the existing API and work around your concerns. Not sure why I didn't think of that before. Code updated, let me know what you think.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

Patch Set 10: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39327 )

Change subject: cbfs: Move more stuff into cbfs_boot_lookup()
......................................................................

cbfs: Move more stuff into cbfs_boot_lookup()

cbfs_boot_locate() is supposed to be deprecated eventually, after slowly
migrating all APIs to bypass it. That means common features (like
RO-fallback or measurement) need to be moved to the new
cbfs_boot_lookup().

Also export the function externally. Since it is a low-level API and
most code should use the higher-level loading or mapping functions
instead, put it into a new <cbfs_private.h> to raise the mental barrier
for using this API (this will make more sense once cbfs_boot_locate() is
removed from <cbfs.h>).

Signed-off-by: Julius Werner 
Change-Id: I4bc9b7cbc42a4211d806a3e3389abab7f589a25a
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39327
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
---
M src/include/boot_device.h
M src/include/cbfs.h
A src/include/cbfs_private.h
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
6 files changed, 74 insertions(+), 41 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved

diff --git a/src/include/boot_device.h b/src/include/boot_device.h
index a03e5aa..84bd16e 100644
--- a/src/include/boot_device.h
+++ b/src/include/boot_device.h
@@ -27,7 +27,8 @@
  * most likely not to work so don't rely on such semantics.
  */

-/* Return the region_device for the read-only boot device. */
+/* Return the region_device for the read-only boot device. This is the root
+   device for all CBFS boot devices. */
 const struct region_device *boot_device_ro(void);

/* Return the region_device for the read-write boot device. */
diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index 32ed7f8..1b446ac 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -42,6 +42,12 @@
 /* Load stage into memory filling in prog. Return 0 on success. < 0 on error. */
 int cbfs_prog_stage_load(struct prog *prog);

+/*
+ * Data structure that represents "a" CBFS boot device, with optional metadata
+ * cache. Generally we only have one of these, or two (RO and RW) when
+ * CONFIG(VBOOT) is set. The region device stored here must always be a
+ * subregion of boot_device_ro().
+ */
 struct cbfs_boot_device {
    struct region_device rdev;
    void *mcache;
diff --git a/src/include/cbfs_private.h b/src/include/cbfs_private.h
new file mode 100644
index 0000000..8e98036
--- /dev/null
+++ b/src/include/cbfs_private.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _CBFS_PRIVATE_H_
+#define _CBFS_PRIVATE_H_
+
+#include <commonlib/bsd/cbfs_private.h>
+#include <commonlib/region.h>
+#include <types.h>
+
+/*
+ * This header contains low-level CBFS APIs that should only be used by code
+ * that really needs this level of access. Most code (particularly platform
+ * code) should use the higher-level CBFS APIs in <cbfs.h>. Code using these
+ * APIs needs to take special care to ensure CBFS file data is verified (in a
+ * TOCTOU-safe manner) before access (TODO: add details on how to do this once
+ * file verification code is in).
+ */
+
+/* Find by name, load metadata into |mdata| and chain file data to |rdev|. */
+cb_err_t cbfs_boot_lookup(const char *name, bool force_ro,
+			  union cbfs_mdata *mdata, struct region_device *rdev);
+
+#endif
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 3b7d429..94dae62 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -3,8 +3,8 @@
 #include <assert.h>
 #include <boot_device.h>
 #include <cbfs.h>
+#include <cbfs_private.h>
 #include <cbmem.h>
-#include <commonlib/bsd/cbfs_private.h>
 #include <commonlib/bsd/compression.h>
 #include <commonlib/endian.h>
 #include <console/console.h>
@@ -17,43 +17,49 @@
 #include <symbols.h>
 #include <timestamp.h>

-static cb_err_t cbfs_boot_lookup(const struct cbfs_boot_device *cbd,
-		const char *name, union cbfs_mdata *mdata, size_t *data_offset)
+cb_err_t cbfs_boot_lookup(const char *name, bool force_ro,
+			  union cbfs_mdata *mdata, struct region_device *rdev)
 {
+	const struct cbfs_boot_device *cbd = cbfs_get_boot_device(force_ro);
+	if (!cbd)
+		return CB_ERR;
+
+	size_t data_offset;
    cb_err_t err = CB_CBFS_CACHE_FULL;
    if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM)
    	err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size,
-					  name, mdata, data_offset);
+					  name, mdata, &data_offset);
    if (err == CB_CBFS_CACHE_FULL)
-		err = cbfs_lookup(&cbd->rdev, name, mdata, data_offset, NULL);
-	return err;
+		err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, NULL);
+
+	if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro &&
+	    err == CB_CBFS_NOT_FOUND) {
+		printk(BIOS_INFO, "CBFS: Fall back to RO region for %s\n",
+		       name);
+		return cbfs_boot_lookup(name, true, mdata, rdev);
+	}
+	if (err)
+		return err;
+
+	if (rdev_chain(rdev, &cbd->rdev, data_offset, be32toh(mdata->h.len)))
+		return CB_ERR;
+
+	if (tspi_measure_cbfs_hook(rdev, name, be32toh(mdata->h.type)))
+		return CB_ERR;
+
+	return CB_SUCCESS;
 }

int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type)
 {
-	const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false);
-	if (!cbd)
-		return -1;
-
-	size_t data_offset;
-	cb_err_t err = cbfs_boot_lookup(cbd, name, &fh->mdata, &data_offset);
-
-	if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && err == CB_CBFS_NOT_FOUND) {
-		printk(BIOS_INFO, "CBFS: Fall back to RO region for %s\n",
-		       name);
-		if (!(cbd = cbfs_get_boot_device(true)))
-			return -1;
-		err = cbfs_boot_lookup(cbd, name, &fh->mdata, &data_offset);
-	}
-	if (err)
+	if (cbfs_boot_lookup(name, false, &fh->mdata, &fh->data))
    	return -1;

size_t msize = be32toh(fh->mdata.h.offset);
    if (rdev_chain(&fh->metadata, &addrspace_32bit.rdev,
-		       (uintptr_t)&fh->mdata, msize) ||
-	    rdev_chain(&fh->data, &cbd->rdev, data_offset,
-		       be32toh(fh->mdata.h.len)))
+		       (uintptr_t)&fh->mdata, msize))
    	return -1;
+
    if (type) {
    	if (!*type)
    		*type = be32toh(fh->mdata.h.type);
@@ -61,9 +67,6 @@
    		return -1;
    }

-	if (tspi_measure_cbfs_hook(fh, name))
-		return -1;
-
    return 0;
 }

@@ -94,9 +97,13 @@
    	return -1;
    }

+	uint32_t dummy_type = 0;
+	if (!type)
+		type = &dummy_type;
+
    ret = cbfs_locate(fh, &rdev, name, type);
    if (!ret)
-		if (tspi_measure_cbfs_hook(fh, name))
+		if (tspi_measure_cbfs_hook(&rdev, name, *type))
    		return -1;
    return ret;
 }
diff --git a/src/security/tpm/tspi/crtm.c b/src/security/tpm/tspi/crtm.c
index eb07442..80483d5 100644
--- a/src/security/tpm/tspi/crtm.c
+++ b/src/security/tpm/tspi/crtm.c
@@ -102,11 +102,10 @@
    return !strcmp(allowlist, name);
 }

-uint32_t tspi_measure_cbfs_hook(struct cbfsf *fh, const char *name)
+uint32_t tspi_measure_cbfs_hook(const struct region_device *rdev, const char *name,
+				uint32_t cbfs_type)
 {
    uint32_t pcr_index;
-	uint32_t cbfs_type;
-	struct region_device rdev;
    char tcpa_metadata[TCPA_PCR_HASH_NAME];

if (!tcpa_log_available()) {
@@ -118,9 +117,6 @@
    	printk(BIOS_DEBUG, "CRTM initialized.\n");
    }

-	cbfsf_file_type(fh, &cbfs_type);
-	cbfs_file_data(&rdev, fh);
-
    switch (cbfs_type) {
    case CBFS_TYPE_MRC_CACHE:
    	pcr_index = TPM_RUNTIME_DATA_PCR;
@@ -143,10 +139,10 @@
    	break;
    }

-	if (create_tcpa_metadata(&rdev, name, tcpa_metadata) < 0)
+	if (create_tcpa_metadata(rdev, name, tcpa_metadata) < 0)
    	return VB2_ERROR_UNKNOWN;

-	return tpm_measure_region(&rdev, pcr_index, tcpa_metadata);
+	return tpm_measure_region(rdev, pcr_index, tcpa_metadata);
 }

int tspi_measure_cache_to_pcr(void)
diff --git a/src/security/tpm/tspi/crtm.h b/src/security/tpm/tspi/crtm.h
index 1b29854..f3678ef 100644
--- a/src/security/tpm/tspi/crtm.h
+++ b/src/security/tpm/tspi/crtm.h
@@ -41,13 +41,13 @@
 #if !ENV_SMM && CONFIG(TPM_MEASURED_BOOT)
 /*
  * Measures cbfs data via hook (cbfs)
- * fh is the cbfs file handle to measure
+ * rdev covers the file data (not metadata)
  * return 0 if successful, else an error
  */
-uint32_t tspi_measure_cbfs_hook(struct cbfsf *fh, const char *name);
-
+uint32_t tspi_measure_cbfs_hook(const struct region_device *rdev,
+				const char *name, uint32_t cbfs_type);
 #else
-#define tspi_measure_cbfs_hook(fh, name) 0
+#define tspi_measure_cbfs_hook(rdev, name, cbfs_type) 0
 #endif

#endif /* __SECURITY_TSPI_CRTM_H__ */