vboot2: Fail vb2_secdata_(get|set) when secdata was not initializedfirmware-kitty-5771.61.B

This patch adds a check to vboot2 secdata accessor functions that
returns an error if vb2_secdata_init() has not yet been called or
failed for some reason. This avoids a problem where vboot may
misinterpret random garbage (e.g. from transient read failures) as
valid secdata in recovery mode and write it back to the TPM (bricking
the device in a way that requires manual repair).

Also removes VB2_ERROR_SECDATA_VERSION check. This check was not
terribly useful since there should be no way a vboot2 device could ever
have secdata version 1 (and if it did, it should still fail CRC checks).
This error can trigger for cases when secdata contains random garbage
(e.g. all zeroes) and prevent the much more appropriate
VB2_ERROR_SECDATA_CRC error from even being checked for, which just
creates confusion and makes it harder to determine the real problem.

BRANCH=veyron
BUG=chrome-os-partner:34871
TEST=Emulated TPM read errors by just manually memset()ing secdata to 0
in coreboot, verified that vboot does not write back to the TPM and the
device will start working fine again once the disruption is removed.

Change-Id: I76bcbdbcd8106a0d34717cc91a8f2d7cda303c3f
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/245981
Reviewed-by: Shawn N <shawnn@chromium.org>
Commit-Queue: Shawn N <shawnn@chromium.org>
Tested-by: Shawn N <shawnn@chromium.org>

3 files changed, 26 insertions, 16 deletions

diff --git a/firmware/2lib/2secdata.c b/firmware/2lib/2secdata.c
index 0c5a34e4..c2983e71 100644
--- a/firmware/2lib/2secdata.c
+++ b/firmware/2lib/2secdata.c
@@ -42,27 +42,22 @@ int vb2_secdata_create(struct vb2_context *ctx)
 int vb2_secdata_init(struct vb2_context *ctx)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
-	struct vb2_secdata *sec = (struct vb2_secdata *)ctx->secdata;
 	int rv;
 
-	/* Data must be new enough to have a CRC */
-	if (sec->struct_version < 2)
-		return VB2_ERROR_SECDATA_VERSION;
-
 	rv = vb2_secdata_check_crc(ctx);
 	if (rv)
 		return rv;
 
+	/* Set status flag */
+	sd->status |= VB2_SD_STATUS_SECDATA_INIT;
+	// TODO: unit test for that
+
 	/* Read this now to make sure crossystem has it even in rec mode. */
 	rv = vb2_secdata_get(ctx, VB2_SECDATA_VERSIONS,
 			     &sd->fw_version_secdata);
 	if (rv)
 		return rv;
 
-	/* Set status flag */
-	sd->status |= VB2_SD_STATUS_SECDATA_INIT;
-	// TODO: unit test for that
-
 	return VB2_SUCCESS;
 }
 
@@ -72,6 +67,9 @@ int vb2_secdata_get(struct vb2_context *ctx,
 {
 	struct vb2_secdata *sec = (struct vb2_secdata *)ctx->secdata;
 
+	if (!(vb2_get_sd(ctx)->status & VB2_SD_STATUS_SECDATA_INIT))
+		return VB2_ERROR_SECDATA_GET_UNINITIALIZED;
+
 	switch(param) {
 	case VB2_SECDATA_FLAGS:
 		*dest = sec->flags;
@@ -93,6 +91,9 @@ int vb2_secdata_set(struct vb2_context *ctx,
 	struct vb2_secdata *sec = (struct vb2_secdata *)ctx->secdata;
 	uint32_t now;
 
+	if (!(vb2_get_sd(ctx)->status & VB2_SD_STATUS_SECDATA_INIT))
+		return VB2_ERROR_SECDATA_SET_UNINITIALIZED;
+
 	/* If not changing the value, don't regenerate the CRC. */
 	if (vb2_secdata_get(ctx, param, &now) == VB2_SUCCESS && now == value)
 		return VB2_SUCCESS;
diff --git a/firmware/2lib/include/2return_codes.h b/firmware/2lib/include/2return_codes.h
index 4f56bdb0..0c368ded 100644
--- a/firmware/2lib/include/2return_codes.h
+++ b/firmware/2lib/include/2return_codes.h
@@ -102,6 +102,12 @@ enum vb2_return_code {
 	/* Invalid flags passed to vb2_secdata_set() */
 	VB2_ERROR_SECDATA_SET_FLAGS,
 
+	/* Called vb2_secdata_get() with uninitialized secdata */
+	VB2_ERROR_SECDATA_GET_UNINITIALIZED,
+
+	/* Called vb2_secdata_set() with uninitialized secdata */
+	VB2_ERROR_SECDATA_SET_UNINITIALIZED,
+
         /**********************************************************************
 	 * Common code errors
 	 */
diff --git a/tests/vb2_secdata_tests.c b/tests/vb2_secdata_tests.c
index 51283317..ca2a1ec9 100644
--- a/tests/vb2_secdata_tests.c
+++ b/tests/vb2_secdata_tests.c
@@ -15,6 +15,7 @@
 
 #include "2common.h"
 #include "2api.h"
+#include "2misc.h"
 #include "2secdata.h"
 
 static void test_changed(struct vb2_context *ctx, int changed, const char *why)
@@ -35,7 +36,6 @@ static void secdata_test(void)
 		.workbuf = workbuf,
 		.workbuf_size = sizeof(workbuf),
 	};
-	struct vb2_secdata *s = (struct vb2_secdata *)c.secdata;
 	uint32_t v = 1;
 
 	/* Blank data is invalid */
@@ -58,12 +58,6 @@ static void secdata_test(void)
 	TEST_EQ(vb2_secdata_init(&c),
 		 VB2_ERROR_SECDATA_CRC, "Init invalid CRC");
 
-	/* Version 1 didn't have a CRC, so init should reject it */
-	vb2_secdata_create(&c);
-	s->struct_version = 1;
-	TEST_EQ(vb2_secdata_init(&c),
-		VB2_ERROR_SECDATA_VERSION, "Init old version");
-
 	vb2_secdata_create(&c);
 	c.flags = 0;
 
@@ -101,6 +95,15 @@ static void secdata_test(void)
 	TEST_EQ(vb2_secdata_set(&c, -1, 456),
 		VB2_ERROR_SECDATA_SET_PARAM, "Set invalid");
 	test_changed(&c, 0, "Set invalid field doesn't change data");
+
+	/* Read/write uninitialized data fails */
+	vb2_get_sd(&c)->status &= ~VB2_SD_STATUS_SECDATA_INIT;
+	TEST_EQ(vb2_secdata_get(&c, VB2_SECDATA_VERSIONS, &v),
+		VB2_ERROR_SECDATA_GET_UNINITIALIZED, "Get uninitialized");
+	test_changed(&c, 0, "Get uninitialized doesn't change data");
+	TEST_EQ(vb2_secdata_set(&c, VB2_SECDATA_VERSIONS, 0x123456ff),
+		VB2_ERROR_SECDATA_SET_UNINITIALIZED, "Set uninitialized");
+	test_changed(&c, 0, "Set uninitialized doesn't change data");
 }
 
 int main(int argc, char* argv[])