vboot2: Add verification for common vb2 struct header

All new-style structs have a common header.  This adds a verification
function for that common header, and tests for it.

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

Change-Id: I668486e77f7200c10b43aa2d17b4dd6639e5538e
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/225459
Reviewed-by: Bill Richardson <wfrichar@chromium.org>

5 files changed, 115 insertions, 11 deletions

diff --git a/firmware/2lib/2common.c b/firmware/2lib/2common.c
index 872b6233..6360ef09 100644
--- a/firmware/2lib/2common.c
+++ b/firmware/2lib/2common.c
@@ -125,7 +125,6 @@ int vb2_verify_member_inside(const void *parent, size_t parent_size,
 			     ptrdiff_t member_data_offset,
 			     size_t member_data_size)
 {
-	const size_t psize = (size_t)parent_size;
 	const uintptr_t parent_end = (uintptr_t)parent + parent_size;
 	const ptrdiff_t member_offs = vb2_offset_of(parent, member);
 	const ptrdiff_t member_end_offs = member_offs + member_size;
@@ -133,27 +132,61 @@ int vb2_verify_member_inside(const void *parent, size_t parent_size,
 	const ptrdiff_t data_end_offs = data_offs + member_data_size;
 
 	/* Make sure parent doesn't wrap */
-	if (parent_end < (uintptr_t)parent)
+	if (parent_size < 0 || parent_end < (uintptr_t)parent)
 		return VB2_ERROR_INSIDE_PARENT_WRAPS;
 
 	/*
 	 * Make sure the member is fully contained in the parent and doesn't
 	 * wrap.  Use >, not >=, since member_size = 0 is possible.
 	 */
-	if (member_end_offs < member_offs)
+	if (member_size < 0 || member_end_offs < member_offs)
 		return VB2_ERROR_INSIDE_MEMBER_WRAPS;
-	if (member_offs < 0 || member_offs > psize || member_end_offs > psize)
+	if (member_offs < 0 || member_offs > parent_size ||
+	    member_end_offs > parent_size)
 		return VB2_ERROR_INSIDE_MEMBER_OUTSIDE;
 
-	/* Make sure parent fully contains member data */
-	if (data_end_offs < data_offs)
+	/* Make sure the member data is after the member */
+	if (member_data_size > 0 && data_offs < member_end_offs)
+		return VB2_ERROR_INSIDE_DATA_OVERLAP;
+
+	/* Make sure parent fully contains member data, if any */
+	if (member_data_size < 0 || data_end_offs < data_offs)
 		return VB2_ERROR_INSIDE_DATA_WRAPS;
-	if (data_offs < 0 || data_offs > psize || data_end_offs > psize)
+	if (data_offs < 0 || data_offs > parent_size ||
+	    data_end_offs > parent_size)
 		return VB2_ERROR_INSIDE_DATA_OUTSIDE;
 
 	return VB2_SUCCESS;
 }
 
+int vb2_verify_common_header(const void *parent,
+			     uint32_t parent_size,
+			     const struct vb2_struct_common *c)
+{
+	int rv;
+
+	/* 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);
+	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)
+			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;
+}
+
 int vb2_verify_signature_inside(const void *parent,
 				uint32_t parent_size,
 				const struct vb2_signature *sig)
diff --git a/firmware/2lib/include/2common.h b/firmware/2lib/include/2common.h
index dcf799a0..8a00dd71 100644
--- a/firmware/2lib/include/2common.h
+++ b/firmware/2lib/include/2common.h
@@ -181,6 +181,20 @@ int vb2_verify_member_inside(const void *parent, size_t parent_size,
 			     size_t member_data_size);
 
 /**
+ * Verify the common struct header is fully contained in its parent data
+ *
+ * Also verifies the description is either zero-length or null-terminated.
+ *
+ * @param parent	Parent data
+ * @param parent_size	Parent size in bytes
+ * @param sig		Signature pointer
+ * @return VB2_SUCCESS, or non-zero if error.
+ */
+int vb2_verify_common_header(const void *parent,
+			     uint32_t parent_size,
+			     const struct vb2_struct_common *c);
+
+/**
  * Verify a signature is fully contained in its parent data
  *
  * @param parent	Parent data
diff --git a/firmware/2lib/include/2return_codes.h b/firmware/2lib/include/2return_codes.h
index ee6c91fe..333c29c1 100644
--- a/firmware/2lib/include/2return_codes.h
+++ b/firmware/2lib/include/2return_codes.h
@@ -167,6 +167,15 @@ enum vb2_return_code {
 	/* Unsupported hash algorithm in vb2_unpack_key() */
 	VB2_ERROR_UNPACK_KEY_HASH_ALGORITHM,
 
+	/* 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,
+
         /**********************************************************************
 	 * Keyblock verification errors (all in vb2_verify_keyblock())
 	 */
diff --git a/firmware/2lib/include/2struct.h b/firmware/2lib/include/2struct.h
index 38153c5c..12e6e0b8 100644
--- a/firmware/2lib/include/2struct.h
+++ b/firmware/2lib/include/2struct.h
@@ -277,12 +277,16 @@ struct vb2_struct_common {
 	uint16_t struct_version_major;
 	uint16_t struct_version_minor;
 
-	/* Offset of null-terminated ASCII description from start of struct */
+	/*
+	 * Offset of null-terminated ASCII description from start of struct.
+	 * Must be 0 if desc_size=0.
+	 */
 	uint32_t desc_offset;
 
 	/*
-	 * Size of description in bytes, counting null terminator.  May be 0
-	 * if no description is present.
+	 * 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).
 	 */
 	uint32_t desc_size;
 } __attribute__((packed));
diff --git a/tests/vb2_common_tests.c b/tests/vb2_common_tests.c
index c70de2ea..5a8cb202 100644
--- a/tests/vb2_common_tests.c
+++ b/tests/vb2_common_tests.c
@@ -228,18 +228,62 @@ static void test_helper_functions(void)
 		TEST_EQ(vb2_verify_member_inside(p, 20, p, 4, 4, 17),
 			VB2_ERROR_INSIDE_DATA_OUTSIDE,
 			"MemberInside data too big");
+		TEST_EQ(vb2_verify_member_inside(p, 20, p, 8, 4, 8),
+			VB2_ERROR_INSIDE_DATA_OVERLAP,
+			"MemberInside data overlaps member");
 		TEST_EQ(vb2_verify_member_inside(p, -8, p, 12, 0, 0),
 			VB2_ERROR_INSIDE_PARENT_WRAPS,
 			"MemberInside wraparound 1");
 		TEST_EQ(vb2_verify_member_inside(p, 20, p, -8, 0, 0),
 			VB2_ERROR_INSIDE_MEMBER_WRAPS,
 			"MemberInside wraparound 2");
-		TEST_EQ(vb2_verify_member_inside(p, 20, p, 4, 0, -8),
+		TEST_EQ(vb2_verify_member_inside(p, 20, p, 4, 4, -12),
 			VB2_ERROR_INSIDE_DATA_WRAPS,
 			"MemberInside wraparound 3");
 	}
 
 	{
+		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->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].desc_size = 128 - sizeof(*c);
+		TEST_SUCC(vb2_verify_common_header(cbuf, sizeof(cbuf), c + 1),
+			  "CommonInside after start");
+
+		TEST_EQ(vb2_verify_common_header(cbuf, sizeof(cbuf) - 1, c),
+			VB2_ERROR_INSIDE_DATA_OUTSIDE,
+			"CommonInside key too big");
+
+		c->desc_offset = 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);
+
+		cbuf[sizeof(cbuf) - 1] = 1;
+		TEST_EQ(vb2_verify_common_header(cbuf, sizeof(cbuf), c),
+			VB2_ERROR_DESC_TERMINATOR,
+			"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");
+	}
+
+	{
 		struct vb2_packed_key k = {.key_offset = sizeof(k),
 					   .key_size = 128};
 		TEST_SUCC(vb2_verify_packed_key_inside(&k, sizeof(k)+128, &k),