Change in coreboot[master]: security/vboot: Ensure firmware body size is respected again
Hello Aaron Durbin, Joel Kitching,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37680
to review the following change.
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
security/vboot: Ensure firmware body size is respected again
CB:36845 simplified how coreboot finds the RW CBFS after vboot has and eliminated a layer of caching. Unfortunately, we missed the fact that the former cached value didn't exactly match the FMAP section... it was in fact truncated to the data actually used by vboot. That patch unintentionally broke this truncation which leads to performance regressions on certain CBFS accesses.
This patch makes use of a new API function added to vboot (CL:1965920) which we can use to retrieve the real firmware body length as before.
(Also stop making all the vb2_context pointers const. vboot generally never marks context pointers as const in its API functions, even when the function doesn't modify the context. Therefore constifying it inside coreboot just makes things weird because it prevents you from calling random API functions for no reason. If we really want const context pointers, that's a refactoring that would have to start inside vboot first.)
Change-Id: I167cd40cb435dbae7f09d6069c9f1ffc1d99fe13 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 4 files changed, 22 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/37680/1
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index c21fe15..214f6fa 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -68,8 +68,7 @@ return vboot_ctx; }
-int vboot_locate_firmware(const struct vb2_context *ctx, - struct region_device *fw) +int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw) { const char *name;
@@ -78,7 +77,12 @@ else name = "FW_MAIN_B";
- return fmap_locate_area_as_rdev(name, fw); + int ret = fmap_locate_area_as_rdev(name, fw); + if (ret) + return ret; + + /* Truncate area to the size that was actually signed by vboot. */ + return rdev_chain(fw, fw, 0, vb2api_get_firmware_size(ctx)); }
static void vboot_setup_cbmem(int unused) diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1fda8b4..0b2c8e5 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -30,7 +30,7 @@ /* * Returns 1 if firmware slot A is used, 0 if slot B is used. */ -static inline int vboot_is_firmware_slot_a(const struct vb2_context *ctx) +static inline int vboot_is_firmware_slot_a(struct vb2_context *ctx) { return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); } @@ -49,8 +49,7 @@ /* * Locates firmware as a region device. Returns 0 on success, -1 on failure. */ -int vboot_locate_firmware(const struct vb2_context *ctx, - struct region_device *fw); +int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw);
/* * Source: security/vboot/bootmode.c diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 9aaaff2..b72c82b 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -72,7 +72,7 @@
static int vboot_locate(struct region_device *rdev) { - const struct vb2_context *ctx; + struct vb2_context *ctx;
/* Don't honor vboot results until the vboot logic has run. */ if (!vboot_logic_executed()) diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 6c4f8fd..1d17a17 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -173,10 +173,10 @@ }
static vb2_error_t hash_body(struct vb2_context *ctx, - struct region_device *fw_main) + struct region_device *fw_body) { uint64_t load_ts; - uint32_t expected_size; + uint32_t remaining; uint8_t block[TODO_BLOCK_SIZE]; uint8_t hash_digest[VBOOT_MAX_HASH_SIZE]; const size_t hash_digest_sz = sizeof(hash_digest); @@ -197,33 +197,22 @@ load_ts = timestamp_get(); timestamp_add(TS_START_HASH_BODY, load_ts);
- expected_size = region_device_sz(fw_main); + remaining = region_device_sz(fw_body); offset = 0;
/* Start the body hash */ - rv = vb2api_init_hash(ctx, VB2_HASH_TAG_FW_BODY, &expected_size); + rv = vb2api_init_hash(ctx, VB2_HASH_TAG_FW_BODY); if (rv) return rv;
- /* - * Honor vboot's RW slot size. The expected size is pulled out of - * the preamble and obtained through vb2api_init_hash() above. By - * creating sub region the RW slot portion of the boot media is - * limited. - */ - if (rdev_chain(fw_main, fw_main, 0, expected_size)) { - printk(BIOS_ERR, "Unable to restrict CBFS size.\n"); - return VB2_ERROR_UNKNOWN; - } - /* Extend over the body */ - while (expected_size) { + while (remaining) { uint64_t temp_ts; - if (block_size > expected_size) - block_size = expected_size; + if (block_size > remaining) + block_size = remaining;
temp_ts = timestamp_get(); - if (rdev_readat(fw_main, block, offset, block_size) < 0) + if (rdev_readat(fw_body, block, offset, block_size) < 0) return VB2_ERROR_UNKNOWN; load_ts += timestamp_get() - temp_ts;
@@ -231,7 +220,7 @@ if (rv) return rv;
- expected_size -= block_size; + remaining -= block_size; offset += block_size; }
@@ -309,7 +298,7 @@ void verstage_main(void) { struct vb2_context *ctx; - struct region_device fw_main; + struct region_device fw_body; vb2_error_t rv;
timestamp_add_now(TS_START_VBOOT); @@ -405,12 +394,12 @@ }
printk(BIOS_INFO, "Phase 4\n"); - rv = vboot_locate_firmware(ctx, &fw_main); + rv = vboot_locate_firmware(ctx, &fw_body); if (rv) die_with_post_code(POST_INVALID_ROM, "Failed to read FMAP to locate firmware");
- rv = hash_body(ctx, &fw_main); + rv = hash_body(ctx, &fw_body); vboot_save_data(ctx); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv);
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37680/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/37680/1/src/security/vboot/vboot_lo... PS1, Line 200: region_device_sz Should this be vb2api_get_firmware_size(ctx)?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37680/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/37680/1/src/security/vboot/vboot_lo... PS1, Line 200: region_device_sz
Should this be vb2api_get_firmware_size(ctx)?
With this patch, the rdev returned by vboot_locate_firmware() is already truncated. So region_device_sz(fw_body) will already be equal to vb2api_get_firmware_size() here, no need to call it again.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37680/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/37680/1/src/security/vboot/vboot_lo... PS1, Line 200: region_device_sz
With this patch, the rdev returned by vboot_locate_firmware() is already truncated. […]
Got it!
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
Patch Set 1: Code-Review+2
Looks fine once we get the vboot patch over here.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
Patch Set 1: Code-Review+2
Hello Aaron Durbin, Mathew King, build bot (Jenkins), Joel Kitching, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37680
to look at the new patch set (#2).
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
security/vboot: Ensure firmware body size is respected again
CB:36845 simplified how coreboot finds the RW CBFS after vboot has and eliminated a layer of caching. Unfortunately, we missed the fact that the former cached value didn't exactly match the FMAP section... it was in fact truncated to the data actually used by vboot. That patch unintentionally broke this truncation which leads to performance regressions on certain CBFS accesses.
This patch makes use of a new API function added to vboot (CL:1965920) which we can use to retrieve the real firmware body length as before.
(Also stop making all the vb2_context pointers const. vboot generally never marks context pointers as const in its API functions, even when the function doesn't modify the context. Therefore constifying it inside coreboot just makes things weird because it prevents you from calling random API functions for no reason. If we really want const context pointers, that's a refactoring that would have to start inside vboot first.)
This patch brings in upstream vboot commit 4b0408d2: 2019-12-12 Julius Werner 2lib: Move firmware body size reporting to separate function
Change-Id: I167cd40cb435dbae7f09d6069c9f1ffc1d99fe13 Signed-off-by: Julius Werner jwerner@chromium.org --- M 3rdparty/vboot M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 5 files changed, 23 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/37680/2
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
Patch Set 2: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37680 )
Change subject: security/vboot: Ensure firmware body size is respected again ......................................................................
security/vboot: Ensure firmware body size is respected again
CB:36845 simplified how coreboot finds the RW CBFS after vboot has and eliminated a layer of caching. Unfortunately, we missed the fact that the former cached value didn't exactly match the FMAP section... it was in fact truncated to the data actually used by vboot. That patch unintentionally broke this truncation which leads to performance regressions on certain CBFS accesses.
This patch makes use of a new API function added to vboot (CL:1965920) which we can use to retrieve the real firmware body length as before.
(Also stop making all the vb2_context pointers const. vboot generally never marks context pointers as const in its API functions, even when the function doesn't modify the context. Therefore constifying it inside coreboot just makes things weird because it prevents you from calling random API functions for no reason. If we really want const context pointers, that's a refactoring that would have to start inside vboot first.)
This patch brings in upstream vboot commit 4b0408d2: 2019-12-12 Julius Werner 2lib: Move firmware body size reporting to separate function
Change-Id: I167cd40cb435dbae7f09d6069c9f1ffc1d99fe13 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37680 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Mathew King mathewk@chromium.org --- M 3rdparty/vboot M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 5 files changed, 23 insertions(+), 31 deletions(-)
Approvals: build bot (Jenkins): Verified Mathew King: Looks good to me, approved
diff --git a/3rdparty/vboot b/3rdparty/vboot index b10e5e3..2843aa6 160000 --- a/3rdparty/vboot +++ b/3rdparty/vboot @@ -1 +1 @@ -Subproject commit b10e5e32cc34dba7660b070616d3481742a28e70 +Subproject commit 2843aa62ba7bcaab2abccf16e3f1b8bd7e058fdb diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index c21fe15..214f6fa 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -68,8 +68,7 @@ return vboot_ctx; }
-int vboot_locate_firmware(const struct vb2_context *ctx, - struct region_device *fw) +int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw) { const char *name;
@@ -78,7 +77,12 @@ else name = "FW_MAIN_B";
- return fmap_locate_area_as_rdev(name, fw); + int ret = fmap_locate_area_as_rdev(name, fw); + if (ret) + return ret; + + /* Truncate area to the size that was actually signed by vboot. */ + return rdev_chain(fw, fw, 0, vb2api_get_firmware_size(ctx)); }
static void vboot_setup_cbmem(int unused) diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1fda8b4..0b2c8e5 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -30,7 +30,7 @@ /* * Returns 1 if firmware slot A is used, 0 if slot B is used. */ -static inline int vboot_is_firmware_slot_a(const struct vb2_context *ctx) +static inline int vboot_is_firmware_slot_a(struct vb2_context *ctx) { return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); } @@ -49,8 +49,7 @@ /* * Locates firmware as a region device. Returns 0 on success, -1 on failure. */ -int vboot_locate_firmware(const struct vb2_context *ctx, - struct region_device *fw); +int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw);
/* * Source: security/vboot/bootmode.c diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 9aaaff2..b72c82b 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -72,7 +72,7 @@
static int vboot_locate(struct region_device *rdev) { - const struct vb2_context *ctx; + struct vb2_context *ctx;
/* Don't honor vboot results until the vboot logic has run. */ if (!vboot_logic_executed()) diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 6c4f8fd..1d17a17 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -173,10 +173,10 @@ }
static vb2_error_t hash_body(struct vb2_context *ctx, - struct region_device *fw_main) + struct region_device *fw_body) { uint64_t load_ts; - uint32_t expected_size; + uint32_t remaining; uint8_t block[TODO_BLOCK_SIZE]; uint8_t hash_digest[VBOOT_MAX_HASH_SIZE]; const size_t hash_digest_sz = sizeof(hash_digest); @@ -197,33 +197,22 @@ load_ts = timestamp_get(); timestamp_add(TS_START_HASH_BODY, load_ts);
- expected_size = region_device_sz(fw_main); + remaining = region_device_sz(fw_body); offset = 0;
/* Start the body hash */ - rv = vb2api_init_hash(ctx, VB2_HASH_TAG_FW_BODY, &expected_size); + rv = vb2api_init_hash(ctx, VB2_HASH_TAG_FW_BODY); if (rv) return rv;
- /* - * Honor vboot's RW slot size. The expected size is pulled out of - * the preamble and obtained through vb2api_init_hash() above. By - * creating sub region the RW slot portion of the boot media is - * limited. - */ - if (rdev_chain(fw_main, fw_main, 0, expected_size)) { - printk(BIOS_ERR, "Unable to restrict CBFS size.\n"); - return VB2_ERROR_UNKNOWN; - } - /* Extend over the body */ - while (expected_size) { + while (remaining) { uint64_t temp_ts; - if (block_size > expected_size) - block_size = expected_size; + if (block_size > remaining) + block_size = remaining;
temp_ts = timestamp_get(); - if (rdev_readat(fw_main, block, offset, block_size) < 0) + if (rdev_readat(fw_body, block, offset, block_size) < 0) return VB2_ERROR_UNKNOWN; load_ts += timestamp_get() - temp_ts;
@@ -231,7 +220,7 @@ if (rv) return rv;
- expected_size -= block_size; + remaining -= block_size; offset += block_size; }
@@ -309,7 +298,7 @@ void verstage_main(void) { struct vb2_context *ctx; - struct region_device fw_main; + struct region_device fw_body; vb2_error_t rv;
timestamp_add_now(TS_START_VBOOT); @@ -405,12 +394,12 @@ }
printk(BIOS_INFO, "Phase 4\n"); - rv = vboot_locate_firmware(ctx, &fw_main); + rv = vboot_locate_firmware(ctx, &fw_body); if (rv) die_with_post_code(POST_INVALID_ROM, "Failed to read FMAP to locate firmware");
- rv = hash_body(ctx, &fw_main); + rv = hash_body(ctx, &fw_body); vboot_save_data(ctx); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv);
-
Aaron Durbin (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Mathew King (Code Review)