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>

2 files changed, 60 insertions, 43 deletions

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");