[coreboot-gerrit] Change in coreboot[master]: security/vboot: overwrite existing spaces during factory init for tpm2

[Andrey Pronin (Code Review)]

Andrey Pronin has uploaded this change for review. ( https://review.coreboot.org/23456


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

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 firmware space, delete, define but not write to the kernel
     space, reboot. coreboot should fill the kernel space and boot the
     OS.

Change-Id: I48d8bb4f9fc0e5276e6ec81247b3b6768ec9fa3b
Signed-off-by: Andrey Pronin <apronin at google.com>
---
M src/security/vboot/secdata_tpm.c
1 file changed, 33 insertions(+), 29 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/23456/1

diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c
index 04162b0..f59361e 100644
--- a/src/security/vboot/secdata_tpm.c
+++ b/src/security/vboot/secdata_tpm.c
@@ -165,45 +165,49 @@
 	return tlcl_write(index, data, length);
 }
 
+static uint32_t set_space(const char *name,
+			  uint32_t index,
+			  const void *data,
+			  uint32_t length)
+{
+	uint32_t rv;
+
+	rv = tlcl_define_space(index, length);
+	if (rv == TPM_E_NV_DEFINED) {
+		/*
+		 * 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(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;
+	return set_space("firmware", FIRMWARE_NV_INDEX, firmware_blob,
+			 VB2_SECDATA_SIZE);
 }
 
 static uint32_t set_kernel_space(const void *kernel_blob)
 {
-	uint32_t rv;
-
-	rv = tlcl_define_space(KERNEL_NV_INDEX, sizeof(secdata_kernel));
-	if (rv == TPM_E_NV_DEFINED) {
-		VBDEBUG("%s: kernel space already exists\n", __func__);
-		return TPM_SUCCESS;
-	}
-
-	if (rv != TPM_SUCCESS)
-		return rv;
-
-	return safe_write(KERNEL_NV_INDEX, kernel_blob, sizeof(secdata_kernel));
+	return set_space("kernel", KERNEL_NV_INDEX, kernel_blob,
+			 sizeof(secdata_kernel));
 }
 
 static uint32_t set_rec_hash_space(const uint8_t *data)
 {
-	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;
-	}
-
-	if (rv != TPM_SUCCESS)
-		return rv;
-
-	return safe_write(REC_HASH_NV_INDEX, data, REC_HASH_NV_SIZE);
+	return set_space("MRC Hash", REC_HASH_NV_INDEX, data,
+			 REC_HASH_NV_SIZE);
 }
 
 static uint32_t _factory_initialize_tpm(struct vb2_context *ctx)

-- 
To view, visit https://review.coreboot.org/23456
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I48d8bb4f9fc0e5276e6ec81247b3b6768ec9fa3b
Gerrit-Change-Number: 23456
Gerrit-PatchSet: 1
Gerrit-Owner: Andrey Pronin <apronin at google.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180126/6653eb8a/attachment-0001.html>