Change in ...coreboot[master]: security/vboot: Do not check for RW partitions if not part of the image

[Werner Zeh (Code Review)]

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31662

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

security/vboot: Do not check for RW partitions if not part of the image

In the setup where measured boot is used with read-only partition only
there is no RW_A or RW_B partition in the flash. In this case it makes
no sense to let VBOOT check for these partitions just to fail and then
fall back to recovery mode.
Instead set the flag VB2_CONTEXT_RECOVERY_MODE right away so that VBOOT
starts in recovery mode any time.
This kind of bypasses VBOOT logic but is still suitable to have a
pure measured boot scheme enabled. In addition it avoids the first two
reboots due to missing RW_A and RW_B.

Change-Id: I07b8ec97be7db63b7ccddb3f33e0f741bed8acd8
Signed-off-by: Werner Zeh 
---
M src/security/vboot/vboot_logic.c
1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31662/1

diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index 8c3ba80..89934b9 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -324,6 +324,12 @@
    		die("Initializing measured boot mode failed!");
    }

+	/* Skip checking for RW_A and RW_B if these partitions are not included
+	   in the image. Instead proceed with recovery mode which uses RO
+	   partition only. */
+	if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A))
+		ctx.flags |= VB2_CONTEXT_RECOVERY_MODE;
+
    if (IS_ENABLED(CONFIG_VBOOT_PHYSICAL_DEV_SWITCH) &&
    	get_developer_mode_switch())
    	ctx.flags |= VB2_CONTEXT_FORCE_DEVELOPER_MODE;

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31662 )

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1: Code-Review+1

[Aaron Durbin (Code Review)]

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

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c@330 
PS1, Line 330: 	if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A))
Can we have a  Kconfig which is more explicit?

config VBOOT_NO_RW_SLOTS
  bool
  default y if !VBOOT_SLOTS_RW_A

Then is the code we can explicitly test for CONFIG_VBOOT_NO_RW_SLOTS?

This will work but you are using recovery mode as a proxy (with implicit semantics) for your desired behavior. You can't really count on the behavior not changing in the future.

But I believe you're going down this path to only do measured boot? Do we need verified boot and verstage to do that? vboot seems like a big hammer for measured boot.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31662 )

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c@330 
PS1, Line 330: 	if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A))
> Can we have a  Kconfig which is more explicit? […]

I agree, this doesn't make sense. If this is the behavior you want, you should probably rather disentangle measured boot from vboot and make it work as a standalone feature. (Note that you can still call vboot functions -- we should change the Makefile so that the vboot library is built and linked even if CONFIG_VBOOT is not enabled -- you just wouldn't be running the main vb2api_fw_phaseX() verification code then.)

My understanding from CB:27714 was that the "RO only" mode meant that you only have a single FMAP partition, it is updateable (so it would've really been more appropriate to call it "RW only"), but it still gets verified during boot (and you just get bricked if that fails). This patch is now changing that to a scheme that doesn't make sense anymore. If you never want to verify anything, you shouldn't be executing this function.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31662 )

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c@330 
PS1, Line 330: 	if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A))
> I agree, this doesn't make sense. […]

What I actually would like to have is a RW_A only (no RW_B due to flash space constraints and even no RO) setup. Then all the checks vboot does are meaningfull. But VBOOT was not designed that way asit counts on RW_A and at least RO.
Is there a way to go that path in VBOOT? And then have measured boot enabled?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31662 )

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c@330 
PS1, Line 330: 	if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A))
> What I actually would like to have is a RW_A only (no RW_B due to flash space constraints and even n […]

I mean, a hacky but easy way to do it would be to just have coreboot present the same section as both "A" and "B" to vboot. Since we got this all abstracted out via is_slot_a(), if you just change that to always return true when VBOOT_SLOTS_RW_AB is not true, that should get you most of the way there. (There may be some oddities like when the verification fails vboot will first try "the other slot" which ends up being the same slot again before falling back to recovery, but that shouldn't really matter much.)

That should allow the VBOOT_SLOTS_RW_A-only configuration to work, but it still wouldn't work if that one is also disabled (i.e. the "RO-only" configuration which should really be called "RW-only without recovery"). In order to do that right I think we have to revisit the config and Makefile stuff set up in CB:27714 because the way it was done there doesn't make all that much sense. Rather than "disabling RW_A", I think what you really want to do is "disable recovery"... so rather than two Kconfigs for "slot A enabled" and "slot A and B enabled", you'd rather want to options that say "slot B enabled" and "recovery enabled" (implying that slot A is always enabled).

Then you would have to decide how the FMAP looks like when both slot B and recovery are disabled. I think the cleanest way would still be to have both a COREBOOT section (for the root-of-trust stuff, e.g. bootblock and verstage) and a FW_MAIN_A section (for the later stages). The only real difference to "recovery enabled" is that your COREBOOT section can be much smaller because it doesn't need to have a second copy of all the later stages. Splitting them up also allows you to write-protect the root of trust (and, honestly, using vboot without that doesn't make a lot of sense anyway).

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31662 )

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/31662/1/src/security/vboot/vboot_logic.c@330 
PS1, Line 330: 	if (!IS_ENABLED(CONFIG_VBOOT_SLOTS_RW_A))
> I mean, a hacky but easy way to do it would be to just have coreboot present the same section as bot […]

You are right Julius, we need to invest more effort to make it matching the architecture of vboot better. I will try it and push some patches once I have understood it in more detail.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31662 )

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Patch Set 1:

(3 comments)

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

https://review.coreboot.org/#/c/31662/1//COMMIT_MSG@9 
PS1, Line 9: In the setup where measured boot is used with read-only partition only
           : there is no RW_A or RW_B partition in the flash.
That sentence seems incomplete.

https://review.coreboot.org/#/c/31662/1//COMMIT_MSG@15 
PS1, Line 15: suitable to have a
            : pure measured boot scheme enabled
… suitable for an enabled measured boot scheme.

https://review.coreboot.org/#/c/31662/1//COMMIT_MSG@9 
PS1, Line 9: In the setup where measured boot is used with read-only partition only
           : there is no RW_A or RW_B partition in the flash. In this case it makes
           : no sense to let VBOOT check for these partitions just to fail and then
           : fall back to recovery mode.
           : Instead set the flag VB2_CONTEXT_RECOVERY_MODE right away so that VBOOT
           : starts in recovery mode any time.
           : This kind of bypasses VBOOT logic but is still suitable to have a
           : pure measured boot scheme enabled. In addition it avoids the first two
           : reboots due to missing RW_A and RW_B.
Please add a blank line between paragraphs.

Change in coreboot[master]: security/vboot: Do not check for RW partitions if not part of the image

[Werner Zeh (Code Review)]

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

Change subject: security/vboot: Do not check for RW partitions if not part of the image
......................................................................

Abandoned