From 0ff87642e865fca49fa9584cffa1b0c4810adced Mon Sep 17 00:00:00 2001 From: Joel Kitching Date: Tue, 20 Aug 2019 15:00:40 +0800 Subject: 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1758149 Tested-by: Joel Kitching Commit-Queue: Joel Kitching Reviewed-by: Julius Werner --- firmware/2lib/2secdata.c | 25 +++++++++------ firmware/2lib/2secdatak.c | 35 +++++++++++++-------- firmware/2lib/include/2api.h | 26 ++++------------ firmware/2lib/include/2return_codes.h | 4 +-- firmware/2lib/include/2secdata.h | 7 ++--- tests/vb2_secdata_tests.c | 45 ++++++++++++++++----------- tests/vb2_secdatak_tests.c | 58 ++++++++++++++++++++--------------- 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 -#include -#include -#include - -#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 -#include -#include -#include - -#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"); -- cgit v1.2.1