vboot2: un-nest data structures

Originally, we designed the vboot data structures so that some of them
had sub-structures.  Then the variable-length data for each of the
structures was at the end.  So:

    struct vb2_keyblock {
      struct vb2_packed_key
      struct vb2_signature
    }
    // Followed by variable-length data for keyblock
    // Followed by variable-length data for packed key
    // Followed by variable-length data for signature

This had the weird side effect that the header and data for the
sub-structs were not contiguous.  That wasn't too bad before, but it
gets more complicated with the new data structures.  Each structure
now can also have a description.  And keyblocks can have a list of
signatures.

Structures also couldn't really know their own size, since a
sub-struct might have a 20-byte header, but then 2K of other data in
between that and the data for the sub-struct itself.

So, un-nest all the data structures.  That is, the keyblock now
contains the offset of the signature struct, rather than the signature
struct itself.  And then all the variable-length data for each struct
immediately follows the struct itself.  So:

    struct vb2_keyblock2 {
      // Offset of packed key
      // Offset of first signature
    }
    // Followed by variable-length data for keyblock
    struct vb2_packed_key
    // Followed by variable-length data for packed key
    struct vb2_signature2
    // Followed by variable-length data for signature (desc, sig data)

Verifying and traversing these objects is much more straightforward.
And each struct can now know its own size.

This first change rearranges the structures.  Descriptions now
immediately follow the fixed size structure headers.

The next change adds better verification of the structures, using the
fixed_size and total_size fields in the common header.

BUG=chromium:423882
BRANCH=none
TEST=VBOOT2=1 make runtests

Change-Id: Ieb9148d6f26c3e59ea542f3a95e59d8019ccee21
Reviewed-on: https://chromium-review.googlesource.com/226824
Tested-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Bill Richardson <wfrichar@chromium.org>
Commit-Queue: Randall Spangler <rspangler@chromium.org>

7 files changed, 89 insertions, 111 deletions

diff --git a/firmware/2lib/2common.c b/firmware/2lib/2common.c
index 6360ef09..221e6115 100644
--- a/firmware/2lib/2common.c
+++ b/firmware/2lib/2common.c
@@ -168,20 +168,16 @@ int vb2_verify_common_header(const void *parent,
 	/* Make sure common data and description are inside parent */
 	rv = vb2_verify_member_inside(parent, parent_size,
 				      c, sizeof(*c),
-				      c->desc_offset, c->desc_size);
+				      c->fixed_size, c->desc_size);
 	if (rv)
 		return rv;
 
 	/* Check description */
 	if (c->desc_size > 0) {
 		/* Description must be null-terminated */
-		const uint8_t *cptr = (const uint8_t *)c;
-		if (cptr[c->desc_offset + c->desc_size - 1] != 0)
+		const uint8_t *desc = (const uint8_t *)c + c->fixed_size;
+		if (desc[c->desc_size - 1] != 0)
 			return VB2_ERROR_DESC_TERMINATOR;
-
-	} else if (c->desc_offset != 0) {
-		/* If size is non-zero, offset must be too */
-		return VB2_ERROR_DESC_EMPTY_OFFSET;
 	}
 
 	return VB2_SUCCESS;
diff --git a/firmware/2lib/2packed_key2.c b/firmware/2lib/2packed_key2.c
index 306206ee..22fa0a40 100644
--- a/firmware/2lib/2packed_key2.c
+++ b/firmware/2lib/2packed_key2.c
@@ -98,7 +98,7 @@ int vb2_unpack_key2(struct vb2_public_key *key,
 
 	/* Key description */
 	if (pkey->c.desc_size)
-		key->desc = (const char *)&(pkey->c) + pkey->c.desc_offset;
+		key->desc = (const char *)&(pkey->c) + pkey->c.fixed_size;
 	else
 		key->desc = "";
 
diff --git a/firmware/2lib/include/2return_codes.h b/firmware/2lib/include/2return_codes.h
index e75f4222..aca6b715 100644
--- a/firmware/2lib/include/2return_codes.h
+++ b/firmware/2lib/include/2return_codes.h
@@ -170,9 +170,6 @@ enum vb2_return_code {
 	/* Common struct description is not null-terminated */
 	VB2_ERROR_DESC_TERMINATOR,
 
-	/* Common struct description offset is empty, but offset is non-zero */
-	VB2_ERROR_DESC_EMPTY_OFFSET,
-
 	/* Member data overlaps member header */
 	VB2_ERROR_INSIDE_DATA_OVERLAP,
 
diff --git a/firmware/2lib/include/2struct.h b/firmware/2lib/include/2struct.h
index d1ee925e..be5ba034 100644
--- a/firmware/2lib/include/2struct.h
+++ b/firmware/2lib/include/2struct.h
@@ -90,11 +90,6 @@ struct vb2_signature {
 	/* Size of the data block which was signed in bytes */
 	uint32_t data_size;
 	uint32_t reserved2;
-
-	/*
-	 * TODO: when redoing this struct, add a text description of the
-	 * signature (including what key was used) and an algorithm type field.
-	 */
 } __attribute__((packed));
 
 #define EXPECTED_VB2_SIGNATURE_SIZE 24
@@ -262,14 +257,15 @@ enum vb2_struct_common_magic {
 
 /*
  * Generic struct header for all vboot2 structs.  This makes it easy to
- * automatically parse and identify vboot structs (e.g., in futility).
+ * automatically parse and identify vboot structs (e.g., in futility).  This
+ * must be the first member of the parent vboot2 struct.
  */
 struct vb2_struct_common {
 	/* Magic number; see vb2_struct_common_magic for expected values */
 	uint32_t magic;
 
 	/*
-	 * Struct version; see each struct for the expected value.
+	 * Parent struct version; see each struct for the expected value.
 	 *
 	 * How to handle struct version mismatches, if the parser is version
 	 * A.b and the data is version C.d:
@@ -289,20 +285,34 @@ struct vb2_struct_common {
 	uint16_t struct_version_minor;
 
 	/*
-	 * Offset of null-terminated ASCII description from start of struct.
-	 * Must be 0 if desc_size=0.
+	 * Size of the parent structure and all its data, including the
+	 * description and any necessary padding.  That is, all data must lie
+	 * in a contiguous region of <total_size> bytes starting at the first
+	 * byte of this header.
+	 */
+	uint32_t total_size;
+
+	/*
+	 * Size of the fixed portion of the parent structure.  If a description
+	 * is present, it must start at this offset.
 	 */
-	uint32_t desc_offset;
+	uint32_t fixed_size;
 
 	/*
-	 * Size of description in bytes, counting null terminator.  Set 0 if no
-	 * description is present.  If non-zero, there must be a null
-	 * terminator (0) at offset (desc_offset + desc_size - 1).
+	 * The object may contain an ASCII description following the fixed
+	 * portion of the structure.  If it is present, it must be
+	 * null-terminated, and padded with 0 (null) bytes to a multiple of 32
+	 * bits.
+	 *
+	 * Size of ASCII description in bytes, counting null terminator and
+	 * padding (if any).  Set 0 if no description is present.  If non-zero,
+	 * there must be a null terminator (0) at offset (fixed_size +
+	 * desc_size - 1).
 	 */
 	uint32_t desc_size;
 } __attribute__((packed));
 
-#define EXPECTED_VB2_STRUCT_COMMON_SIZE 16
+#define EXPECTED_VB2_STRUCT_COMMON_SIZE 20
 
 /* Algorithm types for signatures */
 enum vb2_signature_algorithm {
@@ -339,16 +349,19 @@ enum vb2_hash_algorithm {
 #define VB2_PACKED_KEY2_VERSION_MAJOR 3
 #define VB2_PACKED_KEY2_VERSION_MINOR 0
 
-/* Packed public key data, version 2 */
+/*
+ * Packed public key data, version 2
+ *
+ * The key data must be arranged like this:
+ *     1) vb2_packed_key2 header struct h
+ *     2) Key description (pointed to by h.c.fixed_size)
+ *     3) Key data key (pointed to by h.key_offset)
+ */
 struct vb2_packed_key2 {
 	/* Common header fields */
 	struct vb2_struct_common c;
 
-	/*
-	 * Offset of key data from start of this struct.  Note that data may
-	 * not immediately follow this struct header, if this struct is a
-	 * member of a bigger struct; see vb2_keyblock2 for an example.
-	 */
+	/* Offset of key data from start of this struct */
 	uint32_t key_offset;
 
 	/* Size of key data in bytes (NOT strength of key in bits) */
@@ -378,16 +391,19 @@ struct vb2_packed_key2 {
 #define VB2_SIGNATURE2_VERSION_MAJOR 3
 #define VB2_SIGNATURE2_VERSION_MINOR 0
 
-/* Signature data, version 2 */
+/*
+ * Signature data, version 2
+ *
+ * The signature data must be arranged like this:
+ *     1) vb2_signature2 header struct h
+ *     2) Signature description (pointed to by h.c.fixed_size)
+ *     3) Signature data (pointed to by h.sig_offset)
+ */
 struct vb2_signature2 {
 	/* Common header fields */
 	struct vb2_struct_common c;
 
-	/*
-	 * Offset of signature data from start of this struct.  Note that data
-	 * may not immediately follow the struct header, if this struct is a
-	 * member of a bigger struct; see vb2_keyblock2 for an example.
-	 */
+	/* Offset of signature data from start of this struct */
 	uint32_t sig_offset;
 
 	/* Size of signature data in bytes */
@@ -408,16 +424,10 @@ struct vb2_signature2 {
 	 * with the key being used by the firmware.
 	 */
 	struct vb2_guid key_guid;
-
-	/* Offset of null-terminated ASCII description from start of struct */
-	uint32_t desc_offset;
-
-	/* Size of description in bytes, counting null terminator */
-	uint32_t desc_size;
 } __attribute__((packed));
 
 #define EXPECTED_VB2_SIGNATURE2_SIZE					\
-	(EXPECTED_VB2_STRUCT_COMMON_SIZE + EXPECTED_GUID_SIZE + 24)
+	(EXPECTED_VB2_STRUCT_COMMON_SIZE + EXPECTED_GUID_SIZE + 16)
 
 
 /* Current version of vb2_keyblock2 struct */
@@ -430,45 +440,35 @@ struct vb2_signature2 {
  *
  * The key block data must be arranged like this:
  *     1) vb2_keyblock2 header struct h
- *     2) The following in any order
- *        - Keyblock description (pointed to by h.c.desc_offset)
- *        - Data key description (pointed to by h.data_key.c.desc_offset)
- *        - Data key data (pointed to by h.data_key.key_offset)
- *        - Signature table (pointed to by h.signature_table_offset)
- *     3) Signature data and descriptions (pointed to by each signature table
- *        entry e's e.c.desc_offset and e.sig_offset)
+ *     2) Keyblock description (pointed to by h.c.fixed_size)
+ *     3) Data key (pointed to by h.data_key_offset)
+ *     4) Signatures (first signature pointed to by h.sig_offset)
  *
- * The signatures from 3) must cover all the data from 1) and 2).
+ * The signatures from 4) must cover all the data from 1), 2), 3).  That is,
+ * signatures must sign all data up to sig_offset.
  */
 struct vb2_keyblock2 {
 	/* Common header fields */
 	struct vb2_struct_common c;
 
-	/*
-	 * Size of this key block, including this header and all data (data
-	 * key, signatures, description from common header, padding, etc.), in
-	 * bytes.
-	 */
-	uint32_t keyblock_size;
-
 	/* Flags (VB2_KEY_BLOCK_FLAG_*) */
 	uint32_t flags;
 
-	/* Key to use in next stage of verification */
-	struct vb2_packed_key2 data_key;
+	/*
+	 * Offset of key (struct vb2_packed_key2) to use in next stage of
+	 * verification, from start of the keyblock.
+	 */
+	uint32_t key_offset;
 
 	/* Number of keyblock signatures which follow */
-	uint32_t signature_count;
-
-	/* Size of each signature table entry, in bytes */
-	uint32_t signature_entry_size;
+	uint32_t sig_count;
 
 	/*
-	 * Offset of signature table from the start of the keyblock.  The
-	 * signature table is an array of struct vb2_signature2 (version 3.0).
+	 * Offset of the first signature (struct vb2_signature2) from the start
+	 * of the keyblock.
 	 *
 	 * Signatures sign the contents of this struct and the data pointed to
-	 * by data_key (but not the signature table/data).
+	 * by data_key_offset, but not themselves or other signatures.
 	 *
 	 * For the firmware, there may be only one signature.
 	 *
@@ -476,11 +476,10 @@ struct vb2_keyblock2 {
 	 * subkey from the RW firmware (for signed kernels) and one which is
 	 * simply a SHA-512 hash (for unsigned developer kernels).
 	 */
-	uint32_t signature_table_offset;
+	uint32_t sig_offset;
 } __attribute__((packed));
 
-#define EXPECTED_VB2_KEYBLOCK2_SIZE					\
-	(EXPECTED_VB2_STRUCT_COMMON_SIZE + EXPECTED_VB2_PACKED_KEY2_SIZE + 20)
+#define EXPECTED_VB2_KEYBLOCK2_SIZE (EXPECTED_VB2_STRUCT_COMMON_SIZE + 16)
 
 
 /* Current version of vb2_preamble2 struct */
@@ -488,7 +487,7 @@ struct vb2_keyblock2 {
 #define VB2_PREAMBLE2_VERSION_MINOR 0
 
 /* Single hash entry for the firmware preamble */
-struct vb2_fw_preamble_hash {
+struct vb2_fw_preamble2_hash {
 	/* Type of data being hashed (enum vb2api_hash_tag) */
 	uint32_t tag;
 
@@ -499,39 +498,31 @@ struct vb2_fw_preamble_hash {
 	uint8_t digest[0];
 } __attribute__((packed));
 
-#define EXPECTED_VB2_FW_PREAMBLE_HASH_SIZE 8
+#define EXPECTED_VB2_FW_PREAMBLE2_HASH_SIZE 8
 
 /*
  * Firmware preamble
  *
  * The preamble data must be arranged like this:
  *     1) vb2_fw_preamble2 header struct h
- *     2) The following in any order
- *        - Preamble description (pointed to by h.c.desc_offset)
- *        - Hash table (pointed to by h.hash_table_offset)
- *     3) Signature data and description (pointed to by
- *	  h.preamble_signature.desc_offset and h.preamble_signature.sig_offset)
+ *     2) Preamble description (pointed to by h.c.fixed_size)
+ *     3) Hash table (pointed to by h.hash_table_offset)
+ *     4) Signature (pointed to by h.sig_offset)
  *
- * The signature 3) must cover all the data from 1) and 2).
+ * The signature 4) must cover all the data from 1), 2), 3).
  */
 struct vb2_fw_preamble2 {
 	/* Common header fields */
 	struct vb2_struct_common c;
 
-	/*
-	 * Size of the preamble, including this header struct and all data
-	 * (hashes, signatures, descriptions, padding, etc.), in bytes.
-	 */
-	uint32_t preamble_size;
-
 	/* Flags; see VB2_FIRMWARE_PREAMBLE_* */
 	uint32_t flags;
 
 	/* Firmware version */
 	uint32_t firmware_version;
 
-	/* Signature for this preamble (header + hash table entries) */
-	struct vb2_signature2 preamble_signature;
+	/* Offset of signature (struct vb2_signature2) for this preamble */
+	uint32_t sig_offset;
 
 	/*
 	 * The preamble contains a list of hashes for the various firmware
@@ -561,8 +552,7 @@ struct vb2_fw_preamble2 {
 	uint32_t hash_table_offset;
 } __attribute__((packed));
 
-#define EXPECTED_VB2_FW_PREAMBLE2_SIZE					\
-	(EXPECTED_VB2_STRUCT_COMMON_SIZE + EXPECTED_VB2_SIGNATURE2_SIZE + 24)
+#define EXPECTED_VB2_FW_PREAMBLE2_SIZE (EXPECTED_VB2_STRUCT_COMMON_SIZE + 24)
 
 /****************************************************************************/
 
diff --git a/tests/vb2_common2_tests.c b/tests/vb2_common2_tests.c
index f58d2a35..c2d11d99 100644
--- a/tests/vb2_common2_tests.c
+++ b/tests/vb2_common2_tests.c
@@ -104,7 +104,7 @@ static void test_unpack_key2(const VbPublicKey *orig_key)
 	free(key2);
 
 	key2 = vb2_convert_packed_key2(key1, "Test key", &size);
-	key2->c.desc_offset += size;
+	key2->c.fixed_size += size;
 	TEST_EQ(vb2_unpack_key2(&pubk, (uint8_t *)key2, size),
 		VB2_ERROR_INSIDE_DATA_OUTSIDE,
 		"vb2_unpack_key2() buffer too small for desc");
@@ -112,7 +112,6 @@ static void test_unpack_key2(const VbPublicKey *orig_key)
 
 	key2 = vb2_convert_packed_key2(key1, "Test key", &size);
 	key2->c.desc_size = 0;
-	key2->c.desc_offset = 0;
 	TEST_SUCC(vb2_unpack_key2(&pubk, (uint8_t *)key2, size),
 		  "vb2_unpack_key2() no desc");
 	TEST_EQ(strcmp(pubk.desc, ""), 0, "  empty desc string");
diff --git a/tests/vb2_common_tests.c b/tests/vb2_common_tests.c
index 5a8cb202..31fa036d 100644
--- a/tests/vb2_common_tests.c
+++ b/tests/vb2_common_tests.c
@@ -163,9 +163,9 @@ static void test_struct_packing(void)
 	TEST_EQ(EXPECTED_VB2_FW_PREAMBLE2_SIZE,
 		sizeof(struct vb2_fw_preamble2),
 		"sizeof(vb2_fw_preamble2)");
-	TEST_EQ(EXPECTED_VB2_FW_PREAMBLE_HASH_SIZE,
-		sizeof(struct vb2_fw_preamble_hash),
-		"sizeof(vb2_fw_preamble_hash)");
+	TEST_EQ(EXPECTED_VB2_FW_PREAMBLE2_HASH_SIZE,
+		sizeof(struct vb2_fw_preamble2_hash),
+		"sizeof(vb2_fw_preamble2_hash)");
 }
 
 /**
@@ -246,13 +246,14 @@ static void test_helper_functions(void)
 		uint8_t cbuf[sizeof(struct vb2_struct_common) + 128];
 		struct vb2_struct_common *c = (struct vb2_struct_common *)cbuf;
 
-		c->desc_offset = sizeof(*c);
+		c->total_size = sizeof(cbuf);
+		c->fixed_size = sizeof(*c);
 		c->desc_size = 128;
 		cbuf[sizeof(cbuf) - 1] = 0;
 		TEST_SUCC(vb2_verify_common_header(cbuf, sizeof(cbuf), c),
 			  "CommonInside at start");
 
-		c[1].desc_offset = sizeof(*c);
+		c[1].fixed_size = sizeof(*c);
 		c[1].desc_size = 128 - sizeof(*c);
 		TEST_SUCC(vb2_verify_common_header(cbuf, sizeof(cbuf), c + 1),
 			  "CommonInside after start");
@@ -261,11 +262,11 @@ static void test_helper_functions(void)
 			VB2_ERROR_INSIDE_DATA_OUTSIDE,
 			"CommonInside key too big");
 
-		c->desc_offset = sizeof(cbuf);
+		c->fixed_size = sizeof(cbuf);
 		TEST_EQ(vb2_verify_common_header(cbuf, sizeof(cbuf), c),
 			VB2_ERROR_INSIDE_DATA_OUTSIDE,
 			"CommonInside offset too big");
-		c->desc_offset = sizeof(*c);
+		c->fixed_size = sizeof(*c);
 
 		cbuf[sizeof(cbuf) - 1] = 1;
 		TEST_EQ(vb2_verify_common_header(cbuf, sizeof(cbuf), c),
@@ -273,14 +274,8 @@ static void test_helper_functions(void)
 			"CommonInside description not terminated");
 
 		c->desc_size = 0;
-		c->desc_offset = 0;
 		TEST_SUCC(vb2_verify_common_header(cbuf, sizeof(cbuf), c),
 			  "CommonInside no description");
-
-		c->desc_offset = 4;
-		TEST_EQ(vb2_verify_common_header(cbuf, sizeof(cbuf), c),
-			VB2_ERROR_DESC_EMPTY_OFFSET,
-			"CommonInside description empty offset");
 	}
 
 	{
diff --git a/tests/vb2_convert_structs.c b/tests/vb2_convert_structs.c
index fe74f85f..cb29a979 100644
--- a/tests/vb2_convert_structs.c
+++ b/tests/vb2_convert_structs.c
@@ -24,20 +24,21 @@ struct vb2_packed_key2 *vb2_convert_packed_key2(
 	};
 	uint8_t *kbuf;
 
-	/* Calculate description size */
-	k2.c.desc_offset = sizeof(k2);
+	/* Calculate sizes and offsets */
+	k2.c.fixed_size = sizeof(k2);
 	k2.c.desc_size = roundup32(strlen(desc) + 1);
+	k2.key_offset = k2.c.fixed_size + k2.c.desc_size;
+	k2.key_size = key->key_size;
+	k2.c.total_size = k2.key_offset + k2.key_size;
 
 	/* Copy/initialize fields */
-	k2.key_offset = k2.c.desc_offset + k2.c.desc_size;
-	k2.key_size = key->key_size;
 	k2.key_version = key->key_version;
 	k2.sig_algorithm = vb2_crypto_to_signature(key->algorithm);
 	k2.hash_algorithm = vb2_crypto_to_hash(key->algorithm);
 	/* TODO: fill in a non-zero GUID */
 
 	/* Allocate the new buffer */
-	*out_size = k2.key_offset + k2.key_size;
+	*out_size = k2.c.total_size;
 	kbuf = malloc(*out_size);
 	memset(kbuf, 0, *out_size);
 
@@ -45,8 +46,8 @@ struct vb2_packed_key2 *vb2_convert_packed_key2(
 	memcpy(kbuf, &k2, sizeof(k2));
 
 	/* strcpy() is safe because we allocated above based on strlen() */
-	strcpy((char *)(kbuf + k2.c.desc_offset), desc);
-	kbuf[k2.c.desc_offset + k2.c.desc_size - 1] = 0;
+	strcpy((char *)(kbuf + k2.c.fixed_size), desc);
+	kbuf[k2.c.fixed_size + k2.c.desc_size - 1] = 0;
 
 	memcpy(kbuf + k2.key_offset,
 	       (const uint8_t *)key + key->key_offset,