vboot2: tpm error doesn't block gbb dev flag

In recovery mode, the TPM may be bad / corrupt.  This prevents access to
the soft developer switch stored in secdata.  But it should not prevent
setting dev mode via GBB or context flags.  Those flags may be set
during manufacturing or testing, and override the contents of secdata
anyway.

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

Change-Id: I242714528203cc7cf78a714c660b7f8bbd0e04d0
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/300621
Commit-Ready: Furquan Shaikh <furquan@chromium.org>
Reviewed-by: Furquan Shaikh <furquan@chromium.org>

4 files changed, 153 insertions, 64 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/firmware/2lib/2misc.c b/firmware/2lib/2misc.c
index 5805093b..4b30a5c6 100644
--- a/firmware/2lib/2misc.c
+++ b/firmware/2lib/2misc.c
@@ -204,33 +204,45 @@ int vb2_fw_parse_gbb(struct vb2_context *ctx)
 int vb2_check_dev_switch(struct vb2_context *ctx)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
-	uint32_t flags;
+	uint32_t flags = 0;
 	uint32_t old_flags;
 	int is_dev = 0;
+	int use_secdata = 1;
 	int rv;
 
 	/* Read secure flags */
 	rv = vb2_secdata_get(ctx, VB2_SECDATA_FLAGS, &flags);
-	if (rv)
-		return rv;
-
+	if (rv) {
+		if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) {
+			/*
+			 * Recovery mode needs to check other ways developer
+			 * mode can be enabled, so don't give up yet.  But
+			 * since we can't read secdata, assume dev mode was
+			 * disabled.
+			 */
+			use_secdata = 0;
+			flags = 0;
+		} else {
+			/* Normal mode simply fails */
+			return rv;
+		}
+	}
 	old_flags = flags;
 
 	/* Handle dev disable request */
-	if (vb2_nv_get(ctx, VB2_NV_DISABLE_DEV_REQUEST)) {
+	if (use_secdata && vb2_nv_get(ctx, VB2_NV_DISABLE_DEV_REQUEST)) {
 		flags &= ~VB2_SECDATA_FLAG_DEV_MODE;
 
 		/* Clear the request */
 		vb2_nv_set(ctx, VB2_NV_DISABLE_DEV_REQUEST, 0);
 	}
 
-	if (ctx->flags & VB2_DISABLE_DEVELOPER_MODE) {
-		/*
-		 * Hardware switch and GBB flag will take precedence over
-		 * this.
-		 */
+	/*
+	 * Check if we've been asked by the caller to disable dev mode.  Note
+	 * that hardware switch and GBB flag will take precedence over this.
+	 */
+	if (ctx->flags & VB2_DISABLE_DEVELOPER_MODE)
 		flags &= ~VB2_SECDATA_FLAG_DEV_MODE;
-	}
 
 	/* Check virtual dev switch */
 	if (flags & VB2_SECDATA_FLAG_DEV_MODE)
@@ -276,22 +288,31 @@ int vb2_check_dev_switch(struct vb2_context *ctx)
 		 * done here instead of simply passing a flag to
 		 * vb2_check_tpm_clear(), because we don't want to update
 		 * last_boot_developer and then fail to clear the TPM owner.
+		 *
+		 * Note that we do this even if we couldn't read secdata, since
+		 * the TPM owner and secdata may be independent, and we want
+		 * the owner to be cleared if *this boot* is different than the
+		 * last one (perhaps due to GBB or hardware override).
 		 */
 		rv = vb2ex_tpm_clear_owner(ctx);
-		if (rv) {
-			/*
-			 * Note that this truncates rv to 8 bit.  Which is not
-			 * as useful as the full error code, but we don't have
-			 * NVRAM space to store the full 32-bit code.
-			 */
-			vb2_fail(ctx, VB2_RECOVERY_TPM_CLEAR_OWNER, rv);
-			return rv;
+		if (use_secdata) {
+			/* Check for failure to clear owner */
+			if (rv) {
+				/*
+				 * Note that this truncates rv to 8 bit.  Which
+				 * is not as useful as the full error code, but
+				 * we don't have NVRAM space to store the full
+				 * 32-bit code.
+				 */
+				vb2_fail(ctx, VB2_RECOVERY_TPM_CLEAR_OWNER, rv);
+				return rv;
+			}
+
+			/* Save new flags */
+			rv = vb2_secdata_set(ctx, VB2_SECDATA_FLAGS, flags);
+			if (rv)
+				return rv;
 		}
-
-		/* Save new flags */
-		rv = vb2_secdata_set(ctx, VB2_SECDATA_FLAGS, flags);
-		if (rv)
-			return rv;
 	}
 
 	return VB2_SUCCESS;
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;
diff --git a/tests/vb2_misc_tests.c b/tests/vb2_misc_tests.c
index 3346f20f..8be5ae3a 100644
--- a/tests/vb2_misc_tests.c
+++ b/tests/vb2_misc_tests.c
@@ -290,9 +290,9 @@ static void dev_switch_tests(void)
 	/* Normal mode */
 	reset_common_data();
 	TEST_SUCC(vb2_check_dev_switch(&cc), "dev mode off");
-	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "sd not in dev");
-	TEST_EQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "ctx not in dev");
-	TEST_EQ(mock_tpm_clear_called, 0, "no tpm clear");
+	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd not in dev");
+	TEST_EQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx not in dev");
+	TEST_EQ(mock_tpm_clear_called, 0, "  no tpm clear");
 
 	/* Dev mode */
 	reset_common_data();
@@ -300,9 +300,9 @@ static void dev_switch_tests(void)
 			(VB2_SECDATA_FLAG_DEV_MODE |
 			 VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER));
 	TEST_SUCC(vb2_check_dev_switch(&cc), "dev mode on");
-	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "sd in dev");
-	TEST_NEQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "ctx in dev");
-	TEST_EQ(mock_tpm_clear_called, 0, "no tpm clear");
+	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd in dev");
+	TEST_NEQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx in dev");
+	TEST_EQ(mock_tpm_clear_called, 0, "  no tpm clear");
 
 	/* Any normal mode boot clears dev boot flags */
 	reset_common_data();
@@ -313,34 +313,34 @@ static void dev_switch_tests(void)
 	vb2_nv_set(&cc, VB2_NV_FASTBOOT_UNLOCK_IN_FW, 1);
 	TEST_SUCC(vb2_check_dev_switch(&cc), "dev mode off");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_DEV_BOOT_USB),
-		0, "cleared dev boot usb");
+		0, "  cleared dev boot usb");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_DEV_BOOT_LEGACY),
-		0, "cleared dev boot legacy");
+		0, "  cleared dev boot legacy");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_DEV_BOOT_SIGNED_ONLY),
-		0, "cleared dev boot signed only");
+		0, "  cleared dev boot signed only");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_DEV_BOOT_FASTBOOT_FULL_CAP),
-		0, "cleared dev boot fastboot full cap");
+		0, "  cleared dev boot fastboot full cap");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_FASTBOOT_UNLOCK_IN_FW),
-		0, "cleared dev boot fastboot unlock in fw");
+		0, "  cleared dev boot fastboot unlock in fw");
 
 	/* Normal-dev transition clears TPM */
 	reset_common_data();
 	vb2_secdata_set(&cc, VB2_SECDATA_FLAGS, VB2_SECDATA_FLAG_DEV_MODE);
 	TEST_SUCC(vb2_check_dev_switch(&cc), "to dev mode");
-	TEST_EQ(mock_tpm_clear_called, 1, "tpm clear");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
 	vb2_secdata_get(&cc, VB2_SECDATA_FLAGS, &v);
 	TEST_EQ(v, (VB2_SECDATA_FLAG_DEV_MODE |
 		    VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER),
-		"last boot developer now");
+		"  last boot developer now");
 
 	/* Dev-normal transition clears TPM too */
 	reset_common_data();
 	vb2_secdata_set(&cc, VB2_SECDATA_FLAGS,
 			VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER);
 	TEST_SUCC(vb2_check_dev_switch(&cc), "from dev mode");
-	TEST_EQ(mock_tpm_clear_called, 1, "tpm clear");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
 	vb2_secdata_get(&cc, VB2_SECDATA_FLAGS, &v);
-	TEST_EQ(v, 0, "last boot not developer now");
+	TEST_EQ(v, 0, "  last boot not developer now");
 
 	/* Disable dev mode */
 	reset_common_data();
@@ -349,29 +349,29 @@ static void dev_switch_tests(void)
 			 VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER));
 	vb2_nv_set(&cc, VB2_NV_DISABLE_DEV_REQUEST, 1);
 	TEST_SUCC(vb2_check_dev_switch(&cc), "disable dev request");
-	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "sd not in dev");
+	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd not in dev");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_DISABLE_DEV_REQUEST),
-		0, "request cleared");
+		0, "  request cleared");
 
-	/* Force enabled by gbb */
+	/* Force enabled by GBB */
 	reset_common_data();
 	sd->gbb_flags |= VB2_GBB_FLAG_FORCE_DEV_SWITCH_ON;
 	TEST_SUCC(vb2_check_dev_switch(&cc), "dev on via gbb");
-	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "sd in dev");
+	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd in dev");
 	vb2_secdata_get(&cc, VB2_SECDATA_FLAGS, &v);
 	TEST_EQ(v, VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER,
-		"doesn't set dev on in secdata but does set last boot dev");
-	TEST_EQ(mock_tpm_clear_called, 1, "tpm clear");
+		"  doesn't set dev on in secdata but does set last boot dev");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
 
 	/* Force enabled by ctx flag */
 	reset_common_data();
 	cc.flags |= VB2_CONTEXT_FORCE_DEVELOPER_MODE;
 	TEST_SUCC(vb2_check_dev_switch(&cc), "dev on via ctx flag");
-	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "sd in dev");
+	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd in dev");
 	vb2_secdata_get(&cc, VB2_SECDATA_FLAGS, &v);
 	TEST_EQ(v, VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER,
-		"doesn't set dev on in secdata but does set last boot dev");
-	TEST_EQ(mock_tpm_clear_called, 1, "tpm clear");
+		"  doesn't set dev on in secdata but does set last boot dev");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
 
 	/* Simulate clear owner failure */
 	reset_common_data();
@@ -380,14 +380,65 @@ static void dev_switch_tests(void)
 	mock_tpm_clear_retval = VB2_ERROR_EX_TPM_CLEAR_OWNER;
 	TEST_EQ(vb2_check_dev_switch(&cc),
 		VB2_ERROR_EX_TPM_CLEAR_OWNER, "tpm clear fail");
-	TEST_EQ(mock_tpm_clear_called, 1, "tpm clear");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
 	vb2_secdata_get(&cc, VB2_SECDATA_FLAGS, &v);
 	TEST_EQ(v, VB2_SECDATA_FLAG_LAST_BOOT_DEVELOPER,
-		"last boot still developer");
+		"  last boot still developer");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_RECOVERY_REQUEST),
-		VB2_RECOVERY_TPM_CLEAR_OWNER, "requests recovery");
+		VB2_RECOVERY_TPM_CLEAR_OWNER, "  requests recovery");
 	TEST_EQ(vb2_nv_get(&cc, VB2_NV_RECOVERY_SUBCODE),
-		(uint8_t)VB2_ERROR_EX_TPM_CLEAR_OWNER, "recovery subcode");
+		(uint8_t)VB2_ERROR_EX_TPM_CLEAR_OWNER, "  recovery subcode");
+
+	/*
+	 * Secdata failure in normal mode fails and shows dev=0 even if dev
+	 * mode was on in the (inaccessible) secdata.
+	 */
+	reset_common_data();
+	vb2_secdata_set(&cc, VB2_SECDATA_FLAGS, VB2_SECDATA_FLAG_DEV_MODE);
+	sd->status &= ~VB2_SD_STATUS_SECDATA_INIT;
+	TEST_EQ(vb2_check_dev_switch(&cc), VB2_ERROR_SECDATA_GET_UNINITIALIZED,
+		"secdata fail normal");
+	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd not in dev");
+	TEST_EQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx not in dev");
+
+	/* Secdata failure in recovery mode continues */
+	reset_common_data();
+	cc.flags |= VB2_CONTEXT_RECOVERY_MODE;
+	sd->status &= ~VB2_SD_STATUS_SECDATA_INIT;
+	TEST_SUCC(vb2_check_dev_switch(&cc), "secdata fail recovery");
+	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd not in dev");
+	TEST_EQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx not in dev");
+
+	/* And doesn't check or clear dev disable request */
+	reset_common_data();
+	cc.flags |= VB2_CONTEXT_RECOVERY_MODE;
+	sd->status &= ~VB2_SD_STATUS_SECDATA_INIT;
+	vb2_nv_set(&cc, VB2_NV_DISABLE_DEV_REQUEST, 1);
+	TEST_SUCC(vb2_check_dev_switch(&cc), "secdata fail recovery disable");
+	TEST_EQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd not in dev");
+	TEST_EQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx not in dev");
+	TEST_EQ(vb2_nv_get(&cc, VB2_NV_DISABLE_DEV_REQUEST),
+		1, "  request not cleared");
+
+	/* Can still override with GBB flag */
+	reset_common_data();
+	cc.flags |= VB2_CONTEXT_RECOVERY_MODE;
+	sd->status &= ~VB2_SD_STATUS_SECDATA_INIT;
+	sd->gbb_flags |= VB2_GBB_FLAG_FORCE_DEV_SWITCH_ON;
+	TEST_SUCC(vb2_check_dev_switch(&cc), "secdata fail recovery gbb");
+	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd in dev");
+	TEST_NEQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx in dev");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
+
+	/* Can still override with context flag */
+	reset_common_data();
+	cc.flags |= VB2_CONTEXT_RECOVERY_MODE;
+	cc.flags |= VB2_CONTEXT_FORCE_DEVELOPER_MODE;
+	sd->status &= ~VB2_SD_STATUS_SECDATA_INIT;
+	TEST_SUCC(vb2_check_dev_switch(&cc), "secdata fail recovery ctx");
+	TEST_NEQ(sd->flags & VB2_SD_DEV_MODE_ENABLED, 0, "  sd in dev");
+	TEST_NEQ(cc.flags & VB2_CONTEXT_DEVELOPER_MODE, 0, "  ctx in dev");
+	TEST_EQ(mock_tpm_clear_called, 1, "  tpm clear");
 }
 
 static void tpm_clear_tests(void)