security/vboot: overwrite existing spaces during factory init for tpm2

In TPM 2.0 case, if the factory initialization is interrupted after
defining, say, the kernel tpm nvram space but before writing to this
space, the following will happen upon reboot when the factory
initialization will be re-attempted. Writing to this space will be
skipped, and coreboot will finish the factory initialization with
this space remained unwritten. At a later stage, when the rollback
logic will attempt to check the version in the kernel space, it will
fail (TPM2.0 returns an error when reading from unwritten spaces),
and the system will go into recovery with no way out (since the
kernel space will never be written).

This change fixes that by always writing to the kernel, MRC hash and
firmware spaces during factory initialization, even if the space
already existed by that time.

BUG=b:71884828
TEST=delete, define, but not write to the kernel space; trigger
     factory initialization; coreboot should fill the kernel
     space and continue booting.

Change-Id: I48d8bb4f9fc0e5276e6ec81247b3b6768ec9fa3b
Signed-off-by: Andrey Pronin <apronin@google.com>
Reviewed-on: https://review.coreboot.org/23456
Reviewed-by: Aaron Durbin <adurbin@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>

1 files changed, 27 insertions, 25 deletions

diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c
index 04162b0578..a757f02f98 100644
--- a/src/security/vboot/secdata_tpm.c
+++ b/src/security/vboot/secdata_tpm.c
@@ -165,45 +165,47 @@ static uint32_t safe_write(uint32_t index, const void *data, uint32_t length)
 	return tlcl_write(index, data, length);
 }
 
-static uint32_t set_firmware_space(const void *firmware_blob)
-{
-	RETURN_ON_FAILURE(tlcl_define_space(FIRMWARE_NV_INDEX,
-					    VB2_SECDATA_SIZE));
-	RETURN_ON_FAILURE(safe_write(FIRMWARE_NV_INDEX, firmware_blob,
-				     VB2_SECDATA_SIZE));
-	return TPM_SUCCESS;
-}
-
-static uint32_t set_kernel_space(const void *kernel_blob)
+static uint32_t set_space(const char *name, uint32_t index, const void *data,
+			  uint32_t length)
 {
 	uint32_t rv;
 
-	rv = tlcl_define_space(KERNEL_NV_INDEX, sizeof(secdata_kernel));
+	rv = tlcl_define_space(index, length);
 	if (rv == TPM_E_NV_DEFINED) {
-		VBDEBUG("%s: kernel space already exists\n", __func__);
-		return TPM_SUCCESS;
+		/*
+		 * Continue with writing: it may be defined, but not written
+		 * to. In that case a subsequent tlcl_read() would still return
+		 * TPM_E_BADINDEX on TPM 2.0. The cases when some non-firmware
+		 * space is defined while the firmware space is not there
+		 * should be rare (interrupted initialization), so no big harm
+		 * in writing once again even if it was written already.
+		 */
+		VBDEBUG("%s: %s space already exists\n", __func__, name);
+		rv = TPM_SUCCESS;
 	}
 
 	if (rv != TPM_SUCCESS)
 		return rv;
 
-	return safe_write(KERNEL_NV_INDEX, kernel_blob, sizeof(secdata_kernel));
+	return safe_write(index, data, length);
 }
 
-static uint32_t set_rec_hash_space(const uint8_t *data)
+static uint32_t set_firmware_space(const void *firmware_blob)
 {
-	uint32_t rv;
-
-	rv = tlcl_define_space(REC_HASH_NV_INDEX, REC_HASH_NV_SIZE);
-	if (rv == TPM_E_NV_DEFINED) {
-		VBDEBUG("%s: MRC Hash space already exists\n", __func__);
-		return TPM_SUCCESS;
-	}
+	return set_space("firmware", FIRMWARE_NV_INDEX, firmware_blob,
+			 VB2_SECDATA_SIZE);
+}
 
-	if (rv != TPM_SUCCESS)
-		return rv;
+static uint32_t set_kernel_space(const void *kernel_blob)
+{
+	return set_space("kernel", KERNEL_NV_INDEX, kernel_blob,
+			 sizeof(secdata_kernel));
+}
 
-	return safe_write(REC_HASH_NV_INDEX, data, REC_HASH_NV_SIZE);
+static uint32_t set_rec_hash_space(const uint8_t *data)
+{
+	return set_space("MRC Hash", REC_HASH_NV_INDEX, data,
+			 REC_HASH_NV_SIZE);
 }
 
 static uint32_t _factory_initialize_tpm(struct vb2_context *ctx)