vboot_api_kernel: Remove assumptions about EC-RW hash type and size

With newer PD chips and different update mechanisms, we can no longer
guarantee that the "hash" (really just a sort of version identifier) of
an EC-RW image will always be a SHA256. This patch removes any hardcoded
assumptions about that from vboot, and instead accepts any hash size
returned by VbExEcHashImage() and VbExEcGetExpectedImageHash().

It also removes the assumption that the hash can be regenerated by
running SHA256 over the full image returned by VbExEcGetExpectedImage().
We can thus no longer support VBERROR_EC_GET_EXPECTED_HASH_FROM_IMAGE,
which is fine since that functionality hasn't been needed for years and
there would be no reason why we might need it in the future. This also
allows simplifying the code flow of EcUpdateImage() a bit (since you can
really just return very early if you already figured out that you don't
need to update).

BRANCH=None
BUG=chrome-os-partner:53780
TEST=Tested software sync on Oak both after cold and warm boot.

Change-Id: I498f3d39085a38740734fff9f2d1a186a0801489
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/348001
Reviewed-by: Randall Spangler <rspangler@chromium.org>

2 files changed, 101 insertions, 188 deletions

diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 5adeac41..90703a90 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -714,11 +714,11 @@ static VbError_t EcUpdateImage(int devidx, VbCommonParams *cparams,
 		(VbSharedDataHeader *)cparams->shared_data_blob;
 	int rv;
 	int hash_size;
+	int ec_hash_size;
 	const uint8_t *hash = NULL;
 	const uint8_t *expected = NULL;
 	const uint8_t *ec_hash = NULL;
 	int expected_size;
-	uint8_t expected_hash[SHA256_DIGEST_SIZE];
 	int i;
 	int rw_request = select != VB_SELECT_FIRMWARE_READONLY;
 
@@ -727,202 +727,141 @@ static VbError_t EcUpdateImage(int devidx, VbCommonParams *cparams,
 		 "Check for %s update\n", rw_request ? "RW" : "RO"));
 
 	/* Get current EC hash. */
-	rv = VbExEcHashImage(devidx, select, &ec_hash, &hash_size);
-
+	rv = VbExEcHashImage(devidx, select, &ec_hash, &ec_hash_size);
 	if (rv) {
 		VBDEBUG(("EcUpdateImage() - "
 			 "VbExEcHashImage() returned %d\n", rv));
 		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_FAILED);
 		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
-	if (hash_size != SHA256_DIGEST_SIZE) {
-		VBDEBUG(("EcUpdateImage() - "
-			 "VbExEcHashImage() says size %d, not %d\n",
-			 hash_size, SHA256_DIGEST_SIZE));
-		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_SIZE);
-		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-	}
-	VBDEBUG(("EC-%s hash:", rw_request ? "RW" : "RO"));
-	for (i = 0; i < SHA256_DIGEST_SIZE; i++)
+	VBDEBUG(("EC-%s hash: ", rw_request ? "RW" : "RO"));
+	for (i = 0; i < ec_hash_size; i++)
 		VBDEBUG(("%02x",ec_hash[i]));
 	VBDEBUG(("\n"));
 
 	/* Get expected EC hash. */
 	rv = VbExEcGetExpectedImageHash(devidx, select, &hash, &hash_size);
-
-	if (rv == VBERROR_EC_GET_EXPECTED_HASH_FROM_IMAGE) {
-		/*
-		 * BIOS has verified EC image but doesn't have a precomputed
-		 * hash for it, so we must compute the hash ourselves.
-		 */
-		hash = NULL;
-	} else if (rv) {
+	if (rv) {
 		VBDEBUG(("EcUpdateImage() - "
 			 "VbExEcGetExpectedImageHash() returned %d\n", rv));
 		VbSetRecoveryRequest(VBNV_RECOVERY_EC_EXPECTED_HASH);
 		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-	} else if (hash_size != SHA256_DIGEST_SIZE) {
+	}
+	if (ec_hash_size != hash_size) {
 		VBDEBUG(("EcUpdateImage() - "
-			 "VbExEcGetExpectedImageHash() says size %d, not %d\n",
-			 hash_size, SHA256_DIGEST_SIZE));
-		VbSetRecoveryRequest(VBNV_RECOVERY_EC_EXPECTED_HASH);
+			 "EC uses %d-byte hash, but AP-RW contains %d bytes\n",
+			 ec_hash_size, hash_size));
+		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_SIZE);
 		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-	} else {
-		VBDEBUG(("Expected hash:"));
-		for (i = 0; i < SHA256_DIGEST_SIZE; i++)
-			VBDEBUG(("%02x", hash[i]));
-		VBDEBUG(("\n"));
-		*need_update = SafeMemcmp(ec_hash, hash, SHA256_DIGEST_SIZE);
 	}
 
-	/*
-	 * Get expected EC image if we're sure we need to update (because the
-	 * expected hash didn't match the EC) or we still don't know (because
-	 * there was no expected hash and we need the image to compute one
-	 * ourselves).
-	 */
-	if (*need_update || !hash) {
-		/* Get expected EC image */
-		rv = VbExEcGetExpectedImage(devidx, select, &expected,
-					    &expected_size);
-		if (rv) {
-			VBDEBUG(("EcUpdateImage() - "
-				 "VbExEcGetExpectedImage() returned %d\n", rv));
-			VbSetRecoveryRequest(VBNV_RECOVERY_EC_EXPECTED_IMAGE);
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-		}
-		VBDEBUG(("EcUpdateImage() - image len = %d\n", expected_size));
-
-		/* Hash expected image */
-		internal_SHA256(expected, expected_size, expected_hash);
-		VBDEBUG(("Computed hash of expected image:"));
-		for (i = 0; i < SHA256_DIGEST_SIZE; i++)
-			VBDEBUG(("%02x", expected_hash[i]));
-		VBDEBUG(("\n"));
-	}
+	VBDEBUG(("Expected hash: "));
+	for (i = 0; i < hash_size; i++)
+		VBDEBUG(("%02x", hash[i]));
+	VBDEBUG(("\n"));
+	*need_update = SafeMemcmp(ec_hash, hash, hash_size);
 
-	if (!hash) {
-		/*
-		 * BIOS didn't have expected EC hash, so check if we need
-		 * update by comparing EC hash to the one we just computed.
-		 */
-		*need_update = SafeMemcmp(ec_hash, expected_hash,
-					  SHA256_DIGEST_SIZE);
-	} else if (*need_update && SafeMemcmp(hash, expected_hash,
-					      SHA256_DIGEST_SIZE)) {
-		/*
-		 * We need to update, but the expected EC image doesn't match
-		 * the expected EC hash we were given.
-		 */
+	if (!*need_update)
+		return VBERROR_SUCCESS;
+
+	/* Get expected EC image */
+	rv = VbExEcGetExpectedImage(devidx, select, &expected, &expected_size);
+	if (rv) {
 		VBDEBUG(("EcUpdateImage() - "
 			 "VbExEcGetExpectedImage() returned %d\n", rv));
-		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_MISMATCH);
+		VbSetRecoveryRequest(VBNV_RECOVERY_EC_EXPECTED_IMAGE);
 		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
+	VBDEBUG(("EcUpdateImage() - image len = %d\n", expected_size));
 
 	if (in_rw && rw_request) {
-		if (*need_update) {
-			/*
-			 * Check if BIOS should also load VGA Option ROM when
-			 * rebooting to save another reboot if possible.
-			 */
-			if ((shared->flags & VBSD_EC_SLOW_UPDATE) &&
-			    (shared->flags & VBSD_OPROM_MATTERS) &&
-			    !(shared->flags & VBSD_OPROM_LOADED)) {
-				VBDEBUG(("EcUpdateImage() - Reboot to "
-					 "load VGA Option ROM\n"));
-				VbNvSet(&vnc, VBNV_OPROM_NEEDED, 1);
-			}
-
-			/*
-			 * EC is running the wrong RW image.  Reboot the EC to
-			 * RO so we can update it on the next boot.
-			 */
-			VBDEBUG(("EcUpdateImage() - "
-				 "in RW, need to update RW, so reboot\n"));
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
+		/*
+		 * Check if BIOS should also load VGA Option ROM when
+		 * rebooting to save another reboot if possible.
+		 */
+		if ((shared->flags & VBSD_EC_SLOW_UPDATE) &&
+		    (shared->flags & VBSD_OPROM_MATTERS) &&
+		    !(shared->flags & VBSD_OPROM_LOADED)) {
+			VBDEBUG(("EcUpdateImage() - Reboot to "
+				 "load VGA Option ROM\n"));
+			VbNvSet(&vnc, VBNV_OPROM_NEEDED, 1);
 		}
 
-		VBDEBUG(("EcUpdateImage() in EC-RW and it matches\n"));
-		return VBERROR_SUCCESS;
+		/*
+		 * EC is running the wrong RW image.  Reboot the EC to
+		 * RO so we can update it on the next boot.
+		 */
+		VBDEBUG(("EcUpdateImage() - "
+			 "in RW, need to update RW, so reboot\n"));
+		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
 
-	/* Update EC if necessary */
-
-	if (*need_update) {
-		VBDEBUG(("EcUpdateImage() updating EC-%s...\n",
-			 rw_request ? "RW" : "RO"));
+	VBDEBUG(("EcUpdateImage() updating EC-%s...\n",
+		 rw_request ? "RW" : "RO"));
 
-		if (shared->flags & VBSD_EC_SLOW_UPDATE) {
-			VBDEBUG(("EcUpdateImage() - "
-				 "EC is slow. Show WAIT screen.\n"));
+	if (shared->flags & VBSD_EC_SLOW_UPDATE) {
+		VBDEBUG(("EcUpdateImage() - EC is slow. Show WAIT screen.\n"));
 
-			/* Ensure the VGA Option ROM is loaded */
-			if ((shared->flags & VBSD_OPROM_MATTERS) &&
-			    !(shared->flags & VBSD_OPROM_LOADED)) {
-				VBDEBUG(("EcUpdateImage() - Reboot to "
-					 "load VGA Option ROM\n"));
-				VbNvSet(&vnc, VBNV_OPROM_NEEDED, 1);
-				return VBERROR_VGA_OPROM_MISMATCH;
-			}
-
-			VbDisplayScreen(cparams, VB_SCREEN_WAIT, 0, &vnc);
+		/* Ensure the VGA Option ROM is loaded */
+		if ((shared->flags & VBSD_OPROM_MATTERS) &&
+		    !(shared->flags & VBSD_OPROM_LOADED)) {
+			VBDEBUG(("EcUpdateImage() - Reboot to "
+				 "load VGA Option ROM\n"));
+			VbNvSet(&vnc, VBNV_OPROM_NEEDED, 1);
+			return VBERROR_VGA_OPROM_MISMATCH;
 		}
 
-		rv = VbExEcUpdateImage(devidx, select, expected, expected_size);
-
-		if (rv != VBERROR_SUCCESS) {
-			VBDEBUG(("EcUpdateImage() - "
-				 "VbExEcUpdateImage() returned %d\n", rv));
+		VbDisplayScreen(cparams, VB_SCREEN_WAIT, 0, &vnc);
+	}
 
-			/*
-			 * The EC may know it needs a reboot.  It may need to
-			 * unprotect the region before updating, or may need to
-			 * reboot after updating.  Either way, it's not an error
-			 * requiring recovery mode.
-			 *
-			 * If we fail for any other reason, trigger recovery
-			 * mode.
-			 */
-			if (rv != VBERROR_EC_REBOOT_TO_RO_REQUIRED)
-				VbSetRecoveryRequest(VBNV_RECOVERY_EC_UPDATE);
+	rv = VbExEcUpdateImage(devidx, select, expected, expected_size);
+	if (rv != VBERROR_SUCCESS) {
+		VBDEBUG(("EcUpdateImage() - "
+			 "VbExEcUpdateImage() returned %d\n", rv));
 
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-		}
+		/*
+		 * The EC may know it needs a reboot.  It may need to
+		 * unprotect the region before updating, or may need to
+		 * reboot after updating.  Either way, it's not an error
+		 * requiring recovery mode.
+		 *
+		 * If we fail for any other reason, trigger recovery
+		 * mode.
+		 */
+		if (rv != VBERROR_EC_REBOOT_TO_RO_REQUIRED)
+			VbSetRecoveryRequest(VBNV_RECOVERY_EC_UPDATE);
 
+		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
 
 	/* Verify the EC was updated properly */
-	if (*need_update) {
-		/* Get current EC hash. */
-		rv = VbExEcHashImage(devidx, select, &ec_hash, &hash_size);
-
-		if (rv) {
-			VBDEBUG(("EcUpdateImage() - "
-				 "VbExEcHashImage() returned %d\n", rv));
-			VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_FAILED);
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-		}
-		if (hash_size != SHA256_DIGEST_SIZE) {
-			VBDEBUG(("EcUpdateImage() - "
-				 "VbExEcHashImage() says size %d, not %d\n",
-				 hash_size, SHA256_DIGEST_SIZE));
-			VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_SIZE);
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-		}
-		VBDEBUG(("Updated EC-%s hash:", rw_request ? "RW" : "RO"));
-		for (i = 0; i < SHA256_DIGEST_SIZE; i++)
-			VBDEBUG(("%02x",ec_hash[i]));
-		VBDEBUG(("\n"));
-
-		if (SafeMemcmp(ec_hash, hash, SHA256_DIGEST_SIZE)){
-			VBDEBUG(("EcUpdateImage() - "
-				 "Failed to update EC-%s\n", rw_request ?
-				 "RW" : "RO"));
-			VbSetRecoveryRequest(VBNV_RECOVERY_EC_UPDATE);
-			return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-		}
+	rv = VbExEcHashImage(devidx, select, &ec_hash, &ec_hash_size);
+	if (rv) {
+		VBDEBUG(("EcUpdateImage() - "
+			 "VbExEcHashImage() returned %d\n", rv));
+		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_FAILED);
+		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
+	if (hash_size != ec_hash_size) {
+		VBDEBUG(("EcUpdateImage() - "
+			 "VbExEcHashImage() says size %d, not %d\n",
+			 ec_hash_size, hash_size));
+		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_SIZE);
+		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
+	}
+	VBDEBUG(("Updated EC-%s hash: ", rw_request ? "RW" : "RO"));
+	for (i = 0; i < ec_hash_size; i++)
+		VBDEBUG(("%02x",ec_hash[i]));
+	VBDEBUG(("\n"));
+
+	if (SafeMemcmp(ec_hash, hash, hash_size)){
+		VBDEBUG(("EcUpdateImage() - "
+			 "Failed to update EC-%s\n", rw_request ?
+			 "RW" : "RO"));
+		VbSetRecoveryRequest(VBNV_RECOVERY_EC_UPDATE);
+		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
+	}
+
 	return VBERROR_SUCCESS;
 }
 
diff --git a/tests/vboot_api_kernel3_tests.c b/tests/vboot_api_kernel3_tests.c
index d6f740b6..6ca58f21 100644
--- a/tests/vboot_api_kernel3_tests.c
+++ b/tests/vboot_api_kernel3_tests.c
@@ -47,7 +47,6 @@ static int mock_ec_rw_hash_size;
 static uint8_t want_ec_hash[32];
 static uint8_t update_hash;
 static int want_ec_hash_size;
-static uint8_t mock_sha[32];
 
 static uint32_t screens_displayed[8];
 static uint32_t screens_count = 0;
@@ -104,9 +103,6 @@ static void ResetMocks(void)
 
 	update_hash = 42;
 
-	Memset(mock_sha, 0, sizeof(want_ec_hash));
-	mock_sha[0] = 42;
-
 	// TODO: ensure these are actually needed
 
 	Memset(screens_displayed, 0, sizeof(screens_displayed));
@@ -182,16 +178,7 @@ VbError_t VbExEcGetExpectedImageHash(int devidx, enum VbSelectFirmware_t select,
 	*hash = want_ec_hash;
 	*hash_size = want_ec_hash_size;
 
-	if (want_ec_hash_size == -1)
-		return VBERROR_EC_GET_EXPECTED_HASH_FROM_IMAGE;
-	else
-		return want_ec_hash_size ? VBERROR_SUCCESS : VBERROR_SIMULATED;
-}
-
-uint8_t *internal_SHA256(const uint8_t *data, uint64_t len, uint8_t *digest)
-{
-	Memcpy(digest, mock_sha, sizeof(mock_sha));
-	return digest;
+	return want_ec_hash_size ? VBERROR_SUCCESS : VBERROR_SIMULATED;
 }
 
 VbError_t VbExEcUpdateImage(int devidx, enum VbSelectFirmware_t select,
@@ -296,30 +283,17 @@ static void VbSoftwareSyncTest(void)
 	ResetMocks();
 	want_ec_hash_size = 16;
 	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		   VBNV_RECOVERY_EC_EXPECTED_HASH,
-		   "Bad precalculated hash size");
+		   VBNV_RECOVERY_EC_HASH_SIZE,
+		   "Hash size mismatch");
 
 	ResetMocks();
-	mock_in_rw = 1;
-	want_ec_hash_size = -1;
-	test_ssync(0, 0, "No precomputed hash");
-
-	ResetMocks();
-	want_ec_hash_size = -1;
-	get_expected_retval = VBERROR_SIMULATED;
-	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		   VBNV_RECOVERY_EC_EXPECTED_IMAGE, "Can't fetch image");
+	want_ec_hash_size = 4;
+	mock_ec_rw_hash_size = 4;
+	test_ssync(0, 0, "Custom hash size");
 
 	/* Updates required */
 	ResetMocks();
 	mock_in_rw = 1;
-	want_ec_hash[0]++;
-	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		   VBNV_RECOVERY_EC_HASH_MISMATCH,
-		   "Precalculated hash mismatch");
-
-	ResetMocks();
-	mock_in_rw = 1;
 	mock_ec_rw_hash[0]++;
 	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
 		   0, "Pending update needs reboot");