firmware: VB2_REC_OR_DIE() should not abort before vb2_check_recovery()

Unfortunately, CL:3168437 introduced a new problem when booting with a
broken TPM: secdata accessors no longer return failure but instead just
abort when booting in normal mode and continue when we're in recovery
mode. The problem is that when accessing secdata very early in
vb2api_fw_phase1(), we have not decided whether we're booting in
recovery mode yet. If vb2_secdata_firmware_init() fails, we will call
vb2api_fail() and then continue knowing that vb2_check_recovery() will
later see the recovery reason in NVRAM and decide to boot directly into
recovery from here. But if the code in-between accesses secdata, the
VB2_CONTEXT_RECOVERY_MODE flag is technically not yet set, so our
secdata accessor thinks we are booting in normal mode and something
terrible happened (because it shouldn't be possible to boot in normal
mode when secdata_init failed), so it aborts.

In order to try to solve this problem in a more general way, introduce a
new VB2_SD_STATUS_RECOVERY_DECIDED status flag that gets set once we
reach the point where we have conclusively decided whether we are
booting into recovery mode and set the appropriate context flags. Any
code using VB2_REC_OR_DIE() before that point will play it safe and
assume that we may still go into recovery mode, so we shouldn't abort.

BRANCH=none
BUG=none
TEST=none

Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: Ic3daa8dac932286257cbceebfff8712d25c3a97a
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3301540
Reviewed-by: Hsuan Ting Chen <roccochen@chromium.org>
Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>

1 files changed, 25 insertions, 3 deletions

diff --git a/tests/vb2_misc_tests.c b/tests/vb2_misc_tests.c
index d7afeb68..c552301e 100644
--- a/tests/vb2_misc_tests.c
+++ b/tests/vb2_misc_tests.c
@@ -40,7 +40,7 @@ static void reset_common_data(void)
 		  "vb2api_init failed");
 
 	sd = vb2_get_sd(ctx);
-	sd->status = VB2_SD_STATUS_SECDATA_FWMP_INIT;
+	sd->status |= VB2_SD_STATUS_SECDATA_FWMP_INIT;
 
 	memset(&gbb, 0, sizeof(gbb));
 
@@ -268,12 +268,17 @@ static void misc_tests(void)
 		"vb_workbuf_from_ctx() size");
 
 	reset_common_data();
+	sd->status |= VB2_SD_STATUS_RECOVERY_DECIDED;
 	TEST_ABORT(VB2_REC_OR_DIE(ctx, "die\n"), "REC_OR_DIE in normal mode");
 
 	reset_common_data();
+	sd->status |= VB2_SD_STATUS_RECOVERY_DECIDED;
 	ctx->flags |= VB2_CONTEXT_RECOVERY_MODE;
 	VB2_REC_OR_DIE(ctx, "VB2_REC_OR_DIE() test in recovery mode\n");
 	/* Would exit here if it didn't work as intended. */
+
+	reset_common_data();
+	VB2_REC_OR_DIE(ctx, "VB2_REC_OR_DIE() test in fw_phase1\n");
 }
 
 static void gbb_tests(void)
@@ -421,12 +426,16 @@ static void recovery_tests(void)
 {
 	/* No recovery */
 	reset_common_data();
+	TEST_EQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		0, "recovery not yet decided before testing check_recovery()");
 	vb2_check_recovery(ctx);
 	TEST_EQ(sd->recovery_reason, 0, "No recovery reason");
 	TEST_EQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		0, "Not manual recovery");
 	TEST_EQ(ctx->flags & VB2_CONTEXT_RECOVERY_MODE,
 		0, "Not recovery mode");
+	TEST_NEQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		 0, "Recovery decided");
 
 	/* From request */
 	reset_common_data();
@@ -438,6 +447,8 @@ static void recovery_tests(void)
 		0, "Not manual recovery");
 	TEST_NEQ(ctx->flags & VB2_CONTEXT_RECOVERY_MODE,
 		 0, "Recovery mode");
+	TEST_NEQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		 0, "Recovery decided");
 
 	/* From request, but already failed */
 	reset_common_data();
@@ -447,6 +458,8 @@ static void recovery_tests(void)
 	TEST_EQ(sd->recovery_reason, 5, "Recovery reason already failed");
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
 		4, "NV not cleared");
+	TEST_NEQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		 0, "Recovery decided");
 
 	/* Override */
 	reset_common_data();
@@ -457,6 +470,8 @@ static void recovery_tests(void)
 		"Recovery reason forced");
 	TEST_NEQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		 0, "SD flag set");
+	TEST_NEQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		 0, "Recovery decided");
 
 	/* Override subcode TRAIN_AND_REBOOT */
 	reset_common_data();
@@ -467,6 +482,8 @@ static void recovery_tests(void)
 		"Recovery reason forced");
 	TEST_NEQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		 0, "SD flag set");
+	TEST_NEQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		 0, "Recovery decided");
 
 	/* Promote subcode from BROKEN screen*/
 	reset_common_data();
@@ -477,6 +494,8 @@ static void recovery_tests(void)
 		"Recovery reason forced from BROKEN");
 	TEST_NEQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		 0, "SD flag set");
+	TEST_NEQ(sd->status & VB2_SD_STATUS_RECOVERY_DECIDED,
+		 0, "Recovery decided");
 }
 
 static void dev_switch_tests(void)
@@ -589,13 +608,15 @@ static void dev_switch_tests(void)
 
 	/*
 	 * secdata_firmware failure in normal mode fails and shows dev=0 even
-	 * if dev mode was on in the (inaccessible) secdata_firmware.
+	 * if dev mode was on in the (inaccessible) secdata_firmware. Since this
+	 * happens in fw_phase1, we do not abort -- we know that when secdata
+	 * is uninitialized here, we must be headed for recovery mode.
 	 */
 	reset_common_data();
 	vb2_secdata_firmware_set(ctx, VB2_SECDATA_FIRMWARE_FLAGS,
 				 VB2_SECDATA_FIRMWARE_FLAG_DEV_MODE);
 	sd->status &= ~VB2_SD_STATUS_SECDATA_FIRMWARE_INIT;
-	TEST_ABORT(vb2_check_dev_switch(ctx), "secdata_firmware fail normal");
+	TEST_SUCC(vb2_check_dev_switch(ctx), "secdata_firmware fail normal");
 	TEST_EQ(sd->flags & VB2_SD_FLAG_DEV_MODE_ENABLED, 0, "  sd not in dev");
 	TEST_EQ(ctx->flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx not in dev");
 
@@ -659,6 +680,7 @@ static void enable_dev_tests(void)
 	reset_common_data();
 	allow_recovery_retval = 1;
 	sd->status &= ~VB2_SD_STATUS_SECDATA_FIRMWARE_INIT;
+	sd->status |= VB2_SD_STATUS_RECOVERY_DECIDED;
 	TEST_ABORT(vb2api_enable_developer_mode(ctx),
 		   "secdata_firmware no init, enable dev mode aborted");
 	sd->status |= VB2_SD_STATUS_SECDATA_FIRMWARE_INIT;