summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJoel Kitching <kitching@google.com>2019-08-20 15:00:40 +0800
committerCommit Bot <commit-bot@chromium.org>2019-08-28 07:32:02 +0000
commit0ff87642e865fca49fa9584cffa1b0c4810adced (patch)
treed8c72d0c1a816bab362132930228751cd77a4be9
parent4539726499d6ee43077918eee3e1768040b45983 (diff)
downloadvboot-0ff87642e865fca49fa9584cffa1b0c4810adced.tar.gz
vboot/secdata: fix up 2secdata{,k} and tests
These are not yet used in production and need some fixing up first. BUG=b:124141368, chromium:972956 TEST=make clean && make runtests BRANCH=none Change-Id: Ifbd0e761cc5bc05437bfed774fb15d5e8ef1b8e7 Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1758149 Tested-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org>
-rw-r--r--firmware/2lib/2secdata.c25
-rw-r--r--firmware/2lib/2secdatak.c35
-rw-r--r--firmware/2lib/include/2api.h26
-rw-r--r--firmware/2lib/include/2return_codes.h4
-rw-r--r--firmware/2lib/include/2secdata.h7
-rw-r--r--tests/vb2_secdata_tests.c45
-rw-r--r--tests/vb2_secdatak_tests.c58
7 files changed, 110 insertions, 90 deletions
diff --git a/firmware/2lib/2secdata.c b/firmware/2lib/2secdata.c
index aaca7683..4493fbf3 100644
--- a/firmware/2lib/2secdata.c
+++ b/firmware/2lib/2secdata.c
@@ -11,18 +11,21 @@
#include "2misc.h"
#include "2secdata.h"
-vb2_error_t vb2api_secdata_check(const struct vb2_context *ctx)
+vb2_error_t vb2api_secdata_check(struct vb2_context *ctx)
{
- const struct vb2_secdata *sec =
- (const struct vb2_secdata *)ctx->secdata;
+ struct vb2_secdata *sec = (struct vb2_secdata *)ctx->secdata;
/* Verify CRC */
- if (sec->crc8 != vb2_crc8(sec, offsetof(struct vb2_secdata, crc8)))
+ if (sec->crc8 != vb2_crc8(sec, offsetof(struct vb2_secdata, crc8))) {
+ VB2_DEBUG("secdata_firmware: bad CRC\n");
return VB2_ERROR_SECDATA_CRC;
+ }
- /* CRC(<000...00>) is 0, so check version as well (should never be 0) */
- if (!sec->struct_version)
- return VB2_ERROR_SECDATA_ZERO;
+ /* Verify version */
+ if (sec->struct_version < VB2_SECDATA_VERSION) {
+ VB2_DEBUG("secdata_firmware: version incompatible\n");
+ return VB2_ERROR_SECDATA_VERSION;
+ }
return VB2_SUCCESS;
}
@@ -54,7 +57,6 @@ vb2_error_t vb2_secdata_init(struct vb2_context *ctx)
/* 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,
@@ -68,9 +70,10 @@ vb2_error_t vb2_secdata_init(struct vb2_context *ctx)
vb2_error_t vb2_secdata_get(struct vb2_context *ctx,
enum vb2_secdata_param param, uint32_t *dest)
{
+ struct vb2_shared_data *sd = vb2_get_sd(ctx);
struct vb2_secdata *sec = (struct vb2_secdata *)ctx->secdata;
- if (!(vb2_get_sd(ctx)->status & VB2_SD_STATUS_SECDATA_INIT))
+ if (!(sd->status & VB2_SD_STATUS_SECDATA_INIT))
return VB2_ERROR_SECDATA_GET_UNINITIALIZED;
switch(param) {
@@ -106,10 +109,14 @@ vb2_error_t vb2_secdata_set(struct vb2_context *ctx,
if (value > 0xff)
return VB2_ERROR_SECDATA_SET_FLAGS;
+ VB2_DEBUG("secdata flags updated from 0x%x to 0x%x\n",
+ sec->flags, value);
sec->flags = value;
break;
case VB2_SECDATA_VERSIONS:
+ VB2_DEBUG("secdata versions updated from 0x%x to 0x%x\n",
+ sec->fw_versions, value);
sec->fw_versions = value;
break;
diff --git a/firmware/2lib/2secdatak.c b/firmware/2lib/2secdatak.c
index d965eb44..f6bc4c82 100644
--- a/firmware/2lib/2secdatak.c
+++ b/firmware/2lib/2secdatak.c
@@ -11,14 +11,27 @@
#include "2misc.h"
#include "2secdata.h"
-vb2_error_t vb2api_secdatak_check(const struct vb2_context *ctx)
+vb2_error_t vb2api_secdatak_check(struct vb2_context *ctx)
{
- const struct vb2_secdatak *sec =
- (const struct vb2_secdatak *)ctx->secdatak;
+ struct vb2_secdatak *sec = (struct vb2_secdatak *)ctx->secdatak;
/* Verify CRC */
- if (sec->crc8 != vb2_crc8(sec, offsetof(struct vb2_secdatak, crc8)))
+ if (sec->crc8 != vb2_crc8(sec, offsetof(struct vb2_secdatak, crc8))) {
+ VB2_DEBUG("secdata_kernel: bad CRC\n");
return VB2_ERROR_SECDATAK_CRC;
+ }
+
+ /* Verify version */
+ if (sec->struct_version < VB2_SECDATAK_VERSION) {
+ VB2_DEBUG("secdata_firmware: version incompatible\n");
+ return VB2_ERROR_SECDATAK_VERSION;
+ }
+
+ /* Verify UID */
+ if (sec->uid != VB2_SECDATAK_UID) {
+ VB2_DEBUG("secdata_kernel: bad UID\n");
+ return VB2_ERROR_SECDATAK_UID;
+ }
return VB2_SUCCESS;
}
@@ -44,7 +57,6 @@ vb2_error_t vb2api_secdatak_create(struct vb2_context *ctx)
vb2_error_t vb2_secdatak_init(struct vb2_context *ctx)
{
- struct vb2_secdatak *sec = (struct vb2_secdatak *)ctx->secdatak;
struct vb2_shared_data *sd = vb2_get_sd(ctx);
vb2_error_t rv;
@@ -52,13 +64,8 @@ vb2_error_t vb2_secdatak_init(struct vb2_context *ctx)
if (rv)
return rv;
- /* Make sure the UID is correct */
- if (sec->uid != VB2_SECDATAK_UID)
- return VB2_ERROR_SECDATAK_UID;
-
/* Set status flag */
sd->status |= VB2_SD_STATUS_SECDATAK_INIT;
- /* TODO: unit test for that */
return VB2_SUCCESS;
}
@@ -66,9 +73,10 @@ vb2_error_t vb2_secdatak_init(struct vb2_context *ctx)
vb2_error_t vb2_secdatak_get(struct vb2_context *ctx,
enum vb2_secdatak_param param, uint32_t *dest)
{
+ struct vb2_shared_data *sd = vb2_get_sd(ctx);
struct vb2_secdatak *sec = (struct vb2_secdatak *)ctx->secdatak;
- if (!(vb2_get_sd(ctx)->status & VB2_SD_STATUS_SECDATAK_INIT))
+ if (!(sd->status & VB2_SD_STATUS_SECDATAK_INIT))
return VB2_ERROR_SECDATAK_GET_UNINITIALIZED;
switch(param) {
@@ -84,10 +92,11 @@ vb2_error_t vb2_secdatak_get(struct vb2_context *ctx,
vb2_error_t vb2_secdatak_set(struct vb2_context *ctx,
enum vb2_secdatak_param param, uint32_t value)
{
+ struct vb2_shared_data *sd = vb2_get_sd(ctx);
struct vb2_secdatak *sec = (struct vb2_secdatak *)ctx->secdatak;
uint32_t now;
- if (!(vb2_get_sd(ctx)->status & VB2_SD_STATUS_SECDATAK_INIT))
+ if (!(sd->status & VB2_SD_STATUS_SECDATAK_INIT))
return VB2_ERROR_SECDATAK_SET_UNINITIALIZED;
/* If not changing the value, don't regenerate the CRC. */
@@ -96,6 +105,8 @@ vb2_error_t vb2_secdatak_set(struct vb2_context *ctx,
switch(param) {
case VB2_SECDATAK_VERSIONS:
+ VB2_DEBUG("secdatak versions updated from 0x%x to 0x%x\n",
+ sec->kernel_versions, value);
sec->kernel_versions = value;
break;
diff --git a/firmware/2lib/include/2api.h b/firmware/2lib/include/2api.h
index 98f9ff08..12aa14e8 100644
--- a/firmware/2lib/include/2api.h
+++ b/firmware/2lib/include/2api.h
@@ -393,18 +393,14 @@ enum vb2_pcr_digest {
*/
/**
- * Check the CRC of the secure storage context.
+ * Check the validity of the secure storage context.
*
- * Use this if reading from secure storage may be flaky, and you want to retry
- * reading it several times.
- *
- * This may be called before vb2api_phase1() (externally), and before
- * vb2_context_init() (internally).
+ * Checks version and CRC.
*
* @param ctx Context pointer
* @return VB2_SUCCESS, or non-zero error code if error.
*/
-vb2_error_t vb2api_secdata_check(const struct vb2_context *ctx);
+vb2_error_t vb2api_secdata_check(struct vb2_context *ctx);
/**
* Create fresh data in the secure storage context.
@@ -414,27 +410,20 @@ vb2_error_t vb2api_secdata_check(const struct vb2_context *ctx);
* (or any other API in this library) fails; that could allow the secure data
* to be rolled back to an insecure state.
*
- * This may be called before vb2api_phase1() (externally), and before
- * vb2_context_init() (internally).
- *
* @param ctx Context pointer
* @return VB2_SUCCESS, or non-zero error code if error.
*/
vb2_error_t vb2api_secdata_create(struct vb2_context *ctx);
/**
- * Check the CRC of the kernel version secure storage context.
+ * Check the validity of the kernel version secure storage context.
*
- * Use this if reading from secure storage may be flaky, and you want to retry
- * reading it several times.
- *
- * This may be called before vb2api_phase1() (externally), and before
- * vb2_context_init() (internally).
+ * Checks version, UID, and CRC.
*
* @param ctx Context pointer
* @return VB2_SUCCESS, or non-zero error code if error.
*/
-vb2_error_t vb2api_secdatak_check(const struct vb2_context *ctx);
+vb2_error_t vb2api_secdatak_check(struct vb2_context *ctx);
/**
* Create fresh data in the kernel version secure storage context.
@@ -444,9 +433,6 @@ vb2_error_t vb2api_secdatak_check(const struct vb2_context *ctx);
* (or any other API in this library) fails; that could allow the secure data
* to be rolled back to an insecure state.
*
- * This may be called before vb2api_phase1() (externally), and before
- * vb2_context_init() (internally).
- *
* @param ctx Context pointer
* @return VB2_SUCCESS, or non-zero error code if error.
*/
diff --git a/firmware/2lib/include/2return_codes.h b/firmware/2lib/include/2return_codes.h
index 4aa3f5dd..68edff38 100644
--- a/firmware/2lib/include/2return_codes.h
+++ b/firmware/2lib/include/2return_codes.h
@@ -155,8 +155,8 @@ enum vb2_return_code {
/* Bad CRC in vb2api_secdata_check() */
VB2_ERROR_SECDATA_CRC,
- /* Secdata is all zeroes (uninitialized) in vb2api_secdata_check() */
- VB2_ERROR_SECDATA_ZERO,
+ /* Bad struct version in vb2_secdata_check() */
+ VB2_ERROR_SECDATA_VERSION,
/* Invalid param in vb2_secdata_get() */
VB2_ERROR_SECDATA_GET_PARAM,
diff --git a/firmware/2lib/include/2secdata.h b/firmware/2lib/include/2secdata.h
index 90f4bf23..bcd308b9 100644
--- a/firmware/2lib/include/2secdata.h
+++ b/firmware/2lib/include/2secdata.h
@@ -5,13 +5,12 @@
* Secure non-volatile storage routines
*/
-#ifndef VBOOT_REFERENCE_VBOOT_SECDATA_H_
-#define VBOOT_REFERENCE_VBOOT_SECDATA_H_
+#ifndef VBOOT_REFERENCE_VBOOT_2SECDATA_H_
+#define VBOOT_REFERENCE_VBOOT_2SECDATA_H_
/*****************************************************************************/
/* Firmware version space */
-/* Expected value of vb2_secdata.version */
#define VB2_SECDATA_VERSION 2
/* Flags for firmware space */
@@ -124,7 +123,7 @@ vb2_error_t vb2_secdata_set(struct vb2_context *ctx,
enum vb2_secdata_param param, uint32_t value);
/*****************************************************************************/
-/* Kernel version space functions.
+/* Kernel version space functions
*
* These are separate functions so that they don't bloat the size of the early
* boot code which uses the firmware version space functions.
diff --git a/tests/vb2_secdata_tests.c b/tests/vb2_secdata_tests.c
index add99938..fca31d4d 100644
--- a/tests/vb2_secdata_tests.c
+++ b/tests/vb2_secdata_tests.c
@@ -5,20 +5,14 @@
* Tests for firmware secure storage library.
*/
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include "2sysincludes.h"
-
-#include "test_common.h"
-#include "vboot_common.h"
-
-#include "2common.h"
#include "2api.h"
+#include "2common.h"
+#include "2crc8.h"
#include "2misc.h"
#include "2secdata.h"
+#include "2sysincludes.h"
+#include "test_common.h"
+#include "vboot_common.h"
static void test_changed(struct vb2_context *c, int changed, const char *why)
{
@@ -39,9 +33,11 @@ static void secdata_test(void)
.workbuf = workbuf,
.workbuf_size = sizeof(workbuf),
};
+ struct vb2_secdata *sec = (struct vb2_secdata *)c.secdata;
+ struct vb2_shared_data *sd = vb2_get_sd(&c);
uint32_t v = 1;
- /* Check size constant */
+ /* Check size constant */
TEST_EQ(VB2_SECDATA_SIZE, sizeof(struct vb2_secdata),
"Struct size constant");
@@ -50,16 +46,29 @@ static void secdata_test(void)
TEST_EQ(vb2api_secdata_check(&c),
VB2_ERROR_SECDATA_CRC, "Check blank CRC");
TEST_EQ(vb2_secdata_init(&c),
- VB2_ERROR_SECDATA_CRC, "Init blank CRC");
+ VB2_ERROR_SECDATA_CRC, "Init blank CRC");
/* Ensure zeroed buffers are invalid (coreboot relies on this) */
memset(c.secdata, 0, sizeof(c.secdata));
- TEST_EQ(vb2_secdata_init(&c), VB2_ERROR_SECDATA_ZERO, "Zeroed buffer");
+ TEST_EQ(vb2_secdata_init(&c), VB2_ERROR_SECDATA_VERSION,
+ "Zeroed buffer (invalid version)");
+
+ /* Try with bad version */
+ TEST_SUCC(vb2api_secdata_create(&c), "Create");
+ sec->struct_version -= 1;
+ sec->crc8 = vb2_crc8(sec, offsetof(struct vb2_secdata, crc8));
+ TEST_EQ(vb2api_secdata_check(&c),
+ VB2_ERROR_SECDATA_VERSION, "Check invalid version");
+ TEST_EQ(vb2_secdata_init(&c),
+ VB2_ERROR_SECDATA_VERSION, "Init invalid version");
/* Create good data */
TEST_SUCC(vb2api_secdata_create(&c), "Create");
TEST_SUCC(vb2api_secdata_check(&c), "Check created CRC");
TEST_SUCC(vb2_secdata_init(&c), "Init created CRC");
+ TEST_NEQ(sd->status & VB2_SD_STATUS_SECDATA_INIT, 0,
+ "Init set SD status");
+ sd->status &= ~VB2_SD_STATUS_SECDATA_INIT;
test_changed(&c, 1, "Create changes data");
/* Now corrupt it */
@@ -67,12 +76,12 @@ static void secdata_test(void)
TEST_EQ(vb2api_secdata_check(&c),
VB2_ERROR_SECDATA_CRC, "Check invalid CRC");
TEST_EQ(vb2_secdata_init(&c),
- VB2_ERROR_SECDATA_CRC, "Init invalid CRC");
+ VB2_ERROR_SECDATA_CRC, "Init invalid CRC");
+ /* Read/write flags */
vb2api_secdata_create(&c);
+ vb2_secdata_init(&c);
c.flags = 0;
-
- /* Read/write flags */
TEST_SUCC(vb2_secdata_get(&c, VB2_SECDATA_FLAGS, &v), "Get flags");
TEST_EQ(v, 0, "Flags created 0");
test_changed(&c, 0, "Get doesn't change data");
@@ -108,7 +117,7 @@ static void secdata_test(void)
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;
+ sd->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");
diff --git a/tests/vb2_secdatak_tests.c b/tests/vb2_secdatak_tests.c
index df68351a..45803866 100644
--- a/tests/vb2_secdatak_tests.c
+++ b/tests/vb2_secdatak_tests.c
@@ -5,21 +5,14 @@
* Tests for kernel secure storage library.
*/
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include "2sysincludes.h"
-
-#include "test_common.h"
-#include "vboot_common.h"
-
-#include "2common.h"
#include "2api.h"
+#include "2common.h"
#include "2crc8.h"
#include "2misc.h"
#include "2secdata.h"
+#include "2sysincludes.h"
+#include "test_common.h"
+#include "vboot_common.h"
static void test_changed(struct vb2_context *c, int changed, const char *why)
{
@@ -40,9 +33,11 @@ static void secdatak_test(void)
.workbuf = workbuf,
.workbuf_size = sizeof(workbuf),
};
+ struct vb2_secdatak *sec = (struct vb2_secdatak *)c.secdatak;
+ struct vb2_shared_data *sd = vb2_get_sd(&c);
uint32_t v = 1;
- /* Check size constant */
+ /* Check size constant */
TEST_EQ(VB2_SECDATAK_SIZE, sizeof(struct vb2_secdatak),
"Struct size constant");
@@ -51,12 +46,29 @@ static void secdatak_test(void)
TEST_EQ(vb2api_secdatak_check(&c),
VB2_ERROR_SECDATAK_CRC, "Check blank CRC");
TEST_EQ(vb2_secdatak_init(&c),
- VB2_ERROR_SECDATAK_CRC, "Init blank CRC");
+ VB2_ERROR_SECDATAK_CRC, "Init blank CRC");
+
+ /* Ensure zeroed buffers are invalid */
+ memset(c.secdatak, 0, sizeof(c.secdatak));
+ TEST_EQ(vb2_secdatak_init(&c), VB2_ERROR_SECDATAK_VERSION,
+ "Zeroed buffer (invalid version)");
+
+ /* Try with bad version */
+ TEST_SUCC(vb2api_secdatak_create(&c), "Create");
+ sec->struct_version -= 1;
+ sec->crc8 = vb2_crc8(sec, offsetof(struct vb2_secdatak, crc8));
+ TEST_EQ(vb2api_secdatak_check(&c),
+ VB2_ERROR_SECDATAK_VERSION, "Check invalid version");
+ TEST_EQ(vb2_secdatak_init(&c),
+ VB2_ERROR_SECDATAK_VERSION, "Init invalid version");
/* Create good data */
TEST_SUCC(vb2api_secdatak_create(&c), "Create");
TEST_SUCC(vb2api_secdatak_check(&c), "Check created CRC");
TEST_SUCC(vb2_secdatak_init(&c), "Init created CRC");
+ TEST_NEQ(sd->status & VB2_SD_STATUS_SECDATAK_INIT, 0,
+ "Init set SD status");
+ sd->status &= ~VB2_SD_STATUS_SECDATAK_INIT;
test_changed(&c, 1, "Create changes data");
/* Now corrupt it */
@@ -64,22 +76,18 @@ static void secdatak_test(void)
TEST_EQ(vb2api_secdatak_check(&c),
VB2_ERROR_SECDATAK_CRC, "Check invalid CRC");
TEST_EQ(vb2_secdatak_init(&c),
- VB2_ERROR_SECDATAK_CRC, "Init invalid CRC");
+ VB2_ERROR_SECDATAK_CRC, "Init invalid CRC");
/* Make sure UID is checked */
- {
- struct vb2_secdatak *sec = (struct vb2_secdatak *)c.secdatak;
-
- vb2api_secdatak_create(&c);
- sec->uid++;
- sec->crc8 = vb2_crc8(sec, offsetof(struct vb2_secdatak, crc8));
-
- TEST_EQ(vb2_secdatak_init(&c), VB2_ERROR_SECDATAK_UID,
- "Init invalid struct UID");
- }
+ vb2api_secdatak_create(&c);
+ sec->uid++;
+ sec->crc8 = vb2_crc8(sec, offsetof(struct vb2_secdatak, crc8));
+ TEST_EQ(vb2_secdatak_init(&c), VB2_ERROR_SECDATAK_UID,
+ "Init invalid struct UID");
/* Read/write versions */
vb2api_secdatak_create(&c);
+ vb2_secdatak_init(&c);
c.flags = 0;
TEST_SUCC(vb2_secdatak_get(&c, VB2_SECDATAK_VERSIONS, &v),
"Get versions");
@@ -103,7 +111,7 @@ static void secdatak_test(void)
test_changed(&c, 0, "Set invalid field doesn't change data");
/* Read/write uninitialized data fails */
- vb2_get_sd(&c)->status &= ~VB2_SD_STATUS_SECDATAK_INIT;
+ sd->status &= ~VB2_SD_STATUS_SECDATAK_INIT;
TEST_EQ(vb2_secdatak_get(&c, VB2_SECDATAK_VERSIONS, &v),
VB2_ERROR_SECDATAK_GET_UNINITIALIZED, "Get uninitialized");
test_changed(&c, 0, "Get uninitialized doesn't change data");