Hello Aaron Durbin, Frans Hendriks,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/36797
to review the following change.
Change subject: cbfs: Make cbfs_master_header_props() externally available ......................................................................
cbfs: Make cbfs_master_header_props() externally available
This patch makes the CBFS default locator .locate() callback externally available so that code which overrides cbfs_master_header_locator can reuse or wrap it and doesn't have to copy&paste the whole thing. Use it for the Eltan vendorcode implementation which previously did this.
Change-Id: I54dad5c8ea64ea0fc472217e275daa815736991e Signed-off-by: Julius Werner jwerner@chromium.org --- M src/include/cbfs.h M src/lib/cbfs.c M src/vendorcode/eltan/security/verified_boot/vboot_check.c 3 files changed, 5 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/36797/1
diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 85e25b3..f012441 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -73,6 +73,9 @@ size_t size; };
+/* Default CBFS locator .locate() callback that locates "COREBOOT" region. */ +int cbfs_master_header_props(struct cbfs_props *props); + /* Return < 0 on error otherwise props are filled out accordingly. */ int cbfs_boot_region_properties(struct cbfs_props *props);
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 13b5afb..1601f20 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -298,7 +298,7 @@ }
/* This only supports the "COREBOOT" fmap region. */ -static int cbfs_master_header_props(struct cbfs_props *props) +int cbfs_master_header_props(struct cbfs_props *props) { struct cbfs_header header; const struct region_device *bdev; diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.c b/src/vendorcode/eltan/security/verified_boot/vboot_check.c index 1dc1c3a..823c2de 100644 --- a/src/vendorcode/eltan/security/verified_boot/vboot_check.c +++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.c @@ -17,7 +17,6 @@ #include <cbfs.h> #include <vboot_check.h> #include <vboot_common.h> -#include "fmap_config.h"
#define RSA_PUBLICKEY_FILE_NAME "vboot_public_key.bin"
@@ -91,46 +90,6 @@ return -1; }
-static int vendor_secure_locate(struct cbfs_props *props) -{ - struct cbfs_header header; - const struct region_device *bdev; - int32_t rel_offset; - size_t offset; - - bdev = boot_device_ro(); - - if (bdev == NULL) - return -1; - - size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE; - - /* Find location of header using signed 32-bit offset from - * end of CBFS region. */ - offset = fmap_top - sizeof(int32_t); - if (rdev_readat(bdev, &rel_offset, offset, sizeof(int32_t)) < 0) - return -1; - - offset = fmap_top + rel_offset; - if (rdev_readat(bdev, &header, offset, sizeof(header)) < 0) - return -1; - - header.magic = ntohl(header.magic); - header.romsize = ntohl(header.romsize); - header.offset = ntohl(header.offset); - - if (header.magic != CBFS_HEADER_MAGIC) - return -1; - - props->offset = header.offset; - props->size = header.romsize; - props->size -= props->offset; - - printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", props->offset, props->size); - - return 0; -} - /* * * measure_item @@ -399,5 +358,5 @@ const struct cbfs_locator cbfs_master_header_locator = { .name = "Vendorcode Header Locator", .prepare = vendor_secure_prepare, - .locate = vendor_secure_locate + .locate = cbfs_master_header_props };
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36797 )
Change subject: cbfs: Make cbfs_master_header_props() externally available ......................................................................
Patch Set 1: Code-Review+2
I was thinking of this very approach today when I started digging around in the eltan area.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36797 )
Change subject: cbfs: Make cbfs_master_header_props() externally available ......................................................................
Patch Set 2: Code-Review+1
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36797 )
Change subject: cbfs: Make cbfs_master_header_props() externally available ......................................................................
cbfs: Make cbfs_master_header_props() externally available
This patch makes the CBFS default locator .locate() callback externally available so that code which overrides cbfs_master_header_locator can reuse or wrap it and doesn't have to copy&paste the whole thing. Use it for the Eltan vendorcode implementation which previously did this.
Change-Id: I54dad5c8ea64ea0fc472217e275daa815736991e Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36797 Reviewed-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/cbfs.h M src/lib/cbfs.c M src/vendorcode/eltan/security/verified_boot/vboot_check.c 3 files changed, 5 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Wim Vervoorn: Looks good to me, but someone else must approve
diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 85e25b3..f012441 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -73,6 +73,9 @@ size_t size; };
+/* Default CBFS locator .locate() callback that locates "COREBOOT" region. */ +int cbfs_master_header_props(struct cbfs_props *props); + /* Return < 0 on error otherwise props are filled out accordingly. */ int cbfs_boot_region_properties(struct cbfs_props *props);
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 13b5afb..1601f20 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -298,7 +298,7 @@ }
/* This only supports the "COREBOOT" fmap region. */ -static int cbfs_master_header_props(struct cbfs_props *props) +int cbfs_master_header_props(struct cbfs_props *props) { struct cbfs_header header; const struct region_device *bdev; diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.c b/src/vendorcode/eltan/security/verified_boot/vboot_check.c index 1dc1c3a..823c2de 100644 --- a/src/vendorcode/eltan/security/verified_boot/vboot_check.c +++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.c @@ -17,7 +17,6 @@ #include <cbfs.h> #include <vboot_check.h> #include <vboot_common.h> -#include "fmap_config.h"
#define RSA_PUBLICKEY_FILE_NAME "vboot_public_key.bin"
@@ -91,46 +90,6 @@ return -1; }
-static int vendor_secure_locate(struct cbfs_props *props) -{ - struct cbfs_header header; - const struct region_device *bdev; - int32_t rel_offset; - size_t offset; - - bdev = boot_device_ro(); - - if (bdev == NULL) - return -1; - - size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE; - - /* Find location of header using signed 32-bit offset from - * end of CBFS region. */ - offset = fmap_top - sizeof(int32_t); - if (rdev_readat(bdev, &rel_offset, offset, sizeof(int32_t)) < 0) - return -1; - - offset = fmap_top + rel_offset; - if (rdev_readat(bdev, &header, offset, sizeof(header)) < 0) - return -1; - - header.magic = ntohl(header.magic); - header.romsize = ntohl(header.romsize); - header.offset = ntohl(header.offset); - - if (header.magic != CBFS_HEADER_MAGIC) - return -1; - - props->offset = header.offset; - props->size = header.romsize; - props->size -= props->offset; - - printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", props->offset, props->size); - - return 0; -} - /* * * measure_item @@ -399,5 +358,5 @@ const struct cbfs_locator cbfs_master_header_locator = { .name = "Vendorcode Header Locator", .prepare = vendor_secure_prepare, - .locate = vendor_secure_locate + .locate = cbfs_master_header_props };
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Wim Vervoorn (Code Review)