Change in ...coreboot[master]: src/security/vboot: Added logic to also verify FSP_S component ...
Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32156
Change subject: src/security/vboot: Added logic to also verify FSP_S component and syntax change for verify_stage_if_required. ......................................................................
src/security/vboot: Added logic to also verify FSP_S component and syntax change for verify_stage_if_required.
When VBOOT Stage Verification is enabled, FSP_S component needs to be verified. This logic has been added.
TEST=Create a coreboot.rom image by enabling CONFIG_VBOOT and CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots to authenticated payload and graphics is displayed via HDMI and Display Port.
Change-Id: I7e2323086ecddc5195d8b55b47cc71f599b5a0b8 Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc M src/security/vboot/vboot_logic_ex.c 3 files changed, 57 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/32156/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index e7bd3f9..036c553 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -372,6 +372,11 @@ default "$(VBOOT_SOURCE)/tests/devkeys/rhash.vbprik2" depends on VBOOT_STAGE_VERIFICATION
+config VBOOT_2_1_FSPS_HASH_KEY + string "Coreboot fsps.bin Stage Hashing Key(private)" + default "$(VBOOT_SOURCE)/tests/devkeys/fhash.vbprik2" + depends on VBOOT_STAGE_VERIFICATION + config VBOOT_2_1_PAYLOAD_HASH_KEY string "Coreboot PAYLOAD Stage Hashing Key(private)" default "$(VBOOT_SOURCE)/tests/devkeys/phash.vbprik2" diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 1a6ca9f..1c03c78 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -338,9 +338,9 @@ rm -f $@.tmp $@.tmp.size
ifeq ($(CONFIG_VBOOT_STAGE_VERIFICATION), y) -# Extract RAMStage and PAYLOAD and hash them -# Since we are booting from FW_MAIN_A, RAMStage and payload are -# extracted from FW_MAIN_A. +# Extract RAMStage, fsps.bin and PAYLOAD and hash them +# Since we are booting from FW_MAIN_A, RAMStage, fsps.bin +# and payload are extracted from FW_MAIN_A. $(obj)/ramstage.hash: $(obj)/coreboot.rom @printf " CREATE RAMSTAGE HASH\n" $(CBFSTOOL) $< print -r FW_MAIN_A > $<.tmp @@ -351,6 +351,16 @@ $@.tmp $@ rm -f $<.tmp $@.tmp
+$(obj)/fsps.hash: $(obj)/coreboot.rom + @printf " CREATE FSPS.BIN HASH\n" + $(CBFSTOOL) $< extract -n $(call strip_quotes,$(CONFIG_FSP_S_CBFS)) \ + -r FW_MAIN_A -f $@.tmp &> /dev/null + $(FUTILITY) --vb21 sign \ + --type rwsig \ + --prikey "$(CONFIG_VBOOT_2_1_FSPS_HASH_KEY)" \ + $@.tmp $@ + rm -f $<.tmp $@.tmp + $(obj)/payload.hash: $(obj)/coreboot.rom @printf " CREATE PAYLOAD HASH\n" $(CBFSTOOL) $< print -r FW_MAIN_A > $<.tmp @@ -369,7 +379,8 @@ --flags $(CONFIG_VBOOT_KEYBLOCK_PREAMBLE_FLAGS)
$(obj)/VBLOCK_%.bin: $(obj)/FW_MAIN_%.bin $(FUTILITY) \ - $(obj)/ramstage.hash $(obj)/payload.hash $(obj)/firmware.kb21 + $(obj)/ramstage.hash $(obj)/fsps.hash $(obj)/payload.hash \ + $(obj)/firmware.kb21 $(FUTILITY) vbutil_firmware \ --vblock21 $@ \ --keyblock "$(top)/$(obj)/firmware.kb21" \ diff --git a/src/security/vboot/vboot_logic_ex.c b/src/security/vboot/vboot_logic_ex.c index 1b526c7..0a071f0 100644 --- a/src/security/vboot/vboot_logic_ex.c +++ b/src/security/vboot/vboot_logic_ex.c @@ -154,6 +154,32 @@ } }
+/* get the hash id from component name */ +static void get_hash_id(struct vb2_id *id, const char *name) +{ + /* in POSTCAR stage, safely assume that we are + * in the process of verifying RAMSTAGE */ + if (ENV_POSTCAR) { + const struct vb2_id tmp_id = VB2_ID_RAMSTAGE; + memcpy(id, &tmp_id, sizeof(*id)); + } else if (ENV_RAMSTAGE) { + /* In RAMSTAGE, we verify FSPS and PAYLOAD, + * conditionally, so, get the appropriate ID */ + if (!memcmp(name, CONFIG_CBFS_PREFIX"/payload", + sizeof(name))) { + const struct vb2_id tmp_id = VB2_ID_PAYLOAD; + memcpy(id, &tmp_id, sizeof(*id)); + } else if (!memcmp(name, CONFIG_FSP_S_CBFS, + sizeof(name))) { + const struct vb2_id tmp_id = VB2_ID_FSPS; + memcpy(id, &tmp_id, sizeof(*id)); + } + else + die("Invalid component"); + } else + die("Invalid stage"); +} + /* VB2 context initialization helper function */ static void init_ctx(struct vb2_context *ctx) { @@ -197,11 +223,11 @@ int rv; uint32_t size = 0;
- printk(BIOS_INFO, "Phase 4\n"); + printk(BIOS_INFO, "Phase 4\n");
/* init hash */ rv = vb21api_init_hash(ctx, id, &size); - if (rv) + if (rv) return rv;
/* extend hash over the body */ @@ -237,28 +263,23 @@ vboot_set_selected_region(region_device_region(&fw_main)); }
-/* Veify the stage to be executed */ -static void verify_stage(const struct region_device *rdev) +/* Verify the stage to be executed */ +static void verify_stage(const struct region_device *rdev, + const char *name) { struct vb2_context ctx; struct region_device fw_main; int rv; size_t fsize = 0; void *map = NULL; - const struct vb2_id* id; + struct vb2_id id;
/* get region memory map */ fsize = region_device_sz(rdev); map = rdev_mmap(rdev, 0, fsize); if (!map) die("ERROR: Stage Mapping failed");
- /* get the hash id */ - if (ENV_POSTCAR) - id = vb2_hash_id(VB2_HASH_SHA256); - else if (ENV_RAMSTAGE) - id = vb2_hash_id(VB2_HASH_SHA512); - else - die("Invalid hash id"); + get_hash_id(&id, name);
/* initialize the vb context and read the NV data */ init_ctx(&ctx); @@ -277,7 +298,7 @@ }
/* verify the hash */ - rv = verify_hash(&ctx, map, id); + rv = verify_hash(&ctx, map, &id); if (rv) { printk(BIOS_ERR, "ERROR:0x%x ", rv); die("Stage Verification Failed"); @@ -289,13 +310,14 @@ }
/* stage verification if required */ -void verify_stage_if_required(const struct region_device *rdev) +void verify_stage_if_required(const struct region_device *rdev, + const char *name) { if (!rdev) { die("Invalid region device"); } else { if (ENV_POSTCAR || ENV_RAMSTAGE) - verify_stage(rdev); + verify_stage(rdev, name); } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32156 )
Change subject: src/security/vboot: Added logic to also verify FSP_S component and syntax change for verify_stage_if_required. ......................................................................
Patch Set 1:
(33 comments)
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c File src/security/vboot/vboot_logic_ex.c:
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 162: if (ENV_POSTCAR) { code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 162: if (ENV_POSTCAR) { please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 163: const struct vb2_id tmp_id = VB2_ID_RAMSTAGE; code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 163: const struct vb2_id tmp_id = VB2_ID_RAMSTAGE; please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 164: memcpy(id, &tmp_id, sizeof(*id)); code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 164: memcpy(id, &tmp_id, sizeof(*id)); please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 165: } else if (ENV_RAMSTAGE) { code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 165: } else if (ENV_RAMSTAGE) { please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 168: if (!memcmp(name, CONFIG_CBFS_PREFIX"/payload", code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 168: if (!memcmp(name, CONFIG_CBFS_PREFIX"/payload", please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 170: const struct vb2_id tmp_id = VB2_ID_PAYLOAD; code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 170: const struct vb2_id tmp_id = VB2_ID_PAYLOAD; please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 171: memcpy(id, &tmp_id, sizeof(*id)); code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 171: memcpy(id, &tmp_id, sizeof(*id)); please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 172: } else if (!memcmp(name, CONFIG_FSP_S_CBFS, code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 172: } else if (!memcmp(name, CONFIG_FSP_S_CBFS, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 174: const struct vb2_id tmp_id = VB2_ID_FSPS; code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 174: const struct vb2_id tmp_id = VB2_ID_FSPS; please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 175: memcpy(id, &tmp_id, sizeof(*id)); code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 175: memcpy(id, &tmp_id, sizeof(*id)); please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 176: } code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 176: } please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 177: else code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 177: else please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 177: else else should follow close brace '}'
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 178: die("Invalid component"); code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 178: die("Invalid component"); please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 179: } else code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 179: } else please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 180: die("Invalid stage"); code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 180: die("Invalid stage"); please, no spaces at the start of a line
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 301: rv = verify_hash(&ctx, map, &id); code indent should use tabs where possible
https://review.coreboot.org/#/c/32156/1/src/security/vboot/vboot_logic_ex.c@... PS1, Line 301: rv = verify_hash(&ctx, map, &id); please, no spaces at the start of a line
Hello Aaron Durbin, Subrata Banik, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32156
to look at the new patch set (#2).
Change subject: src/security/vboot: Added logic to also verify FSP_S component and syntax change for verify_stage_if_required. ......................................................................
src/security/vboot: Added logic to also verify FSP_S component and syntax change for verify_stage_if_required.
When VBOOT Stage Verification is enabled, FSP_S component needs to be verified. This logic has been added.
TEST=Create a coreboot.rom image by enabling CONFIG_VBOOT and CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots to authenticated payload and graphics is displayed via HDMI and Display Port.
Change-Id: I7e2323086ecddc5195d8b55b47cc71f599b5a0b8 Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com --- M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc M src/security/vboot/vboot_logic_ex.c 3 files changed, 57 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/32156/2
Julius Werner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32156 )
Change subject: src/security/vboot: Added logic to also verify FSP_S component and syntax change for verify_stage_if_required. ......................................................................
Abandoned
This direction of development was abandoned and instead the CONFIG_CBFS_VERIFICATION effort is intended to solve this use case. See CB:32159 for original discussion.
-
Amol N Sukerkar (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)