coreboot_tables: Make existing alignment conventions more explicit

There seem to be some recurring vague concerns about the alignment of
coreboot table entries. While the existing implementation has been
producing tables with a well-defined alignment (4 bytes) for a long
time, the code doesn't always make it very clear. This patch adds an
explicit constant to codify that alignment, assertions to check it after
each entry, and adds explicit padding to the few entry structures that
were relying on compiler padding to return a correct sizeof() value.

Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: Iaeef29ef255047a855066469e03b5481812e5975
Reviewed-on: https://review.coreboot.org/c/coreboot/+/70158
Reviewed-by: Arthur Heymans <arthur@aheymans.xyz>
Reviewed-by: Jakub Czapiga <jacz@semihalf.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Peter Stuge <peter@stuge.se>

3 files changed, 23 insertions, 12 deletions

diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h
index 6b5eb59cf8..76dce0a41b 100644
--- a/src/commonlib/include/commonlib/coreboot_tables.h
+++ b/src/commonlib/include/commonlib/coreboot_tables.h
@@ -95,6 +95,9 @@ enum {
 	LB_TAG_OPTION_CHECKSUM		= 0x00cc,
 };
 
+/* All table entry base addresses and sizes must be 4-byte aligned. */
+#define LB_ENTRY_ALIGN 4
+
 /* Since coreboot is usually compiled 32bit, gcc will align 64bit
  * types to 32bit boundaries. If the coreboot table is dumped on a
  * 64bit system, a uint64_t would be aligned to 64bit boundaries,
@@ -104,7 +107,7 @@ enum {
  * to ensure compatibility.
  */
 
-typedef __aligned(4) uint64_t lb_uint64_t;
+typedef __aligned(LB_ENTRY_ALIGN) uint64_t lb_uint64_t;
 
 struct lb_header {
 	uint8_t  signature[4]; /* LBIO */
@@ -203,6 +206,7 @@ struct lb_console {
 	uint32_t tag;
 	uint32_t size;
 	uint16_t type;
+	uint8_t pad[2];
 };
 
 #define LB_TAG_CONSOLE_SERIAL8250	0
@@ -288,6 +292,7 @@ struct lb_framebuffer {
 	uint8_t reserved_mask_pos;
 	uint8_t reserved_mask_size;
 	uint8_t orientation;
+	uint8_t pad[2];
 };
 
 struct lb_gpio {
@@ -553,6 +558,7 @@ struct lb_tpm_physical_presence {
 	uint32_t ppi_address;	/* Address of ACPI PPI communication buffer */
 	uint8_t tpm_version;	/* 1: TPM1.2, 2: TPM2.0 */
 	uint8_t ppi_version;	/* BCD encoded */
+	uint8_t pad[2];
 };
 
 
diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c
index 0f20735204..eaf487ff4e 100644
--- a/src/lib/coreboot_table.c
+++ b/src/lib/coreboot_table.c
@@ -76,9 +76,11 @@ struct lb_record *lb_new_record(struct lb_header *header)
 {
 	struct lb_record *rec;
 	rec = lb_last_record(header);
-	if (header->table_entries)
+	if (header->table_entries) {
+		assert(IS_ALIGNED(rec->size, LB_ENTRY_ALIGN));
 		header->table_bytes += rec->size;
-	rec = lb_last_record(header);
+		rec = lb_last_record(header);
+	}
 	header->table_entries++;
 	rec->tag = LB_TAG_UNUSED;
 	rec->size = sizeof(*rec);
@@ -301,7 +303,7 @@ static struct lb_mainboard *lb_mainboard(struct lb_header *header)
 
 	mainboard->size = ALIGN_UP(sizeof(*mainboard) +
 		strlen(mainboard_vendor) + 1 +
-		strlen(mainboard_part_number) + 1, 8);
+		strlen(mainboard_part_number) + 1, LB_ENTRY_ALIGN);
 
 	mainboard->vendor_idx = 0;
 	mainboard->part_number_idx = strlen(mainboard_vendor) + 1;
@@ -380,7 +382,7 @@ static void lb_strings(struct lb_header *header)
 		rec = (struct lb_string *)lb_new_record(header);
 		len = strlen(strings[i].string);
 		rec->tag = strings[i].tag;
-		rec->size = ALIGN_UP(sizeof(*rec) + len + 1, 8);
+		rec->size = ALIGN_UP(sizeof(*rec) + len + 1, LB_ENTRY_ALIGN);
 		memcpy(rec->string, strings[i].string, len+1);
 	}
 
@@ -422,8 +424,10 @@ static unsigned long lb_table_fini(struct lb_header *head)
 {
 	struct lb_record *rec, *first_rec;
 	rec = lb_last_record(head);
-	if (head->table_entries)
+	if (head->table_entries) {
+		assert(IS_ALIGNED(rec->size, LB_ENTRY_ALIGN));
 		head->table_bytes += rec->size;
+	}
 
 	first_rec = lb_first_record(head);
 	head->table_checksum = compute_ip_checksum(first_rec,
diff --git a/tests/lib/coreboot_table-test.c b/tests/lib/coreboot_table-test.c
index aa10f6534b..9547ee07a6 100644
--- a/tests/lib/coreboot_table-test.c
+++ b/tests/lib/coreboot_table-test.c
@@ -109,7 +109,8 @@ static void test_lb_new_record(void **state)
 	accumulated_size = sizeof(struct lb_record);
 	for (i = 0; i < entries; ++i) {
 		curr = lb_new_record(header);
-		curr->size = sizeof(struct lb_record) + ((i + 2) * 7) % 32;
+		curr->size = sizeof(struct lb_record) +
+			     ALIGN_UP(((i + 2) * 7) % 32, LB_ENTRY_ALIGN);
 
 		assert_int_equal(entries_offset + (i + 1), header->table_entries);
 		assert_int_equal(accumulated_size, header->table_bytes);
@@ -359,31 +360,31 @@ static void test_write_tables(void **state)
 			assert_int_equal(ALIGN_UP(sizeof(struct lb_mainboard)
 							  + ARRAY_SIZE(mainboard_vendor)
 							  + ARRAY_SIZE(mainboard_part_number),
-						  8),
+						  LB_ENTRY_ALIGN),
 					 record->size);
 			break;
 		case LB_TAG_VERSION:
 			assert_int_equal(ALIGN_UP(sizeof(struct lb_string)
 							  + ARRAY_SIZE(coreboot_version),
-						  8),
+						  LB_ENTRY_ALIGN),
 					 record->size);
 			break;
 		case LB_TAG_EXTRA_VERSION:
 			assert_int_equal(ALIGN_UP(sizeof(struct lb_string)
 							  + ARRAY_SIZE(coreboot_extra_version),
-						  8),
+						  LB_ENTRY_ALIGN),
 					 record->size);
 			break;
 		case LB_TAG_BUILD:
 			assert_int_equal(
 				ALIGN_UP(sizeof(struct lb_string) + ARRAY_SIZE(coreboot_build),
-					 8),
+					 LB_ENTRY_ALIGN),
 				record->size);
 			break;
 		case LB_TAG_COMPILE_TIME:
 			assert_int_equal(ALIGN_UP(sizeof(struct lb_string)
 							  + ARRAY_SIZE(coreboot_compile_time),
-						  8),
+						  LB_ENTRY_ALIGN),
 					 record->size);
 			break;
 		case LB_TAG_SERIAL: