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>

5 files changed, 50 insertions, 47 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.