Check EC_IN_RW before proceeding to recovery mode

Depthcharge currently asks EC whether recovery was requested manually
or not without verifying EC is in RO or not. If EC-RW is compromised,
recovery switch state can be spoofed.

This patch makes Depthcharge check EC_IN_RW to determine whether EC
is in RO or not. Only if it's in RO and it says recovery button was
pressed at boot, we proceed to the recovery process.

All other recovery requests including manual recovery requested by a
(compromised) host will end up with 'broken' screen.

BUG=b:66516882
BRANCH=none
TEST=Boot Fizz. make runtests.

Change-Id: I01d2df05fe22e79bbc949f5cb83db605147667b3
Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/693008
Reviewed-by: Randall Spangler <rspangler@chromium.org>

9 files changed, 59 insertions, 140 deletions

diff --git a/firmware/lib/ec_sync.c b/firmware/lib/ec_sync.c
index 10ce5576..baf3cfc2 100644
--- a/firmware/lib/ec_sync.c
+++ b/firmware/lib/ec_sync.c
@@ -197,59 +197,16 @@ static VbError_t update_ec(struct vb2_context *ctx, int devidx,
 }
 
 /**
- * Check if the EC has the correct image active.
+ * Set IN_RW flag for a EC
  *
  * @param ctx		Vboot2 context
  * @param devidx	Which device (EC=0, PD=1)
- * @return VBERROR_SUCCESS, or non-zero if error.
  */
-static VbError_t check_ec_active(struct vb2_context *ctx, int devidx)
+static void check_ec_active(struct vb2_context *ctx, int devidx)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
-
-	/* Determine whether the EC is in RO or RW */
-	int in_rw = 0;
-	int rv = VbExEcRunningRW(devidx, &in_rw);
-	if (in_rw) {
+	if (!VbExTrustEC(devidx))
 		sd->flags |= IN_RW(devidx);
-	}
-
-	if (sd->recovery_reason) {
-		/*
-		 * Recovery mode; just verify the EC is in RO code.  Don't do
-		 * software sync, since we don't have a RW image.
-		 */
-		if (rv == VBERROR_SUCCESS && in_rw == 1) {
-			/*
-			 * EC is definitely in RW firmware.  We want it in
-			 * read-only code, so preserve the current recovery
-			 * reason and reboot.
-			 *
-			 * We don't reboot on error or unknown EC code, because
-			 * we could end up in an endless reboot loop.  If we
-			 * had some way to track that we'd already rebooted for
-			 * this reason, we could retry only once.
-			 */
-			VB2_DEBUG("want recovery but got EC-RW\n");
-			request_recovery(ctx, sd->recovery_reason);
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-		}
-
-		VB2_DEBUG("in recovery; EC-RO\n");
-		return VBERROR_SUCCESS;
-	}
-
-	/*
-	 * Not in recovery.  If we couldn't determine where the EC was,
-	 * reboot to recovery.
-	 */
-	if (rv != VBERROR_SUCCESS) {
-		VB2_DEBUG("VbExEcRunningRW() returned %d\n", rv);
-		request_recovery(ctx, VB2_RECOVERY_EC_UNKNOWN_IMAGE);
-		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-	}
-
-	return VBERROR_SUCCESS;
 }
 
 #define RO_RETRIES 2  /* Maximum times to retry flashing RO */
@@ -383,18 +340,10 @@ VbError_t ec_sync_phase1(struct vb2_context *ctx, VbCommonParams *cparams)
 	const int do_pd_sync = 0;
 #endif
 
-	/* Make sure the EC is running the correct image */
-	if (check_ec_active(ctx, 0))
-		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-	if (do_pd_sync && check_ec_active(ctx, 1))
-		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-
-	/*
-	 * In recovery mode; just verify the EC is in RO code.  Don't do
-	 * software sync, since we don't have a RW image.
-	 */
-	if (sd->recovery_reason)
-		return VBERROR_SUCCESS;
+	/* Set IN_RW flags */
+	check_ec_active(ctx, 0);
+	if (do_pd_sync)
+		check_ec_active(ctx, 1);
 
 	/* Check if we need to update RW.  Failures trigger recovery mode. */
 	if (check_ec_hash(ctx, 0, VB_SELECT_FIRMWARE_EC_ACTIVE))
diff --git a/firmware/lib/include/vboot_common.h b/firmware/lib/include/vboot_common.h
index 88bcb631..acfb58b7 100644
--- a/firmware/lib/include/vboot_common.h
+++ b/firmware/lib/include/vboot_common.h
@@ -137,4 +137,17 @@ uint64_t VbSharedDataReserve(VbSharedDataHeader *header, uint64_t size);
 int VbSharedDataSetKernelKey(VbSharedDataHeader *header,
                              const VbPublicKey *src);
 
+/**
+ * Check whether recovery is allowed or not.
+ *
+ * The only way to pass this check and proceed to the recovery process is to
+ * physically request a recovery (a.k.a. manual recovery). All other recovery
+ * requests including manual recovery requested by a (compromised) host will
+ * end up with 'broken' screen.
+ *
+ * @param flags Flags of VbSharedDataHeader.
+ * @return 1: Yes. 0: No or not sure.
+ */
+int vb2_allow_recovery(uint32_t flags);
+
 #endif  /* VBOOT_REFERENCE_VBOOT_COMMON_H_ */
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index d1afc4f7..5c54c122 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -417,27 +417,29 @@ VbError_t VbSelectAndLoadKernel(VbCommonParams *cparams,
 		goto VbSelectAndLoadKernel_exit;
 
 	/*
-	 * Do EC software sync if necessary.  This has UI, but it's just a
-	 * single non-interactive WAIT screen.
+	 * Do EC software sync unless we're in recovery mode. This has UI but
+	 * it's just a single non-interactive WAIT screen.
 	 */
-	retval = ec_sync_all(&ctx, cparams);
-	if (retval)
-		goto VbSelectAndLoadKernel_exit;
+	if (shared->recovery_reason == VB2_RECOVERY_NOT_REQUESTED) {
+		retval = ec_sync_all(&ctx, cparams);
+		if (retval)
+			goto VbSelectAndLoadKernel_exit;
+	}
 
 	/* Select boot path */
 	if (shared->recovery_reason) {
 		/* Recovery boot.  This has UI. */
-	        if (kparams->inflags & VB_SALK_INFLAGS_ENABLE_DETACHABLE_UI)
-	            retval = VbBootRecoveryMenu(&ctx, cparams);
-	        else
-		    retval = VbBootRecovery(&ctx, cparams);
+		if (kparams->inflags & VB_SALK_INFLAGS_ENABLE_DETACHABLE_UI)
+			retval = VbBootRecoveryMenu(&ctx, cparams);
+		else
+			retval = VbBootRecovery(&ctx, cparams);
 		VbExEcEnteringMode(0, VB_EC_RECOVERY);
 	} else if (shared->flags & VBSD_BOOT_DEV_SWITCH_ON) {
 		/* Developer boot.  This has UI. */
-	        if (kparams->inflags & VB_SALK_INFLAGS_ENABLE_DETACHABLE_UI)
-		    retval = VbBootDeveloperMenu(&ctx, cparams);
+		if (kparams->inflags & VB_SALK_INFLAGS_ENABLE_DETACHABLE_UI)
+			retval = VbBootDeveloperMenu(&ctx, cparams);
 		else
-		    retval = VbBootDeveloper(&ctx, cparams);
+			retval = VbBootDeveloper(&ctx, cparams);
 		VbExEcEnteringMode(0, VB_EC_DEVELOPER);
 	} else {
 		/* Normal boot */
diff --git a/firmware/lib/vboot_common.c b/firmware/lib/vboot_common.c
index fd6ef79b..1c826667 100644
--- a/firmware/lib/vboot_common.c
+++ b/firmware/lib/vboot_common.c
@@ -210,3 +210,17 @@ int VbSharedDataSetKernelKey(VbSharedDataHeader *header, const VbPublicKey *src)
 
 	return PublicKeyCopy(kdest, src);
 }
+
+int vb2_allow_recovery(uint32_t flags)
+{
+	/* In dev mode, unconditionally allowed. */
+	if (flags & VBSD_BOOT_DEV_SWITCH_ON)
+		return 1;
+
+	/* If EC is in RW, it implies recovery wasn't manually requested. */
+	if (!VbExTrustEC(0))
+		return 0;
+
+	/* Now we confidently check the recovery switch state at boot */
+	return !!(flags & VBSD_BOOT_REC_SWITCH_ON);
+}
diff --git a/firmware/lib/vboot_ui.c b/firmware/lib/vboot_ui.c
index 4739cd64..c3a3e9e9 100644
--- a/firmware/lib/vboot_ui.c
+++ b/firmware/lib/vboot_ui.c
@@ -413,7 +413,7 @@ VbError_t VbBootDeveloper(struct vb2_context *ctx, VbCommonParams *cparams)
 #define REC_KEY_DELAY        20       /* Check keys every 20ms */
 #define REC_MEDIA_INIT_DELAY 500      /* Check removable media every 500ms */
 
-VbError_t vb2_recovery_ui(struct vb2_context *ctx, VbCommonParams *cparams)
+static VbError_t recovery_ui(struct vb2_context *ctx, VbCommonParams *cparams)
 {
 	VbSharedDataHeader *shared =
 		(VbSharedDataHeader *)cparams->shared_data_blob;
@@ -423,21 +423,14 @@ VbError_t vb2_recovery_ui(struct vb2_context *ctx, VbCommonParams *cparams)
 
 	VB2_DEBUG("VbBootRecovery() start\n");
 
-	/*
-	 * If the dev-mode switch is off and the user didn't press the recovery
-	 * button (recovery was triggerred automatically), show 'broken' screen.
-	 * The user can either only shutdown to abort or hit esc+refresh+power
-	 * to initiate recovery as instructed on the screen.
-	 */
-	if (!(shared->flags & VBSD_BOOT_DEV_SWITCH_ON) &&
-	    !(shared->flags & VBSD_BOOT_REC_SWITCH_ON)) {
+	if (!vb2_allow_recovery(shared->flags)) {
 		/*
 		 * We have to save the reason here so that it will survive
 		 * coming up three-finger-salute. We're saving it in
 		 * VBNV_RECOVERY_SUBCODE to avoid a recovery loop.
 		 * If we save the reason in VBNV_RECOVERY_REQUEST, we will come
 		 * back here, thus, we won't be able to give a user a chance to
-		 * reboot to workaround boot hicups.
+		 * reboot to workaround a boot hiccup.
 		 */
 		VB2_DEBUG("VbBootRecovery() saving recovery reason (%#x)\n",
 			 shared->recovery_reason);
@@ -563,7 +556,7 @@ VbError_t vb2_recovery_ui(struct vb2_context *ctx, VbCommonParams *cparams)
 
 VbError_t VbBootRecovery(struct vb2_context *ctx, VbCommonParams *cparams)
 {
-	VbError_t retval = vb2_recovery_ui(ctx, cparams);
+	VbError_t retval = recovery_ui(ctx, cparams);
 	VbDisplayScreen(ctx, cparams, VB_SCREEN_BLANK, 0);
 	return retval;
 }
diff --git a/firmware/lib/vboot_ui_menu.c b/firmware/lib/vboot_ui_menu.c
index cacee432..9d6b37d8 100644
--- a/firmware/lib/vboot_ui_menu.c
+++ b/firmware/lib/vboot_ui_menu.c
@@ -1019,7 +1019,7 @@ VbError_t VbBootDeveloperMenu(struct vb2_context *ctx, VbCommonParams *cparams)
  * @param cparams	Vboot1 common params
  * @return VBERROR_SUCCESS, or non-zero error code if error.
  */
-VbError_t vb2_recovery_menu(struct vb2_context *ctx, VbCommonParams *cparams)
+static VbError_t recovery_ui(struct vb2_context *ctx, VbCommonParams *cparams)
 {
 	VbSharedDataHeader *shared =
 		(VbSharedDataHeader *)cparams->shared_data_blob;
@@ -1031,21 +1031,14 @@ VbError_t vb2_recovery_menu(struct vb2_context *ctx, VbCommonParams *cparams)
 
 	VB2_DEBUG("start\n");
 
-	/*
-	 * If the dev-mode switch is off and the user didn't press the recovery
-	 * button (recovery was triggerred automatically), show 'broken' screen.
-	 * The user can either only shutdown to abort or hit esc+refresh+power
-	 * to initiate recovery as instructed on the screen.
-	 */
-	if (!(shared->flags & VBSD_BOOT_DEV_SWITCH_ON) &&
-	    !(shared->flags & VBSD_BOOT_REC_SWITCH_ON)) {
+	if (!vb2_allow_recovery(shared->flags)) {
 		/*
 		 * We have to save the reason here so that it will survive
 		 * coming up three-finger-salute. We're saving it in
 		 * VBNV_RECOVERY_SUBCODE to avoid a recovery loop.
 		 * If we save the reason in VBNV_RECOVERY_REQUEST, we will come
 		 * back here, thus, we won't be able to give a user a chance to
-		 * reboot to workaround boot hicups.
+		 * reboot to workaround a boot hiccup.
 		 */
 		VB2_DEBUG("saving recovery reason (%#x)\n",
 			  shared->recovery_reason);
@@ -1253,7 +1246,7 @@ VbError_t vb2_recovery_menu(struct vb2_context *ctx, VbCommonParams *cparams)
 
 VbError_t VbBootRecoveryMenu(struct vb2_context *ctx, VbCommonParams *cparams)
 {
-	VbError_t retval = vb2_recovery_menu(ctx, cparams);
+	VbError_t retval = recovery_ui(ctx, cparams);
 	VbDisplayScreen(ctx, cparams, VB_SCREEN_BLANK, 0);
 	return retval;
 }
diff --git a/tests/ec_sync_tests.c b/tests/ec_sync_tests.c
index dc708c1f..56f3baae 100644
--- a/tests/ec_sync_tests.c
+++ b/tests/ec_sync_tests.c
@@ -32,9 +32,7 @@ static uint8_t shared_data[VB_SHARED_DATA_MIN_SIZE];
 static VbSharedDataHeader *shared = (VbSharedDataHeader *)shared_data;
 static GoogleBinaryBlockHeader gbb;
 
-static int trust_ec;
 static int mock_in_rw;
-static VbError_t in_rw_retval;
 static int protect_retval;
 static int ec_ro_protected;
 static int ec_rw_protected;
@@ -90,14 +88,12 @@ static void ResetMocks(void)
 	VbSharedDataInit(shared, sizeof(shared_data));
 	shared->flags = VBSD_EC_SOFTWARE_SYNC;
 
-	trust_ec = 0;
 	mock_in_rw = 0;
 	ec_ro_protected = 0;
 	ec_rw_protected = 0;
 	ec_run_image = 0;   /* 0 = RO, 1 = RW */
 	ec_ro_updated = 0;
 	ec_rw_updated = 0;
-	in_rw_retval = VBERROR_SUCCESS;
 	protect_retval = VBERROR_SUCCESS;
 	update_retval = VBERROR_SUCCESS;
 	run_retval = VBERROR_SUCCESS;
@@ -143,13 +139,7 @@ uint32_t VbExIsShutdownRequested(void)
 
 int VbExTrustEC(int devidx)
 {
-	return trust_ec;
-}
-
-VbError_t VbExEcRunningRW(int devidx, int *in_rw)
-{
-	*in_rw = mock_in_rw;
-	return in_rw_retval;
+	return !mock_in_rw;
 }
 
 VbError_t VbExEcProtect(int devidx, enum VbSelectFirmware_t select)
@@ -169,7 +159,6 @@ VbError_t VbExEcDisableJump(int devidx)
 VbError_t VbExEcJumpToRW(int devidx)
 {
 	ec_run_image = 1;
-	mock_in_rw = 1;
 	return run_retval;
 }
 
@@ -248,24 +237,6 @@ static void test_ssync(VbError_t retval, int recovery_reason, const char *desc)
 
 static void VbSoftwareSyncTest(void)
 {
-	/* Recovery cases */
-	ResetMocks();
-	sd->recovery_reason = 123;
-	test_ssync(0, 0, "In recovery, EC-RO");
-	TEST_EQ(ec_rw_protected, 0, "  ec rw protected");
-
-	ResetMocks();
-	sd->recovery_reason = 123;
-	mock_in_rw = 1;
-	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		   123, "Recovery needs EC-RO");
-
-	/* AP-RO cases */
-	ResetMocks();
-	in_rw_retval = VBERROR_SIMULATED;
-	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		   VBNV_RECOVERY_EC_UNKNOWN_IMAGE, "Unknown EC image");
-
 	/* Calculate hashes */
 	ResetMocks();
 	mock_ec_rw_hash_size = 0;
diff --git a/tests/vboot_api_kernel2_tests.c b/tests/vboot_api_kernel2_tests.c
index f6776e9e..02a9087a 100644
--- a/tests/vboot_api_kernel2_tests.c
+++ b/tests/vboot_api_kernel2_tests.c
@@ -591,8 +591,8 @@ static void VbBootRecTest(void)
 	TEST_EQ(VbBootRecovery(&ctx, &cparams),
 		VBERROR_SHUTDOWN_REQUESTED,
 		"No remove in rec");
-	TEST_EQ(screens_displayed[0], VB_SCREEN_RECOVERY_INSERT,
-		"  insert screen");
+	TEST_EQ(screens_displayed[0], VB_SCREEN_OS_BROKEN,
+		"  broken screen");
 
 	/* Removal if no disk initially found, but found on second attempt */
 	ResetMocks();
diff --git a/tests/vboot_api_kernel4_tests.c b/tests/vboot_api_kernel4_tests.c
index 41e58c8e..59650701 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -29,7 +29,6 @@ static uint8_t shared_data[VB_SHARED_DATA_MIN_SIZE];
 static VbSharedDataHeader *shared = (VbSharedDataHeader *)shared_data;
 static GoogleBinaryBlockHeader gbb;
 
-static int ecsync_retval;
 static uint32_t rkr_version;
 static uint32_t new_version;
 static struct RollbackSpaceFwmp rfr_fwmp;
@@ -62,7 +61,6 @@ static void ResetMocks(void)
 	memset(&rfr_fwmp, 0, sizeof(rfr_fwmp));
 	rfr_retval = TPM_SUCCESS;
 
-	ecsync_retval = VBERROR_SUCCESS;
 	rkr_version = new_version = 0x10002;
 	rkr_retval = rkw_retval = rkl_retval = VBERROR_SUCCESS;
 	vbboot_retval = VBERROR_SUCCESS;
@@ -82,11 +80,6 @@ VbError_t VbExNvStorageWrite(const uint8_t *buf)
 	return VBERROR_SUCCESS;
 }
 
-VbError_t VbExEcRunningRW(int devidx, int *in_rw)
-{
-	return ecsync_retval;
-}
-
 uint32_t RollbackKernelRead(uint32_t *version)
 {
 	*version = rkr_version;
@@ -158,26 +151,17 @@ static void VbSlkTest(void)
 	ResetMocks();
 	test_slk(0, 0, "Normal");
 
-	/* Mock error early in software sync */
-	ResetMocks();
-	shared->flags |= VBSD_EC_SOFTWARE_SYNC;
-	ecsync_retval = VBERROR_SIMULATED;
-	test_slk(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		 VBNV_RECOVERY_EC_UNKNOWN_IMAGE, "EC sync bad");
-
 	/*
 	 * If shared->flags doesn't ask for software sync, we won't notice
 	 * that error.
 	 */
 	ResetMocks();
-	ecsync_retval = VBERROR_SIMULATED;
 	test_slk(0, 0, "EC sync not done");
 
 	/* Same if shared->flags asks for sync, but it's overridden by GBB */
 	ResetMocks();
 	shared->flags |= VBSD_EC_SOFTWARE_SYNC;
 	gbb.flags |= GBB_FLAG_DISABLE_EC_SOFTWARE_SYNC;
-	ecsync_retval = VBERROR_SIMULATED;
 	test_slk(0, 0, "EC sync disabled by GBB");
 
 	/* Rollback kernel version */