cbfstool: Ensure attributes always come last in the metadata

In a rare placement edge case when adding a file with alignment
requirements, cbfstool may need to generate a CBFS header that's
slightly larger than it needs to be. The way we do this is by just
increasing the data offset field in the CBFS header until the data falls
to the desired value.

This approach works but it may confuse parsing code in the presence of
CBFS attributes. Normally, the whole area between the attribute offset
and the data offset is filled with valid attributes written back to
back, but when this header expansion occurs the attributes are followed
by some garbage data (usually 0xff). Parsers are resilient against this
but may show unexpected error messages.

This patch solves the problem by moving the attribute offset forwards
together with the data offset, so that the total area used for
attributes doesn't change. Instead, the filename field becomes the
expanded area, which is a closer match to how this worked when it was
originally implemented (before attributes existed) and is less confusing
for parsers since filenames are zero-terminated anyway.

Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: I3dd503dd5c9e6c4be437f694a7f8993a57168c2b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47824
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>

1 files changed, 19 insertions, 6 deletions

diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c
index 4249015c24..0191682de9 100644
--- a/util/cbfstool/cbfs_image.c
+++ b/util/cbfstool/cbfs_image.c
@@ -658,15 +658,28 @@ static int cbfs_add_entry_at(struct cbfs_image *image,
 	len = content_offset - addr - header_size;
 	memcpy(entry, header, header_size);
 	if (len != 0) {
-		/* the header moved backwards a bit to accommodate cbfs_file
+		/*
+		 * The header moved backwards a bit to accommodate cbfs_file
 		 * alignment requirements, so patch up ->offset to still point
-		 * to file data.
+		 * to file data. Move attributes forward so the end of the
+		 * attribute list still matches the end of the metadata.
 		 */
+		uint32_t offset = ntohl(entry->offset);
+		uint32_t attrs = ntohl(entry->attributes_offset);
 		DEBUG("|..|header|content|... <use offset to create entry>\n");
-		DEBUG("before: offset=0x%x\n", ntohl(entry->offset));
-		// TODO reset expanded name buffer to 0xFF.
-		entry->offset = htonl(ntohl(entry->offset) + len);
-		DEBUG("after: offset=0x%x\n", ntohl(entry->len));
+		DEBUG("before: attr_offset=0x%x, offset=0x%x\n", attrs, offset);
+		if (attrs == 0) {
+			memset((uint8_t *)entry + offset, 0, len);
+		} else {
+			uint8_t *p = (uint8_t *)entry + attrs;
+			memmove(p + len, p, offset - attrs);
+			memset(p, 0, len);
+			attrs += len;
+			entry->attributes_offset = htonl(attrs);
+		}
+		offset += len;
+		entry->offset = htonl(offset);
+		DEBUG("after: attr_offset=0x%x, offset=0x%x\n", attrs, offset);
 	}
 
 	// Ready to fill data into entry.