[coreboot-gerrit] Change in coreboot[master]: cbfstool: Ensure attributes always come last in the metadata

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47824 )

Change subject: cbfstool: Ensure attributes always come last in the metadata
......................................................................

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 
Change-Id: I3dd503dd5c9e6c4be437f694a7f8993a57168c2b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47824
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Aaron Durbin 
---
M util/cbfstool/cbfs_image.c
1 file changed, 19 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c
index 4249015..0191682 100644
--- a/util/cbfstool/cbfs_image.c
+++ b/util/cbfstool/cbfs_image.c
@@ -658,15 +658,28 @@
    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.