cbfs: Move stage header into a CBFS attribute

The CBFS stage header is part of the file data (not the header) from
CBFS's point of view, which is problematic for verification: in pre-RAM
environments, there's usually not enough scratch space in CBFS_CACHE to
load the full stage into memory, so it must be directly loaded into its
final destination. However, that destination is decided from reading the
stage header. There's no way we can verify the stage header without
loading the whole file and we can't load the file without trusting the
information in the stage header.

To solve this problem, this patch changes the CBFS stage format to move
the stage header out of the file contents and into a separate CBFS
attribute. Attributes are part of the metadata, so they have already
been verified before the file is loaded.

Since CBFS stages are generally only meant to be used by coreboot itself
and the coreboot build system builds cbfstool and all stages together in
one go, maintaining backwards-compatibility should not be necessary. An
older version of coreboot will build the old version of cbfstool and a
newer version of coreboot will build the new version of cbfstool before
using it to add stages to the final image, thus cbfstool and coreboot's
stage loader should stay in sync. This only causes problems when someone
stashes away a copy of cbfstool somewhere and later uses it to try to
extract stages from a coreboot image built from a different revision...
a debugging use-case that is hopefully rare enough that affected users
can manually deal with finding a matching version of cbfstool.

The SELF (payload) format, on the other hand, is designed to be used for
binaries outside of coreboot that may use independent build systems and
are more likely to be added with a potentially stale copy of cbfstool,
so it would be more problematic to make a similar change for SELFs. It
is not necessary for verification either, since they're usually only
used in post-RAM environments and selfload() already maps SELFs to
CBFS_CACHE before loading them to their final destination anyway (so
they can be hashed at that time).

Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: I8471ad7494b07599e24e82b81e507fcafbad808a
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46484
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>

src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h

@@ -20,7 +20,8 @@ enum cbfs_type {
 	CBFS_TYPE_NULL		= 0xffffffff,
 	CBFS_TYPE_BOOTBLOCK	= 0x01,
 	CBFS_TYPE_CBFSHEADER	= 0x02,
-	CBFS_TYPE_STAGE		= 0x10,
+	CBFS_TYPE_LEGACY_STAGE	= 0x10,
+	CBFS_TYPE_STAGE		= 0x11,
 	CBFS_TYPE_SELF		= 0x20,
 	CBFS_TYPE_FIT		= 0x21,
 	CBFS_TYPE_OPTIONROM	= 0x30,
@@ -131,6 +132,7 @@ enum cbfs_file_attr_tag {
 	CBFS_FILE_ATTR_TAG_ALIGNMENT	= 0x42434c41, /* BE: 'BCLA' */
 	CBFS_FILE_ATTR_TAG_IBB		= 0x32494242, /* BE: '2IBB' */
 	CBFS_FILE_ATTR_TAG_PADDING	= 0x47444150, /* BE: 'GNDP' */
+	CBFS_FILE_ATTR_TAG_STAGEHEADER	= 0x53746748, /* BE: 'StgH' */
 };
 
 struct cbfs_file_attr_compression {
@@ -160,22 +162,20 @@ struct cbfs_file_attr_align {
 	uint32_t alignment;
 } __packed;
 
+struct cbfs_file_attr_stageheader {
+	uint32_t tag;
+	uint32_t len;
+	uint64_t loadaddr;	/* Memory address to load the code to. */
+	uint32_t entry_offset;	/* Offset of entry point from loadaddr. */
+	uint32_t memlen;	/* Total length (including BSS) in memory. */
+} __packed;
+
+
 /*** Component sub-headers ***/
 
 /* Following are component sub-headers for the "standard"
    component types */
 
-/** This is the sub-header for stage components.  Stages are
-    loaded by coreboot during the normal boot process */
-
-struct cbfs_stage {
-	uint32_t compression;  /** Compression type */
-	uint64_t entry;  /** entry point */
-	uint64_t load;   /** Where to load in memory */
-	uint32_t len;          /** length of data to load */
-	uint32_t memlen;	   /** total length of object in memory */
-} __packed;
-
 /** this is the sub-header for payload components.  Payloads
     are loaded by coreboot at the end of the boot process */
 

src/lib/cbfs.c

@@ -386,11 +386,6 @@ cb_err_t cbfs_prog_stage_load(struct prog *pstage)
 {
 	union cbfs_mdata mdata;
 	struct region_device rdev;
-	struct cbfs_stage stage;
-	uint8_t *load;
-	void *entry;
-	size_t fsize;
-	size_t foffset;
 	cb_err_t err;
 
 	prog_locate_hook(pstage);
@@ -401,50 +396,41 @@ cb_err_t cbfs_prog_stage_load(struct prog *pstage)
 	assert(be32toh(mdata.h.type) == CBFS_TYPE_STAGE);
 	pstage->cbfs_type = CBFS_TYPE_STAGE;
 
-	if (rdev_readat(&rdev, &stage, 0, sizeof(stage)) != sizeof(stage))
-		return CB_CBFS_IO;
-
-	fsize = region_device_sz(&rdev);
-	fsize -= sizeof(stage);
-	foffset = 0;
-	foffset += sizeof(stage);
-
-	/* cbfs_stage fields are written in little endian despite the other
-	   cbfs data types being encoded in big endian. */
-	stage.compression = read_le32(&stage.compression);
-	stage.entry = read_le64(&stage.entry);
-	stage.load = read_le64(&stage.load);
-	stage.len = read_le32(&stage.len);
-	stage.memlen = read_le32(&stage.memlen);
-
-	assert(fsize == stage.len);
+	enum cbfs_compression compression = CBFS_COMPRESS_NONE;
+	const struct cbfs_file_attr_compression *cattr = cbfs_find_attr(&mdata,
+				CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*cattr));
+	if (cattr)
+		compression = be32toh(cattr->compression);
 
-	load = (void *)(uintptr_t)stage.load;
-	entry = (void *)(uintptr_t)stage.entry;
+	const struct cbfs_file_attr_stageheader *sattr = cbfs_find_attr(&mdata,
+				CBFS_FILE_ATTR_TAG_STAGEHEADER, sizeof(*sattr));
+	if (!sattr)
+		return CB_ERR;
+	prog_set_area(pstage, (void *)(uintptr_t)be64toh(sattr->loadaddr),
+		      be32toh(sattr->memlen));
+	prog_set_entry(pstage, prog_start(pstage) +
+			       be32toh(sattr->entry_offset), NULL);
 
 	/* Hacky way to not load programs over read only media. The stages
 	 * that would hit this path initialize themselves. */
 	if ((ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) &&
 	    !CONFIG(NO_XIP_EARLY_STAGES) && CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) {
-		void *mapping = rdev_mmap(&rdev, foffset, fsize);
+		void *mapping = rdev_mmap_full(&rdev);
 		rdev_munmap(&rdev, mapping);
-		if (mapping == load)
-			goto out;
+		if (mapping == prog_start(pstage))
+			return CB_SUCCESS;
 	}
 
-	fsize = cbfs_stage_load_and_decompress(&rdev, foffset, fsize, load,
-					       stage.memlen, stage.compression);
+	size_t fsize = cbfs_stage_load_and_decompress(&rdev, 0, region_device_sz(&rdev),
+				prog_start(pstage), prog_size(pstage), compression);
 	if (!fsize)
 		return CB_ERR;
 
 	/* Clear area not covered by file. */
-	memset(&load[fsize], 0, stage.memlen - fsize);
-
-	prog_segment_loaded((uintptr_t)load, stage.memlen, SEG_FINAL);
+	memset(prog_start(pstage) + fsize, 0, prog_size(pstage) - fsize);
 
-out:
-	prog_set_area(pstage, load, stage.memlen);
-	prog_set_entry(pstage, entry, NULL);
+	prog_segment_loaded((uintptr_t)prog_start(pstage), prog_size(pstage),
+			    SEG_FINAL);
 
 	return CB_SUCCESS;
 }

src/lib/rmodule.c

@@ -2,7 +2,6 @@
 #include <assert.h>
 #include <cbmem.h>
 #include <cbfs.h>
-#include <cbfs_private.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
@@ -13,6 +12,8 @@
 /* Change this define to get more verbose debugging for module loading. */
 #define PK_ADJ_LEVEL BIOS_NEVER
 
+const size_t region_alignment = MIN_UNSAFE(DYN_CBMEM_ALIGN_SIZE, 4096);
+
 static inline int rmodule_is_loaded(const struct rmodule *module)
 {
 	return module->location != NULL;
@@ -190,20 +191,26 @@ int rmodule_load(void *base, struct rmodule *module)
 	return 0;
 }
 
-int rmodule_calc_region(unsigned int region_alignment, size_t rmodule_size,
-			size_t *region_size, int *load_offset)
+static void *rmodule_cbfs_allocator(void *rsl_arg, size_t unused,
+				    union cbfs_mdata *mdata)
 {
-	/* region_alignment must be a power of 2. */
-	if (region_alignment & (region_alignment - 1))
-		BUG();
-
-	if (region_alignment < 4096)
-		region_alignment = 4096;
+	struct rmod_stage_load *rsl = rsl_arg;
+
+	assert(IS_POWER_OF_2(region_alignment) &&
+	       region_alignment >= sizeof(struct rmodule_header));
+
+	/* The CBFS core just passes us the decompressed size of the file, but
+	   we need to know the memlen of the binary image. We need to find and
+	   parse the stage header explicitly. */
+	const struct cbfs_file_attr_stageheader *sattr = cbfs_find_attr(mdata,
+			CBFS_FILE_ATTR_TAG_STAGEHEADER, sizeof(*sattr));
+	if (!sattr) {
+		printk(BIOS_ERR, "rmodule '%s' has no stage header!\n",
+		       rsl->prog->name);
+		return NULL;
+	}
 
-	/* Sanity check rmodule_header size. The code below assumes it is less
-	 * than the minimum alignment required. */
-	if (region_alignment < sizeof(struct rmodule_header))
-		BUG();
+	const size_t memlen = be32toh(sattr->memlen);
 
 	/* Place the rmodule according to alignment. The rmodule files
 	 * themselves are packed as a header and a payload, however the rmodule
@@ -215,7 +222,7 @@ int rmodule_calc_region(unsigned int region_alignment, size_t rmodule_size,
 	 * to place the rmodule so that the program falls on the aligned
 	 * address with the header just before it. Therefore, we need at least
 	 * a page to account for the size of the header. */
-	*region_size = ALIGN(rmodule_size + region_alignment, 4096);
+	size_t region_size = ALIGN(memlen + region_alignment, 4096);
 	/* The program starts immediately after the header. However,
 	 * it needs to be aligned to a 4KiB boundary. Therefore, adjust the
 	 * program location so that the program lands on a page boundary.  The
@@ -231,60 +238,33 @@ int rmodule_calc_region(unsigned int region_alignment, size_t rmodule_size,
 	 * |  >= 0 bytes from alignment     |
 	 * +--------------------------------+  region_alignment
 	 */
-	*load_offset = region_alignment;
 
-	return region_alignment - sizeof(struct rmodule_header);
+	uint8_t *stage_region = cbmem_add(rsl->cbmem_id, region_size);
+	if (stage_region == NULL)
+		return NULL;
+
+	return stage_region + region_alignment - sizeof(struct rmodule_header);
 }
 
 int rmodule_stage_load(struct rmod_stage_load *rsl)
 {
 	struct rmodule rmod_stage;
-	size_t region_size;
-	char *stage_region;
-	int rmodule_offset;
-	int load_offset;
-	struct cbfs_stage stage;
-	void *rmod_loc;
-	struct region_device rdev;
-	union cbfs_mdata mdata;
 
 	if (rsl->prog == NULL || prog_name(rsl->prog) == NULL)
 		return -1;
 
 	if (prog_locate_hook(rsl->prog))
 		return -1;
 
-	if (cbfs_boot_lookup(prog_name(rsl->prog), false, &mdata, &rdev) != CB_SUCCESS)
-		return -1;
-
-	assert(be32toh(mdata.h.type) == CBFS_TYPE_STAGE);
-	rsl->prog->cbfs_type = CBFS_TYPE_STAGE;
-
-	if (rdev_readat(&rdev, &stage, 0, sizeof(stage)) != sizeof(stage))
-		return -1;
-
-	rmodule_offset =
-		rmodule_calc_region(DYN_CBMEM_ALIGN_SIZE,
-				    stage.memlen, &region_size, &load_offset);
-
-	stage_region = cbmem_add(rsl->cbmem_id, region_size);
-
-	if (stage_region == NULL)
-		return -1;
-
-	rmod_loc = &stage_region[rmodule_offset];
-
-	printk(BIOS_INFO, "Decompressing stage %s @ %p (%d bytes)\n",
-	       prog_name(rsl->prog), rmod_loc, stage.memlen);
-
-	if (!cbfs_load_and_decompress(&rdev, sizeof(stage), stage.len, rmod_loc,
-				      stage.memlen, stage.compression))
+	void *rmod_loc = cbfs_alloc(prog_name(rsl->prog),
+				    rmodule_cbfs_allocator, rsl, NULL);
+	if (!rmod_loc)
 		return -1;
 
 	if (rmodule_parse(rmod_loc, &rmod_stage))
 		return -1;
 
-	if (rmodule_load(&stage_region[load_offset], &rmod_stage))
+	if (rmodule_load(rmod_loc + sizeof(struct rmodule_header), &rmod_stage))
 		return -1;
 
 	prog_set_area(rsl->prog, rmod_stage.location,

src/soc/amd/common/block/pi/refcode_loader.c

@@ -29,14 +29,8 @@ static int agesa_locate_raw_file(const char *name, struct region_device *rdev)
 static int agesa_locate_stage_file_early(const char *name,
 					struct region_device *rdev)
 {
-	const size_t metadata_sz = sizeof(struct cbfs_stage);
-
 	if (agesa_locate_file(name, rdev, CBFS_TYPE_STAGE))
 		return -1;
-
-	/* Peel off the cbfs stage metadata. */
-	return rdev_chain(rdev, rdev, metadata_sz,
-			region_device_sz(rdev) - metadata_sz);
 }
 
 static int agesa_locate_stage_file_ramstage(const char *name,