From b550fb180487f161b3f704056f6e05a9cce9d308 Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Fri, 30 Jan 2015 14:27:54 -0800 Subject: [PATCH] vboot2: Fail vb2_secdata_(get|set) when secdata was not initialized 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 Reviewed-on: https://chromium-review.googlesource.com/244846 --- firmware/2lib/2secdata.c | 23 ++++++++++++----------- firmware/2lib/include/2return_codes.h | 6 ++++++ tests/vb2_secdata_tests.c | 17 ++++++++++------- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/firmware/2lib/2secdata.c b/firmware/2lib/2secdata.c index 0c5a34e41a..c2983e7122 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; - /* 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 + /* 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; + 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 e89897f2fd..7b54f6dbff 100644 --- a/firmware/2lib/include/2return_codes.h +++ b/firmware/2lib/include/2return_codes.h @@ -105,6 +105,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 d4344b0b69..94f92fb7f9 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) @@ -36,7 +37,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 */ @@ -59,12 +59,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; @@ -102,6 +96,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[])