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>

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,64 +727,45 @@ 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(("EC-%s hash: ", rw_request ? "RW" : "RO"));
-		VBDEBUG(("EcUpdateImage() - "
+	for (i = 0; i < ec_hash_size; i++)
-			 "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(("%02x",ec_hash[i]));
 	VBDEBUG(("\n"));
 
 	/* Get expected EC hash. */
 	rv = VbExEcGetExpectedImageHash(devidx, select, &hash, &hash_size);
-
+	if (rv) {
-	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) {
 		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",
+			 "EC uses %d-byte hash, but AP-RW contains %d bytes\n",
-			 hash_size, SHA256_DIGEST_SIZE));
+			 ec_hash_size, hash_size));
-		VbSetRecoveryRequest(VBNV_RECOVERY_EC_EXPECTED_HASH);
+		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);
 	}
 
-	/*
+	VBDEBUG(("Expected hash: "));
-	 * Get expected EC image if we're sure we need to update (because the
+	for (i = 0; i < hash_size; i++)
-	 * expected hash didn't match the EC) or we still don't know (because
+		VBDEBUG(("%02x", hash[i]));
-	 * there was no expected hash and we need the image to compute one
+	VBDEBUG(("\n"));
-	 * ourselves).
+	*need_update = SafeMemcmp(ec_hash, hash, hash_size);
-	 */
+
-	if (*need_update || !hash) {
+	if (!*need_update)
+		return VBERROR_SUCCESS;
+
 	/* Get expected EC image */
-		rv = VbExEcGetExpectedImage(devidx, select, &expected,
+	rv = VbExEcGetExpectedImage(devidx, select, &expected, &expected_size);
-					    &expected_size);
 	if (rv) {
 		VBDEBUG(("EcUpdateImage() - "
 			 "VbExEcGetExpectedImage() returned %d\n", rv));
@@ -793,35 +774,7 @@ static VbError_t EcUpdateImage(int devidx, VbCommonParams *cparams,
 	}
 	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"));
-	}
-
-	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.
-		 */
-		VBDEBUG(("EcUpdateImage() - "
-			 "VbExEcGetExpectedImage() returned %d\n", rv));
-		VbSetRecoveryRequest(VBNV_RECOVERY_EC_HASH_MISMATCH);
-		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-	}
-
 	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.
@@ -843,19 +796,11 @@ static VbError_t EcUpdateImage(int devidx, VbCommonParams *cparams,
 		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
 
-		VBDEBUG(("EcUpdateImage() in EC-RW and it matches\n"));
-		return VBERROR_SUCCESS;
-	}
-
-	/* Update EC if necessary */
-
-	if (*need_update) {
 	VBDEBUG(("EcUpdateImage() updating EC-%s...\n",
 		 rw_request ? "RW" : "RO"));
 
 	if (shared->flags & VBSD_EC_SLOW_UPDATE) {
-			VBDEBUG(("EcUpdateImage() - "
+		VBDEBUG(("EcUpdateImage() - EC is slow. Show WAIT screen.\n"));
-				 "EC is slow. Show WAIT screen.\n"));
 
 		/* Ensure the VGA Option ROM is loaded */
 		if ((shared->flags & VBSD_OPROM_MATTERS) &&
@@ -870,7 +815,6 @@ static VbError_t EcUpdateImage(int devidx, VbCommonParams *cparams,
 	}
 
 	rv = VbExEcUpdateImage(devidx, select, expected, expected_size);
-
 	if (rv != VBERROR_SUCCESS) {
 		VBDEBUG(("EcUpdateImage() - "
 			 "VbExEcUpdateImage() returned %d\n", rv));
@@ -890,39 +834,34 @@ static VbError_t EcUpdateImage(int devidx, VbCommonParams *cparams,
 		return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
 	}
 
-	}
-
 	/* Verify the EC was updated properly */
-	if (*need_update) {
+	rv = VbExEcHashImage(devidx, select, &ec_hash, &ec_hash_size);
-		/* 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) {
+	if (hash_size != ec_hash_size) {
 		VBDEBUG(("EcUpdateImage() - "
 			 "VbExEcHashImage() says size %d, not %d\n",
-				 hash_size, SHA256_DIGEST_SIZE));
+			 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"));
+	VBDEBUG(("Updated EC-%s hash: ", rw_request ? "RW" : "RO"));
-		for (i = 0; i < SHA256_DIGEST_SIZE; i++)
+	for (i = 0; i < ec_hash_size; i++)
 		VBDEBUG(("%02x",ec_hash[i]));
 	VBDEBUG(("\n"));
 
-		if (SafeMemcmp(ec_hash, hash, SHA256_DIGEST_SIZE)){
+	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;
 }
 

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,18 +178,9 @@ 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;
-}
-
 VbError_t VbExEcUpdateImage(int devidx, enum VbSelectFirmware_t select,
 			    const uint8_t *image, int image_size)
 {
@@ -296,28 +283,15 @@ static void VbSoftwareSyncTest(void)
 	ResetMocks();
 	want_ec_hash_size = 16;
 	test_ssync(VBERROR_EC_REBOOT_TO_RO_REQUIRED,
-		   VBNV_RECOVERY_EC_EXPECTED_HASH,
+		   VBNV_RECOVERY_EC_HASH_SIZE,
-		   "Bad precalculated hash size");
+		   "Hash size mismatch");
 
 	ResetMocks();
-	mock_in_rw = 1;
+	want_ec_hash_size = 4;
-	want_ec_hash_size = -1;
+	mock_ec_rw_hash_size = 4;
-	test_ssync(0, 0, "No precomputed hash");
+	test_ssync(0, 0, "Custom hash size");
-
-	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");
 
 	/* 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]++;