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 <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>
diff --git a/firmware/2lib/2secdata.c b/firmware/2lib/2secdata.c
index 0c5a34e..c2983e7 100644
--- a/firmware/2lib/2secdata.c
+++ b/firmware/2lib/2secdata.c
@@ -42,27 +42,22 @@
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 @@
{
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 @@
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 4f56bdb..0c368de 100644
--- a/firmware/2lib/include/2return_codes.h
+++ b/firmware/2lib/include/2return_codes.h
@@ -102,6 +102,12 @@
/* 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 5128331..ca2a1ec 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 @@
.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 @@
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 @@
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[])
