Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39305
to review the following change.
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
cbfs: Port cbfs_load() and cbfs_map() to new API
This patch adapts cbfs_load() and cbfs_map() to use the new CBFS API directly, rather than through cbfs_boot_locate(). For cbfs_load() this means that attribute metadata does not need to be read twice.
Change-Id: I754cc34b1c1471129e15475aa0f1891e02439a02 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/lib/cbfs.c 3 files changed, 43 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39305/1
diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c index 85a75ab..7e309ec 100644 --- a/src/commonlib/bsd/cbfs_private.c +++ b/src/commonlib/bsd/cbfs_private.c @@ -155,3 +155,21 @@ }; return cbfs_walk(dev, lookup_walker, &args, master_hash, 0); } + +void *cbfs_find_attr(union cbfs_mdata *mdata, uint32_t attr_tag) +{ + uint32_t offset = be32toh(mdata->h.attributes_offset); + uint32_t end = be32toh(mdata->h.offset); + + if (!offset) + return NULL; + + while (offset < end) { + struct cbfs_file_attribute *attr = (void *)mdata + offset; + if (be32toh(attr->tag) == attr_tag) + return attr; + offset += be32toh(attr->len); + } + + return NULL; +} diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h index df97009..4962cef 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h @@ -112,4 +112,7 @@ /* Returns the amount of bytes actually used by the CBFS metadata cache in |mcache|. */ size_t cbfs_mcache_real_size(const void *mcache, size_t mcache_size);
+/* Finds a CBFS attribute in a metadata block. Attribute returned as-is (still big-endian). */ +void *cbfs_find_attr(union cbfs_mdata *mdata, uint32_t attr_tag); + #endif /* _COMMONLIB_BSD_CBFS_PRIVATE_H_ */ diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index bece4d0..c3660aa 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -81,18 +81,19 @@
void *cbfs_map(const char *name, size_t *size) { - struct cbfsf fh; - size_t fsize; + const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false); + union cbfs_mdata mdata; + size_t data_offset;
- if (cbfs_boot_locate(&fh, name, NULL)) + if (cbfs_boot_lookup(cbd, name, &mdata, &data_offset)) return NULL;
- fsize = region_device_sz(&fh.data); + size_t data_size = be32toh(mdata.h.len);
if (size != NULL) - *size = fsize; + *size = data_size;
- return rdev_mmap(&fh.data, 0, fsize); + return rdev_mmap(&cbd->rdev, data_offset, data_size); }
int cbfs_unmap(void *mapping) @@ -216,21 +217,25 @@
size_t cbfs_load(const char *name, void *buf, size_t buf_size) { - struct cbfsf fh; - uint32_t compression_algo; - size_t decompressed_size; + const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false); + union cbfs_mdata mdata; + size_t data_offset;
- if (cbfs_boot_locate(&fh, name, NULL) < 0) + if (cbfs_boot_lookup(cbd, name, &mdata, &data_offset)) return 0;
- if (cbfsf_decompression_info(&fh, &compression_algo, - &decompressed_size) - < 0 - || decompressed_size > buf_size) - return 0; + uint32_t compression = CBFS_COMPRESS_NONE; + struct cbfs_file_attr_compression *attr = + cbfs_find_attr(&mdata, CBFS_FILE_ATTR_TAG_COMPRESSION); + if (attr) { + compression = be32toh(attr->compression); + if (buf_size < be32toh(attr->decompressed_size)) + return 0; + }
- return cbfs_load_and_decompress(&fh.data, 0, region_device_sz(&fh.data), - buf, buf_size, compression_algo); + return cbfs_load_and_decompress(&cbd->rdev, data_offset, + be32toh(mdata.h.len), buf, buf_size, + compression); }
size_t cbfs_prog_stage_section(struct prog *pstage, uintptr_t *base)
Hello build bot (Jenkins), Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39305
to look at the new patch set (#2).
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
cbfs: Port cbfs_load() and cbfs_map() to new API
This patch adapts cbfs_load() and cbfs_map() to use the new CBFS API directly, rather than through cbfs_boot_locate(). For cbfs_load() this means that attribute metadata does not need to be read twice.
Change-Id: I754cc34b1c1471129e15475aa0f1891e02439a02 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/lib/cbfs.c 3 files changed, 43 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39305/2
Hello build bot (Jenkins), Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39305
to look at the new patch set (#3).
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
cbfs: Port cbfs_load() and cbfs_map() to new API
This patch adapts cbfs_load() and cbfs_map() to use the new CBFS API directly, rather than through cbfs_boot_locate(). For cbfs_load() this means that attribute metadata does not need to be read twice.
Change-Id: I754cc34b1c1471129e15475aa0f1891e02439a02 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/lib/cbfs.c 3 files changed, 39 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39305/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39305/4/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
https://review.coreboot.org/c/coreboot/+/39305/4/src/commonlib/bsd/include/c... PS4, Line 116: (still big-endian) I don't understand why we're exposing serialized implementation details to callers in both the inputs and outputs.
https://review.coreboot.org/c/coreboot/+/39305/4/src/commonlib/bsd/include/c... PS4, Line 117: void *cbfs_find_attr(union cbfs_mdata *mdata, uint32_t attr_tag); Why are these pointers not pointing to const objects?
Hello build bot (Jenkins), Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39305
to look at the new patch set (#5).
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
cbfs: Port cbfs_load() and cbfs_map() to new API
This patch adapts cbfs_load() and cbfs_map() to use the new CBFS API directly, rather than through cbfs_boot_locate(). For cbfs_load() this means that attribute metadata does not need to be read twice.
Change-Id: I754cc34b1c1471129e15475aa0f1891e02439a02 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/lib/cbfs.c 3 files changed, 39 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39305/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39305/4/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
https://review.coreboot.org/c/coreboot/+/39305/4/src/commonlib/bsd/include/c... PS4, Line 116: (still big-endian)
I don't understand why we're exposing serialized implementation details to callers in both the input […]
This is just finding an attribute, it doesn't know anything about that specific attributes contents, so it wouldn't know what to byteswap. (Again I prefer to not apply byte order swapping within the structures that represent the layout on flash, to avoid confusion. This is supposed to be a low-level helper function that's only used by other CBFS code, so I wouldn't call it "exposing".)
https://review.coreboot.org/c/coreboot/+/39305/4/src/commonlib/bsd/include/c... PS4, Line 117: void *cbfs_find_attr(union cbfs_mdata *mdata, uint32_t attr_tag);
Why are these pointers not pointing to const objects?
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39305/7/src/commonlib/bsd/cbfs_priv... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/39305/7/src/commonlib/bsd/cbfs_priv... PS7, Line 172: return attr; Should we be checking length of attribute actually fits within mdata? i.e. offset + len <= end?
Hello build bot (Jenkins), Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39305
to look at the new patch set (#9).
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
cbfs: Port cbfs_load() and cbfs_map() to new API
This patch adapts cbfs_load() and cbfs_map() to use the new CBFS API directly, rather than through cbfs_boot_locate(). For cbfs_load() this means that attribute metadata does not need to be read twice.
Change-Id: I754cc34b1c1471129e15475aa0f1891e02439a02 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/lib/cbfs.c 3 files changed, 55 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39305/9
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39305/7/src/commonlib/bsd/cbfs_priv... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/39305/7/src/commonlib/bsd/cbfs_priv... PS7, Line 172: return attr;
Should we be checking length of attribute actually fits within mdata? i.e. […]
Hmm... hmmmmmmmm... yeah, you know what, that's a very good point. And then the caller also needs to check the length again before it can assume it can safely dereference attribute-specific fields. And in fact the while loop here also needs to make sure tag and len can be safely dereferenced. I guess I really didn't pay enough attention here. (Maybe I thought it's fine because verification would've already failed at this point? But still, we should always do proper bounds checking.)
Ideally we should have an API that automatically checks the whole thing is safe to use without requiring extra action from the caller. But some attributes like hashes have variable size... hmm... I think I'll add an optional size field to the function so callers can pass sizeof(*attr) to get automatic checking for the common case of fixed-size attributes, but also leave it out in cases where the size may be variable.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
Patch Set 11: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
cbfs: Port cbfs_load() and cbfs_map() to new API
This patch adapts cbfs_load() and cbfs_map() to use the new CBFS API directly, rather than through cbfs_boot_locate(). For cbfs_load() this means that attribute metadata does not need to be read twice.
Change-Id: I754cc34b1c1471129e15475aa0f1891e02439a02 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/39305 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h M src/lib/cbfs.c 3 files changed, 55 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c index 035684b..7814c4a 100644 --- a/src/commonlib/bsd/cbfs_private.c +++ b/src/commonlib/bsd/cbfs_private.c @@ -159,3 +159,35 @@ }; return cbfs_walk(dev, lookup_walker, &args, metadata_hash, 0); } + +const void *cbfs_find_attr(const union cbfs_mdata *mdata, uint32_t attr_tag, size_t size_check) +{ + uint32_t offset = be32toh(mdata->h.attributes_offset); + uint32_t end = be32toh(mdata->h.offset); + + if (!offset) + return NULL; + + while (offset + sizeof(struct cbfs_file_attribute) <= end) { + const struct cbfs_file_attribute *attr = (const void *)mdata->raw + offset; + const uint32_t tag = be32toh(attr->tag); + const uint32_t len = be32toh(attr->len); + + if (offset + len > end) { + ERROR("Attribute %s[%u] overflows end of metadata\n", + mdata->filename, tag); + return NULL; + } + if (tag == attr_tag) { + if (size_check && len != size_check) { + ERROR("Attribute %s[%u] size mismatch: %u != %zu\n", + mdata->filename, tag, len, size_check); + return NULL; + } + return attr; + } + offset += len; + } + + return NULL; +} diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h index 64dcf9f..b72463a 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h @@ -134,4 +134,9 @@ /* Returns the amount of bytes actually used by the CBFS metadata cache in |mcache|. */ size_t cbfs_mcache_real_size(const void *mcache, size_t mcache_size);
+/* Finds a CBFS attribute in a metadata block. Attribute returned as-is (still big-endian). + If |size| is not 0, will check that it matches the length of the attribute (if found)... + else caller is responsible for checking the |len| field to avoid reading out-of-bounds. */ +const void *cbfs_find_attr(const union cbfs_mdata *mdata, uint32_t attr_tag, size_t size_check); + #endif /* _COMMONLIB_BSD_CBFS_PRIVATE_H_ */ diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 493093e..8d868a6 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -72,18 +72,16 @@
void *cbfs_map(const char *name, size_t *size_out) { - struct cbfsf fh; - size_t fsize; + struct region_device rdev; + union cbfs_mdata mdata;
- if (cbfs_boot_locate(&fh, name, NULL)) + if (cbfs_boot_lookup(name, false, &mdata, &rdev)) return NULL;
- fsize = region_device_sz(&fh.data); - if (size_out != NULL) - *size_out = fsize; + *size_out = region_device_sz(&rdev);
- return rdev_mmap(&fh.data, 0, fsize); + return rdev_mmap_full(&rdev); }
int cbfs_unmap(void *mapping) @@ -285,21 +283,23 @@
size_t cbfs_load(const char *name, void *buf, size_t buf_size) { - struct cbfsf fh; - uint32_t compression_algo; - size_t decompressed_size; + struct region_device rdev; + union cbfs_mdata mdata;
- if (cbfs_boot_locate(&fh, name, NULL) < 0) + if (cbfs_boot_lookup(name, false, &mdata, &rdev)) return 0;
- if (cbfsf_decompression_info(&fh, &compression_algo, - &decompressed_size) - < 0 - || decompressed_size > buf_size) - return 0; + uint32_t compression = CBFS_COMPRESS_NONE; + const struct cbfs_file_attr_compression *attr = cbfs_find_attr(&mdata, + CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*attr)); + if (attr) { + compression = be32toh(attr->compression); + if (buf_size < be32toh(attr->decompressed_size)) + return 0; + }
- return cbfs_load_and_decompress(&fh.data, 0, region_device_sz(&fh.data), - buf, buf_size, compression_algo); + return cbfs_load_and_decompress(&rdev, 0, region_device_sz(&rdev), + buf, buf_size, compression); }
int cbfs_prog_stage_load(struct prog *pstage)
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)