Change in ...coreboot[master]: src/security/vboot: When VBOOT Stage Verification is enabled, ...

[Amol N Sukerkar (Code Review)]

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

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

src/security/vboot: When VBOOT Stage Verification is enabled,
                    boot ROMSTAGE and POSTCAR from Read-Only region.

When VBOOT Stage Verification is enabled, the root-of-trust is the
Read-Only image. So, move the ROMSTAGE and POSTCAR is Read-Only
region. POSTCAR triggers VBOOT Stage Authentication starting with
RAMSTAGE. RAMSTAGE authenticates PAYLOAD.

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: I6d4b7dbea62a92ca75d731c84b7c1402a207634a
Signed-off-by: Sukerkar, Amol N 
---
M src/security/vboot/Makefile.inc
M src/security/vboot/vboot_loader.c
2 files changed, 19 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32153/1

diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc
index a65b066..1a6ca9f 100644
--- a/src/security/vboot/Makefile.inc
+++ b/src/security/vboot/Makefile.inc
@@ -211,6 +211,10 @@
    $(if $(filter \
    	$(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK)),, \
    		%/romstage) \
+		$(if $(filter y,$(CONFIG_VBOOT_STAGE_VERIFICATION)), \
+			%/romstage, ) \
+		$(if $(filter y,$(CONFIG_VBOOT_STAGE_VERIFICATION)), \
+			%/postcar, ) \
    	mts \
    	%/verstage \
    	locales \
diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c
index b71178e..36f2a07 100644
--- a/src/security/vboot/vboot_loader.c
+++ b/src/security/vboot/vboot_loader.c
@@ -37,6 +37,16 @@
           CONFIG(VBOOT_SEPARATE_VERSTAGE),
           "return from verstage only makes sense for separate verstages");

+/* This helper decides if stage verification logic needs to be
+ * initiated or not. */
+static int stage_verification_should_run(void)
+{
+	if (CONFIG(VBOOT_STAGE_VERIFICATION))
+		return ENV_POSTCAR | ENV_RAMSTAGE;
+
+	return 0;
+}
+
 /* The stage loading code is compiled and entered from multiple stages. The
  * helper functions below attempt to provide more clarity on when certain
  * code should be called. */
@@ -141,6 +151,11 @@
    if (!vboot_logic_executed())
    	return -1;

+	/* Do not initiate VBOOT Stage Verification until all the
+	 * stages from RO region are loaded */
+	if (!stage_verification_should_run())
+		return -1;
+
    if (vboot_get_selected_region(&selected_region))
    	return -1;

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG@13 
PS1, Line 13: RAMSTAGE. RAMSTAGE authenticates PAYLOAD.
Where is this assumption coming from? It's not correct.

[Amol N Sukerkar (Code Review)]

Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG@13 
PS1, Line 13: RAMSTAGE. RAMSTAGE authenticates PAYLOAD.
> Where is this assumption coming from? It's not correct.

This is Intel enhancement. Here, we make use of VBOOT 2.1 libraries to verify each stage as opposed verifying the entire partition in verstage. So, when ramstage executes, it loads the payload in DRAM, then authenticates using VBOOT 2.1 library (uses VB2_ID to identify the corrsponding SHA256 hash stored in VBLOCK), authenticates it and then payload is launched. Corresponding VBOOT utilities are added into vboot_reference repo. Currently in the process of getting all the permissions so I can raise a code review there. I also added you to the documentation review. I need to fix the links so figures are visible in md file.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG@13 
PS1, Line 13: RAMSTAGE. RAMSTAGE authenticates PAYLOAD.
> This is Intel enhancement. Here, we make use of VBOOT 2. […]

Well, I'm confused by the comment that romstage and postcar are in RO. That's not a property that applies globally.

[Amol N Sukerkar (Code Review)]

Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG@13 
PS1, Line 13: RAMSTAGE. RAMSTAGE authenticates PAYLOAD.
> Well, I'm confused by the comment that romstage and postcar are in RO. […]

Once the switch CONFIG_VBOOT_STAGE_VERIFICATION is enabled, ROMSTAGE and POSTCAR will be only in RO. In src/security/vboot/Makefile.inc, lines 214-217 take care of that.

[Amol N Sukerkar (Code Review)]

Hello Aaron Durbin, Subrata Banik, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32153

to look at the new patch set (#2).

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

src/security/vboot: When VBOOT Stage Verification is enabled,
                    boot ROMSTAGE and POSTCAR from Read-Only region.

When VBOOT Stage Verification is enabled, the root-of-trust is the
Read-Only image. So, move the ROMSTAGE and POSTCAR is Read-Only
region when CONFIG_VBOOT_STAGE_VERIFICATION is enabled. In this case,
POSTCAR triggers VBOOT Stage Authentication starting with RAMSTAGE and
RAMSTAGE authenticates PAYLOAD.

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: I6d4b7dbea62a92ca75d731c84b7c1402a207634a
Signed-off-by: Sukerkar, Amol N 
---
M src/security/vboot/Makefile.inc
M src/security/vboot/vboot_loader.c
2 files changed, 19 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32153/2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG@13 
PS1, Line 13: RAMSTAGE. RAMSTAGE authenticates PAYLOAD.
> Once the switch CONFIG_VBOOT_STAGE_VERIFICATION is enabled, ROMSTAGE and POSTCAR will be only in RO. […]

Where is that assumption coming from? Why would you be making that assumption on behalf of other users?

Similarly, STAGE_VERIFICATION is extremely generic. I have no idea what the semantics are -- especially because vboot inherently verifies things.

That said, there needs to be a broader discussion and coordination aside from posting CLs. Did you bring the plan and intention up on the mailing list?

[Amol N Sukerkar (Code Review)]

Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32153/1//COMMIT_MSG@13 
PS1, Line 13: RAMSTAGE. RAMSTAGE authenticates PAYLOAD.
> Where is that assumption coming from? Why would you be making that assumption on behalf of other use […]

This is the implementation and behavior when VBOOT_STAGE_VERIFICATION switch is enabled. I agree we need to have a discussion on the forum. I will introduce the plan in the mailing list.

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 2:

This seems like a pretty huge change to the way vboot operates that is incompatible with most (all?) implementations on the last several generations of platforms. From a practical standpoint it also means that you won't be able to update core silicon init modules (e.g. MRC), which is a pretty huge change.

Judging by the test case, this seems to have something to do with graphics. What is the requirement here? If there is some sort of graphics/display ACM, then you might want to look into some of the measured launch support that Philipp (cc'd) has added for TXT.

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32153 )

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

Patch Set 2:
> Patch Set 2:
>
> This seems like a pretty huge change to the way vboot operates that is incompatible with most (all?) implementations on the last several generations of platforms. From a practical standpoint it also means that you won't be able to update core silicon init modules (e.g. MRC), which is a pretty huge change.
>
> Judging by the test case, this seems to have something to do with graphics. What is the requirement here? If there is some sort of graphics/display ACM, then you might want to look into some of the measured launch support that Philipp (cc'd) has added for TXT.

Oops, I forgot to point at Philipp's measured boot patch: https://review.coreboot.org/c/coreboot/+/29547

Change in coreboot[master]: src/security/vboot: When VBOOT Stage Verification is enabled, ...

[Julius Werner (Code Review)]

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

Change subject: src/security/vboot: When VBOOT Stage Verification is enabled,                     boot ROMSTAGE and POSTCAR from Read-Only region.
......................................................................

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.