Change in ...coreboot[master]: src/security/vboot: Changed the logic to verify a stage after it has ...

[Amol N Sukerkar (Code Review)]

Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32152

Change subject: src/security/vboot: Changed the logic to verify a stage after it has been loaded into DRAM
......................................................................

src/security/vboot: Changed the logic to verify a stage after it has been loaded into DRAM

This feature enables VBOOT_STAGE_VERIFICATION logic to make use of
function prototype made available by Coreboot to verify a stage after
it has been loaded into DRAM

TEST=Create a coreboot.rom image which has keyblock and VBLOCK
     with VBOOT version 2.1 structures. This is done by enabling
     CONFIG_VBOOT_STAGE_VERIFICATION. Verify that the image boots
     to authenticated payload.

Change-Id: I0381299f97d0b59969e2d6c6b4df4e4cc3e39f69
Signed-off-by: Sukerkar, Amol N 
---
M src/security/vboot/vboot_loader.c
M src/security/vboot/vboot_logic_ex.c
2 files changed, 25 insertions(+), 55 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/32152/1

diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c
index e09a314..b71178e 100644
--- a/src/security/vboot/vboot_loader.c
+++ b/src/security/vboot/vboot_loader.c
@@ -61,16 +61,6 @@
    	return 0;
 }

-/* This is the helper function that decides when the stage verification
- * code should be called. */
-static int stage_verification_should_run(void)
-{
-	if (IS_ENABLED(CONFIG_VBOOT_STAGE_VERIFICATION))
-		return ENV_POSTCAR | ENV_RAMSTAGE;
-
-	return 0;
-}
-
 static int vboot_executed CAR_GLOBAL;

int vboot_logic_executed(void)
@@ -97,11 +87,9 @@

static void vboot_prepare(void)
 {
-	if (verification_should_run() ||
-		stage_verification_should_run()) {
+	if (verification_should_run()) {
    	/* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */
    	verstage_main();
-
    	car_set_var(vboot_executed, 1);
    	vboot_save_recovery_reason_vbnv();
    } else if (verstage_should_load()) {
diff --git a/src/security/vboot/vboot_logic_ex.c b/src/security/vboot/vboot_logic_ex.c
index 7a735a9..1b526c7 100644
--- a/src/security/vboot/vboot_logic_ex.c
+++ b/src/security/vboot/vboot_logic_ex.c
@@ -238,55 +238,28 @@
 }

/* Veify the stage to be executed */
-static void verify_stage(void)
+static void verify_stage(const struct region_device *rdev)
 {
    struct vb2_context ctx;
         struct region_device fw_main;
         int rv;
-        const struct region_device *fh = NULL;
         size_t fsize = 0;
         void *map = NULL;
-        struct cbfsf file;
    const struct vb2_id* id;

-	/* For each stage to be verified, extract map and
-	 * hashing algo */
-	if (ENV_POSTCAR) {
-		printk(BIOS_INFO, "Verify ramstage\n");
+	/* 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);
-		struct prog stage = PROG_INIT(PROG_RAMSTAGE,
-					CONFIG_CBFS_PREFIX "/ramstage");
+	else if (ENV_RAMSTAGE)
+		id = vb2_hash_id(VB2_HASH_SHA512);
+	else
+		die("Invalid hash id");

-		/* load stage */
-        	if (cbfs_boot_locate(&file, prog_name(&stage), NULL))
-                	die("failed to load stage");
-
-        	cbfs_file_data(prog_rdev(&stage), &file);
-        	fh = &stage.rdev;
-
-        	fsize = region_device_sz(fh);
-        	map = rdev_mmap(fh, 0, fsize);
-        	if (!map) printk(BIOS_INFO, "ERROR: Mapping failed\n");
-	} else if (ENV_RAMSTAGE) {
-		printk(BIOS_INFO, "Verify payload\n");
-                id = vb2_hash_id(VB2_HASH_SHA512);
-		struct prog stage = PROG_INIT(PROG_PAYLOAD,
-					CONFIG_CBFS_PREFIX "/payload");
-
-		/* load stage */
-        	if (cbfs_boot_locate(&file, prog_name(&stage), NULL))
-                	die("failed to load stage");
-
-        	cbfs_file_data(prog_rdev(&stage), &file);
-        	fh = &stage.rdev;
-
-        	fsize = region_device_sz(fh);
-        	map = rdev_mmap(fh, 0, fsize);
-        	if (!map) printk(BIOS_INFO, "ERROR: Mapping failed\n");
-	} else
-		die("Impossible");
-
-	//get_stage_attr(&map, &id);
    /* initialize the vb context and read the NV data */
         init_ctx(&ctx);

@@ -310,17 +283,26 @@
                 die("Stage Verification Failed");
         }

-        rdev_munmap(fh, map);
+        rdev_munmap(rdev, map);

printk(BIOS_INFO, "stage verified successfully, proceed...\n");
 }

+/* stage verification if required */
+void verify_stage_if_required(const struct region_device *rdev)
+{
+	if (!rdev) {
+		die("Invalid region device");
+	} else 	{
+		if (ENV_POSTCAR || ENV_RAMSTAGE)
+			verify_stage(rdev);
+	}
+}
+
 /* Main Entry Point for Stage Verification */
 void verstage_main(void)
 {
    if (ENV_VERSTAGE)
    	init_ctx_verstage();
-	else if (ENV_POSTCAR || ENV_RAMSTAGE)
-		verify_stage();
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32152 )

Change subject: src/security/vboot: Changed the logic to verify a stage after it has been loaded into DRAM
......................................................................

Patch Set 1:

(6 comments)

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c 
File src/security/vboot/vboot_logic_ex.c:

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c@... 
PS1, Line 253:         if (!map) die("ERROR: Stage Mapping failed");
code indent should use tabs where possible

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c@... 
PS1, Line 253:         if (!map) die("ERROR: Stage Mapping failed");
please, no spaces at the start of a line

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c@... 
PS1, Line 253:         if (!map) die("ERROR: Stage Mapping failed");
trailing statements should be on next line

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c@... 
PS1, Line 286:         rdev_munmap(rdev, map);
code indent should use tabs where possible

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c@... 
PS1, Line 286:         rdev_munmap(rdev, map);
please, no spaces at the start of a line

https://review.coreboot.org/#/c/32152/1/src/security/vboot/vboot_logic_ex.c@... 
PS1, Line 296: 	} else 	{
please, no space before tabs

Change in coreboot[master]: src/security/vboot: Changed the logic to verify a stage after it has ...

[Julius Werner (Code Review)]

Julius Werner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32152 )

Change subject: src/security/vboot: Changed the logic to verify a stage after it has been loaded into DRAM
......................................................................

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.