vboot2: Fix issues with check_dev_switch

check_dev_switch decides whether dev-mode is enabled based on GBB
flags in case of TPM errors. However, the check for recovery is done
after check_dev_switch is called. This leads to recovery mode equating
to false always in check_dev_switch. Instead move the call to
check_for_recovery before check_dev_switch.

Update the unit tests accordingly.

[Reference: Based on patchset # 2 :
https://chromium-review.googlesource.com/#/c/300621/1..2]

BUG=chrome-os-partner:45511
BRANCH=ryu
TEST=make runtests

Change-Id: Idd87f389e94f66a0d617c13386386061f86e45aa
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Signed-off-by: Furquan Shaikh <furquan@google.com>
Reviewed-on: https://chromium-review.googlesource.com/301442
Trybot-Ready: Furquan Shaikh <furquan@chromium.org>
Tested-by: Furquan Shaikh <furquan@chromium.org>
Reviewed-by: Furquan Shaikh <furquan@chromium.org>
Commit-Queue: Furquan Shaikh <furquan@chromium.org>

2 files changed, 29 insertions, 12 deletions

diff --git a/firmware/2lib/2api.c b/firmware/2lib/2api.c
index a7b41d2b..7f12d22c 100644
--- a/firmware/2lib/2api.c
+++ b/firmware/2lib/2api.c
@@ -75,22 +75,32 @@ int vb2api_fw_phase1(struct vb2_context *ctx)
 	if (rv)
 		vb2_fail(ctx, VB2_RECOVERY_GBB_HEADER, rv);
 
-	/* Check for dev switch */
-	rv = vb2_check_dev_switch(ctx);
-	if (rv)
-		vb2_fail(ctx, VB2_RECOVERY_DEV_SWITCH, rv);
-
 	/*
-	 * Check for recovery.  Note that this function returns void, since
-	 * any errors result in requesting recovery.
+	 * Check for recovery.  Note that this function returns void, since any
+	 * errors result in requesting recovery.  That's also why we don't
+	 * return error from failures in the preceding two steps; those
+	 * failures simply cause us to detect recovery mode here.
 	 */
 	vb2_check_recovery(ctx);
 
+	/* Check for dev switch */
+	rv = vb2_check_dev_switch(ctx);
+	if (rv && !(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) {
+		/*
+		 * Error in dev switch processing, and we weren't already
+		 * headed for recovery mode.  Reboot into recovery mode, since
+		 * it's too late to handle those errors this boot, and we need
+		 * to take a different path through the dev switch checking
+		 * code in that case.
+		 */
+		vb2_fail(ctx, VB2_RECOVERY_DEV_SWITCH, rv);
+		return rv;
+	}
+
 	/* Return error if recovery is needed */
 	if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) {
 		/* Always clear RAM when entering recovery mode */
 		ctx->flags |= VB2_CONTEXT_CLEAR_RAM;
-
 		return VB2_ERROR_API_PHASE1_RECOVERY;
 	}
 
diff --git a/tests/vb2_api_tests.c b/tests/vb2_api_tests.c
index adc536b6..49af6fd1 100644
--- a/tests/vb2_api_tests.c
+++ b/tests/vb2_api_tests.c
@@ -131,14 +131,21 @@ static void phase1_tests(void)
 	TEST_NEQ(cc.flags & VB2_CONTEXT_RECOVERY_MODE, 0, "  recovery flag");
 	TEST_NEQ(cc.flags & VB2_CONTEXT_CLEAR_RAM, 0, "  clear ram flag");
 
+	/* Dev switch error in normal mode reboots to recovery */
 	reset_common_data(FOR_MISC);
 	retval_vb2_check_dev_switch = VB2_ERROR_MOCK;
+	TEST_EQ(vb2api_fw_phase1(&cc), VB2_ERROR_MOCK, "phase1 dev switch");
+	TEST_EQ(vb2_nv_get(&cc, VB2_NV_RECOVERY_REQUEST),
+		VB2_RECOVERY_DEV_SWITCH, "  recovery request");
+
+	/* Dev switch error already in recovery mode just proceeds */
+	reset_common_data(FOR_MISC);
+	vb2_nv_set(&cc, VB2_NV_RECOVERY_REQUEST, VB2_RECOVERY_RO_UNSPECIFIED);
+	retval_vb2_check_dev_switch = VB2_ERROR_MOCK;
 	TEST_EQ(vb2api_fw_phase1(&cc), VB2_ERROR_API_PHASE1_RECOVERY,
-		"phase1 dev switch");
-	TEST_EQ(sd->recovery_reason, VB2_RECOVERY_DEV_SWITCH,
+		"phase1 dev switch error in recovery");
+	TEST_EQ(sd->recovery_reason, VB2_RECOVERY_RO_UNSPECIFIED,
 		"  recovery reason");
-	TEST_NEQ(cc.flags & VB2_CONTEXT_RECOVERY_MODE, 0, "  recovery flag");
-	TEST_NEQ(cc.flags & VB2_CONTEXT_CLEAR_RAM, 0, "  clear ram flag");
 
 	reset_common_data(FOR_MISC);
 	cc.secdata[0] ^= 0x42;