Change in coreboot[master]: cbfs: Move stage header into a CBFS attribute - coreboot-gerrit

16 Oct 2020

Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46484
to review the following change.
Change subject: cbfs: Move stage header into a CBFS attribute
......................................................................
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
---
M src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h
M src/include/program_loading.h
M src/lib/cbfs.c
M src/lib/prog_loaders.c
M src/lib/rmodule.c
M src/security/vboot/vboot_loader.c
M src/soc/amd/common/block/pi/refcode_loader.c
M util/cbfstool/cbfs-mkstage.c
M util/cbfstool/cbfs_image.c
M util/cbfstool/cbfstool.c
M util/cbfstool/common.h
11 files changed, 210 insertions(+), 314 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/46484/1

diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h
index ac4b38f..c31d995 100644
--- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h
+++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h
@@ -17,7 +17,8 @@
    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,
@@ -125,6 +126,7 @@
    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 {
@@ -154,22 +156,20 @@
    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 */
diff --git a/src/include/program_loading.h b/src/include/program_loading.h
index e26792e..57efd7b 100644
--- a/src/include/program_loading.h
+++ b/src/include/program_loading.h
@@ -4,6 +4,7 @@
#include <assert.h>
 #include <bootmem.h>
+#include <commonlib/bsd/cbfs_serialized.h>
 #include <commonlib/region.h>
 #include <types.h>
@@ -41,7 +42,8 @@
 /* Representation of a program. */
 struct prog {
    enum prog_type type;
-	uint32_t cbfs_type;
+	enum cbfs_type cbfs_type;
+	enum cbfs_compression compression; /* Only used for CBFS_TYPE_STAGE. */
    const char *name;
    struct region_device load_rdev;	/* Source for loading (e.g. flash). */
    void *start;		/* Program start in memory after loading. */
@@ -66,11 +68,16 @@
    return prog->type;
 }
-static inline uint32_t prog_cbfs_type(const struct prog *prog)
+static inline enum cbfs_type prog_cbfs_type(const struct prog *prog)
 {
    return prog->cbfs_type;
 }
+static inline enum cbfs_compression prog_compression(const struct prog *prog)
+{
+	return prog->compression;
+}
+
 static inline struct region_device *prog_rdev(struct prog *prog)
 {
    return &prog->load_rdev;
@@ -118,6 +125,8 @@
static inline void prog_set_entry(struct prog *prog, void *e, void *arg)
 {
+	if (prog->cbfs_type == CBFS_TYPE_STAGE)
+		assert(e >= prog->start && e - prog->start < prog->size);
    prog->entry = e;
    prog->arg = arg;
 }
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index a6f0e69..452dce4 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -338,57 +338,30 @@
int cbfs_prog_stage_load(struct prog *pstage)
 {
-	struct cbfs_stage stage;
-	uint8_t *load;
-	void *entry;
+	struct region_device *rdev = prog_rdev(pstage);
    size_t fsize;
-	size_t foffset;
-	const struct region_device *fh = prog_rdev(pstage);
-
-	if (rdev_readat(fh, &stage, 0, sizeof(stage)) != sizeof(stage))
-		return -1;
-
-	fsize = region_device_sz(fh);
-	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);
-
-	load = (void *)(uintptr_t)stage.load;
-	entry = (void *)(uintptr_t)stage.entry;
/* 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(fh, foffset, fsize);
-		rdev_munmap(fh, mapping);
-		if (mapping == load)
-			goto out;
+		void *mapping = rdev_mmap_full(rdev);
+		rdev_munmap(rdev, mapping);
+		if (mapping == prog_start(pstage))
+			return 0;
    }
-	fsize = cbfs_stage_load_and_decompress(fh, foffset, fsize, load,
-					 stage.memlen, stage.compression);
+	fsize = cbfs_stage_load_and_decompress(rdev, 0, region_device_sz(rdev),
+					prog_start(pstage), prog_size(pstage),
+					prog_compression(pstage));
    if (!fsize)
    	return -1;
/* Clear area not covered by file. */
-	memset(&load[fsize], 0, stage.memlen - fsize);
+	memset(prog_start(pstage) + fsize, 0, prog_size(pstage) - fsize);
-	prog_segment_loaded((uintptr_t)load, stage.memlen, SEG_FINAL);
-
-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 0;
 }
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index 4fae5d2..f75f2ee 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -36,6 +36,26 @@
    	return err;
prog->cbfs_type = be32toh(mdata.h.type);
+
+	const struct cbfs_file_attr_compression *compr = cbfs_find_attr(&mdata,
+				CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*compr));
+	if (compr)
+		prog->compression = be32toh(compr->compression);
+	else
+		prog->compression = CBFS_COMPRESS_NONE;
+
+	if (prog->cbfs_type == CBFS_TYPE_STAGE) {
+		const struct cbfs_file_attr_stageheader *stgh =
+			cbfs_find_attr(&mdata, CBFS_FILE_ATTR_TAG_STAGEHEADER,
+				       sizeof(*stgh));
+		if (!stgh)
+			return CB_ERR;
+		prog_set_area(prog, (void *)(uintptr_t)be64toh(stgh->loadaddr),
+			      be32toh(stgh->memlen));
+		prog_set_entry(prog, prog_start(prog) +
+				     be32toh(stgh->entry_offset), NULL);
+	}
+
    return CB_SUCCESS;
 }
diff --git a/src/lib/rmodule.c b/src/lib/rmodule.c
index 7944535..c68fe20 100644
--- a/src/lib/rmodule.c
+++ b/src/lib/rmodule.c
@@ -242,21 +242,15 @@
    char *stage_region;
    int rmodule_offset;
    int load_offset;
-	struct cbfs_stage stage;
    void *rmod_loc;
-	struct region_device *fh;
-	if (rsl->prog == NULL || prog_name(rsl->prog) == NULL)
-		return -1;
-
-	fh = prog_rdev(rsl->prog);
-
-	if (rdev_readat(fh, &stage, 0, sizeof(stage)) != sizeof(stage))
+	struct prog *prog = rsl->prog;
+	if (prog == NULL || prog_name(prog) == NULL)
    	return -1;
rmodule_offset =
-		rmodule_calc_region(DYN_CBMEM_ALIGN_SIZE,
-				    stage.memlen, &region_size, &load_offset);
+		rmodule_calc_region(DYN_CBMEM_ALIGN_SIZE, prog_size(prog),
+				    &region_size, &load_offset);
stage_region = cbmem_add(rsl->cbmem_id, region_size);
@@ -266,10 +260,11 @@
    rmod_loc = &stage_region[rmodule_offset];
printk(BIOS_INFO, "Decompressing stage %s @ %p (%d bytes)\n",
-	       prog_name(rsl->prog), rmod_loc, stage.memlen);
+	       prog_name(rsl->prog), rmod_loc, prog_size(prog));
-	if (!cbfs_load_and_decompress(fh, sizeof(stage), stage.len, rmod_loc,
-				      stage.memlen, stage.compression))
+	if (!cbfs_load_and_decompress(prog_rdev(prog), 0,
+				region_device_sz(prog_rdev(prog)), rmod_loc,
+				prog_size(prog), prog_compression(prog)))
    	return -1;
if (rmodule_parse(rmod_loc, &rmod_stage))
@@ -278,13 +273,13 @@
    if (rmodule_load(&stage_region[load_offset], &rmod_stage))
    	return -1;
-	prog_set_area(rsl->prog, rmod_stage.location,
+	prog_set_area(prog, rmod_stage.location,
    		rmodule_memory_size(&rmod_stage));
/* Allow caller to pick up parameters, if available. */
    rsl->params = rmodule_parameters(&rmod_stage);
-	prog_set_entry(rsl->prog, rmodule_entry(&rmod_stage), rsl->params);
+	prog_set_entry(prog, rmodule_entry(&rmod_stage), rsl->params);
return 0;
 }
diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c
index 56a0664..d8b42f8 100644
--- a/src/security/vboot/vboot_loader.c
+++ b/src/security/vboot/vboot_loader.c
@@ -45,7 +45,6 @@
    	verstage_main();
    	after_verstage();
    } else if (verstage_should_load()) {
-		struct cbfsf file;
    	struct prog verstage =
    		PROG_INIT(PROG_VERSTAGE,
    			CONFIG_CBFS_PREFIX "/verstage");
@@ -53,10 +52,8 @@
    	printk(BIOS_DEBUG, "VBOOT: Loading verstage.\n");
/* load verstage from RO */
-		if (cbfs_boot_locate(&file, prog_name(&verstage), NULL))
-			die("failed to load verstage");
-
-		cbfs_file_data(prog_rdev(&verstage), &file);
+		if (prog_locate(&verstage))
+			die("failed to locate verstage");
if (cbfs_prog_stage_load(&verstage))
    		die("failed to load verstage");
diff --git a/src/soc/amd/common/block/pi/refcode_loader.c b/src/soc/amd/common/block/pi/refcode_loader.c
index bedd847..9be2079 100644
--- a/src/soc/amd/common/block/pi/refcode_loader.c
+++ b/src/soc/amd/common/block/pi/refcode_loader.c
@@ -29,14 +29,8 @@
 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,
diff --git a/util/cbfstool/cbfs-mkstage.c b/util/cbfstool/cbfs-mkstage.c
index bfe7bf8..935bca0 100644
--- a/util/cbfstool/cbfs-mkstage.c
+++ b/util/cbfstool/cbfs-mkstage.c
@@ -10,8 +10,6 @@
 #include "cbfs.h"
 #include "rmodule.h"
-#include <commonlib/bsd/compression.h>
-
 /* Checks if program segment contains the ignored section */
 static int is_phdr_ignored(Elf64_Phdr *phdr, Elf64_Shdr *shdr)
 {
@@ -73,19 +71,20 @@
    return NULL;
 }
-static void fill_cbfs_stage(struct buffer *outheader, enum cbfs_compression algo,
-				uint64_t entry, uint64_t loadaddr,
-				uint32_t filesize, uint32_t memsize)
+static int fill_cbfs_stageheader(struct cbfs_file_attr_stageheader *stageheader,
+				 uint64_t entry, uint64_t loadaddr,
+				 uint32_t memsize)
 {
-	/* N.B. The original plan was that SELF data was B.E.
-	 * but: this is all L.E.
-	 * Maybe we should just change the spec.
-	 */
-	xdr_le.put32(outheader, algo);
-	xdr_le.put64(outheader, entry);
-	xdr_le.put64(outheader, loadaddr);
-	xdr_le.put32(outheader, filesize);
-	xdr_le.put32(outheader, memsize);
+	if (entry - loadaddr >= memsize) {
+		ERROR("stage entry point out of bounds!\n");
+		return -1;
+	}
+
+	stageheader->loadaddr = htonll(loadaddr);
+	stageheader->memlen = htonl(memsize);
+	stageheader->entry_offset = htonl(entry - loadaddr);
+
+	return  0;
 }
/* returns size of result, or -1 if error.
@@ -93,26 +92,20 @@
  * works for all elf files, not just the restricted set.
  */
 int parse_elf_to_stage(const struct buffer *input, struct buffer *output,
-		       enum cbfs_compression algo, uint32_t *location,
-		       const char *ignore_section)
+		       uint32_t *location, const char *ignore_section,
+		       struct cbfs_file_attr_stageheader *stageheader)
 {
    struct parsed_elf pelf;
    Elf64_Phdr *phdr;
    Elf64_Ehdr *ehdr;
    Elf64_Shdr *shdr_ignored;
    Elf64_Addr virt_to_phys;
-	char *buffer;
-	struct buffer outheader;
    int ret = -1;
int headers;
-	int i, outlen;
+	int i;
    uint64_t data_start, data_end, mem_end;
-	comp_func_ptr compress = compression_function(algo);
-	if (!compress)
-		return -1;
-
    DEBUG("start: parse_elf_to_stage(location=0x%x)\n", *location);
int flags = ELF_PARSE_PHDR | ELF_PARSE_SHDR | ELF_PARSE_STRTAB;
@@ -185,15 +178,13 @@
    	exit(1);
    }
-	/* allocate an intermediate buffer for the data */
-	buffer = calloc(data_end - data_start, 1);
-
-	if (buffer == NULL) {
+	if (buffer_create(output, data_end - data_start, input->name) != 0) {
    	ERROR("Unable to allocate memory: %m\n");
    	goto err;
    }
+	memset(output->data, 0, output->size);
-	/* Copy the file data into the buffer */
+	/* Copy the file data into the output buffer */
for (i = 0; i < headers; i++) {
    	uint64_t l_start, l_offset = 0;
@@ -222,92 +213,17 @@
    		ERROR("Underflow copying out the segment."
    		      "File has %zu bytes left, segment end is %zu\n",
    		      input->size, (size_t)(phdr[i].p_offset + phdr[i].p_filesz));
-			free(buffer);
    		goto err;
    	}
-		memcpy(buffer + (l_start - data_start),
+		memcpy(&output->data[l_start - data_start],
    	       &input->data[phdr[i].p_offset + l_offset],
    	       phdr[i].p_filesz - l_offset);
    }
-	/* Now make the output buffer */
-	if (buffer_create(output, sizeof(struct cbfs_stage) + data_end - data_start,
-			  input->name) != 0) {
-		ERROR("Unable to allocate memory: %m\n");
-		free(buffer);
-		goto err;
-	}
-	memset(output->data, 0, output->size);
-
-	/* Compress the data, at which point we'll know information
-	 * to fill out the header. This seems backward but it works because
-	 * - the output header is a known size (not always true in many xdr's)
-	 * - we do need to know the compressed output size first
-	 * If compression fails or makes the data bigger, we'll warn about it
-	 * and use the original data.
-	 */
-	if (compress(buffer, data_end - data_start,
-		     (output->data + sizeof(struct cbfs_stage)),
-		     &outlen) < 0 || (unsigned)outlen > data_end - data_start) {
-		WARN("Compression failed or would make the data bigger "
-		     "- disabled.\n");
-		memcpy(output->data + sizeof(struct cbfs_stage),
-		       buffer, data_end - data_start);
-		outlen = data_end - data_start;
-		algo = CBFS_COMPRESS_NONE;
-	}
-
-	/* Check for enough BSS scratch space to decompress LZ4 in-place. */
-	if (algo == CBFS_COMPRESS_LZ4) {
-		size_t result;
-		size_t memlen = mem_end - data_start;
-		size_t compressed_size = outlen;
-		char *compare_buffer = malloc(memlen);
-		char *start = compare_buffer + memlen - compressed_size;
-
-		if (compare_buffer == NULL) {
-			ERROR("Can't allocate memory!\n");
-			free(buffer);
-			goto err;
-		}
-
-		memcpy(start, output->data + sizeof(struct cbfs_stage),
-		       compressed_size);
-		result = ulz4fn(start, compressed_size, compare_buffer, memlen);
-
-		if (result == 0) {
-			ERROR("Not enough scratch space to decompress LZ4 in-place -- increase BSS size or disable compression!\n");
-			free(compare_buffer);
-			free(buffer);
-			goto err;
-		}
-		if (result != data_end - data_start ||
-		    memcmp(compare_buffer, buffer, data_end - data_start)) {
-			ERROR("LZ4 compression BUG! Report to mailing list.\n");
-			free(compare_buffer);
-			free(buffer);
-			goto err;
-		}
-		free(compare_buffer);
-	}
-
-	free(buffer);
-
-	/* Set up for output marshaling. */
-	outheader.data = output->data;
-	outheader.size = 0;
-
    /* coreboot expects entry point to be physical address. Thus, adjust the
-	 * entry point accordingly.
-	 */
-	fill_cbfs_stage(&outheader, algo, ehdr->e_entry + virt_to_phys,
-			data_start, outlen, mem_end - data_start);
-
-	if (*location)
-		*location -= sizeof(struct cbfs_stage);
-	output->size = sizeof(struct cbfs_stage) + outlen;
-	ret = 0;
-
+	   entry point accordingly. */
+	ret = fill_cbfs_stageheader(stageheader, ehdr->e_entry + virt_to_phys,
+				    data_start, mem_end - data_start);
 err:
    parsed_elf_destroy(&pelf);
    return ret;
@@ -358,13 +274,13 @@
 }
int parse_elf_to_xip_stage(const struct buffer *input, struct buffer *output,
-				uint32_t *location, const char *ignore_section)
+			   uint32_t *location, const char *ignore_section,
+			   struct cbfs_file_attr_stageheader *stageheader)
 {
    struct xip_context xipctx;
    struct rmod_context *rmodctx;
    struct reloc_filter filter;
    struct parsed_elf *pelf;
-	size_t output_sz;
    uint32_t adjustment;
    struct buffer binput;
    struct buffer boutput;
@@ -398,13 +314,12 @@
    if (rmodule_collect_relocations(rmodctx, &filter))
    	goto out;
-	output_sz = sizeof(struct cbfs_stage) + pelf->phdr->p_filesz;
-	if (buffer_create(output, output_sz, input->name) != 0) {
+	if (buffer_create(output, pelf->phdr->p_filesz, input->name) != 0) {
    	ERROR("Unable to allocate memory: %m\n");
    	goto out;
    }
    buffer_clone(&boutput, output);
-	memset(buffer_get(&boutput), 0, output_sz);
+	memset(buffer_get(&boutput), 0, pelf->phdr->p_filesz);
    buffer_set_size(&boutput, 0);
/* Single loadable segment. The entire segment moves to final
@@ -412,17 +327,16 @@
    adjustment = *location - pelf->phdr->p_vaddr;
    DEBUG("Relocation adjustment: %08x\n", adjustment);
-	fill_cbfs_stage(&boutput, CBFS_COMPRESS_NONE,
-			(uint32_t)pelf->ehdr.e_entry + adjustment,
-			(uint32_t)pelf->phdr->p_vaddr + adjustment,
-			pelf->phdr->p_filesz, pelf->phdr->p_memsz);
+	fill_cbfs_stageheader(stageheader,
+			      (uint32_t)pelf->ehdr.e_entry + adjustment,
+			      (uint32_t)pelf->phdr->p_vaddr + adjustment,
+			      pelf->phdr->p_memsz);
    /* Need an adjustable buffer. */
    buffer_clone(&binput, input);
    buffer_seek(&binput, pelf->phdr->p_offset);
    bputs(&boutput, buffer_get(&binput), pelf->phdr->p_filesz);
buffer_clone(&boutput, output);
-	buffer_seek(&boutput, sizeof(struct cbfs_stage));
/* Make adjustments to all the relocations within the program. */
    for (i = 0; i < rmodctx->nrelocs; i++) {
@@ -448,8 +362,6 @@
    	xdr_le.put32(&out, val + adjustment);
    }
-	/* Need to back up the location to include cbfs stage metadata. */
-	*location -= sizeof(struct cbfs_stage);
    ret = 0;
out:
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c
index 05fdc8a..a51f941 100644
--- a/util/cbfstool/cbfs_image.c
+++ b/util/cbfstool/cbfs_image.c
@@ -823,69 +823,6 @@
    return NULL;
 }
-static int cbfs_stage_decompress(struct cbfs_stage *stage, struct buffer *buff)
-{
-	struct buffer reader;
-	char *orig_buffer;
-	char *new_buffer;
-	size_t new_buff_sz;
-	decomp_func_ptr decompress;
-
-	buffer_clone(&reader, buff);
-
-	/* The stage metadata is in little endian. */
-	stage->compression = xdr_le.get32(&reader);
-	stage->entry = xdr_le.get64(&reader);
-	stage->load = xdr_le.get64(&reader);
-	stage->len = xdr_le.get32(&reader);
-	stage->memlen = xdr_le.get32(&reader);
-
-	/* Create a buffer just with the uncompressed program now that the
-	 * struct cbfs_stage has been peeled off. */
-	if (stage->compression == CBFS_COMPRESS_NONE) {
-		new_buff_sz = buffer_size(buff) - sizeof(struct cbfs_stage);
-
-		orig_buffer = buffer_get(buff);
-		new_buffer = calloc(1, new_buff_sz);
-		memcpy(new_buffer, orig_buffer + sizeof(struct cbfs_stage),
-			new_buff_sz);
-		buffer_init(buff, buff->name, new_buffer, new_buff_sz);
-		free(orig_buffer);
-		return 0;
-	}
-
-	decompress = decompression_function(stage->compression);
-	if (decompress == NULL)
-		return -1;
-
-	orig_buffer = buffer_get(buff);
-
-	/* This can be too big of a buffer needed, but there's no current
-	 * field indicating decompressed size of data. */
-	new_buff_sz = stage->memlen;
-	new_buffer = calloc(1, new_buff_sz);
-
-	if (decompress(orig_buffer + sizeof(struct cbfs_stage),
-			(int)(buffer_size(buff) - sizeof(struct cbfs_stage)),
-			new_buffer, (int)new_buff_sz, &new_buff_sz)) {
-		ERROR("Couldn't decompress stage.\n");
-		free(new_buffer);
-		return -1;
-	}
-
-	/* Include correct size for full stage info. */
-	buffer_init(buff, buff->name, new_buffer, new_buff_sz);
-
-	/* True decompressed size is just the data size -- no metadata. */
-	stage->len = new_buff_sz;
-	/* Stage is not compressed. */
-	stage->compression = CBFS_COMPRESS_NONE;
-
-	free(orig_buffer);
-
-	return 0;
-}
-
 static int cbfs_payload_decompress(struct cbfs_payload_segment *segments,
    	struct buffer *buff, int num_seg)
 {
@@ -1019,11 +956,11 @@
    return 0;
 }
-static int cbfs_stage_make_elf(struct buffer *buff, uint32_t arch)
+static int cbfs_stage_make_elf(struct buffer *buff, uint32_t arch,
+			       struct cbfs_file *entry)
 {
    Elf64_Ehdr ehdr;
    Elf64_Shdr shdr;
-	struct cbfs_stage stage;
    struct elf_writer *ew;
    struct buffer elf_out;
    size_t empty_sz;
@@ -1034,16 +971,23 @@
    	return -1;
    }
-	if (cbfs_stage_decompress(&stage, buff)) {
-		ERROR("Failed to decompress stage.\n");
+	struct cbfs_file_attr_stageheader *stage = NULL;
+	for (struct cbfs_file_attribute *attr = cbfs_file_first_attr(entry);
+	     attr != NULL; attr = cbfs_file_next_attr(entry, attr)) {
+		if (ntohl(attr->tag) == CBFS_FILE_ATTR_TAG_STAGEHEADER) {
+			stage = (struct cbfs_file_attr_stageheader *)attr;
+			break;
+		}
+	}
+
+	if (stage == NULL) {
+		ERROR("Stage header not found for %s\n", entry->filename);
    	return -1;
    }
if (init_elf_from_arch(&ehdr, arch))
    	return -1;
-	ehdr.e_entry = stage.entry;
-
    /* Attempt rmodule translation first. */
    rmod_ret = rmodule_stage_to_elf(&ehdr, buff);
@@ -1055,6 +999,8 @@
/* Rmodule couldn't do anything with the data. Continue on with SELF. */
+	ehdr.e_entry = ntohll(stage->loadaddr) + ntohl(stage->entry_offset);
+
    ew = elf_writer_init(&ehdr);
    if (ew == NULL) {
    	ERROR("Unable to init ELF writer.\n");
@@ -1064,9 +1010,9 @@
    memset(&shdr, 0, sizeof(shdr));
    shdr.sh_type = SHT_PROGBITS;
    shdr.sh_flags = SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR;
-	shdr.sh_addr = stage.load;
-	shdr.sh_size = stage.len;
-	empty_sz = stage.memlen - stage.len;
+	shdr.sh_addr = ntohll(stage->loadaddr);
+	shdr.sh_size = buffer_size(buff);
+	empty_sz = ntohl(stage->memlen) - buffer_size(buff);
if (elf_writer_add_section(ew, &shdr, buff, ".program")) {
    	ERROR("Unable to add ELF section: .program\n");
@@ -1081,7 +1027,7 @@
    	memset(&shdr, 0, sizeof(shdr));
    	shdr.sh_type = SHT_NOBITS;
    	shdr.sh_flags = SHF_WRITE | SHF_ALLOC;
-		shdr.sh_addr = stage.load + stage.len;
+		shdr.sh_addr = ntohl(stage->loadaddr) + buffer_size(buff);
    	shdr.sh_size = empty_sz;
    	if (elf_writer_add_section(ew, &shdr, &b, ".empty")) {
    		ERROR("Unable to add ELF section: .empty\n");
@@ -1105,7 +1051,8 @@
    return 0;
 }
-static int cbfs_payload_make_elf(struct buffer *buff, uint32_t arch)
+static int cbfs_payload_make_elf(struct buffer *buff, uint32_t arch,
+				 unused struct cbfs_file *entry)
 {
    Elf64_Ehdr ehdr;
    Elf64_Shdr shdr;
@@ -1257,7 +1204,7 @@
    }
if (elf_writer_serialize(ew, &elf_out)) {
-		ERROR("Unable to create ELF file from stage.\n");
+		ERROR("Unable to create ELF file from payload.\n");
    	goto out;
    }
@@ -1319,13 +1266,13 @@
    }
/*
-	 * The stage metadata is never compressed proper for cbfs_stage
-	 * files. The contents of the stage data can be though. Therefore
-	 * one has to do a second pass for stages to potentially decompress
-	 * the stage data to make it more meaningful.
+	 * We want to export stages and payloads as ELFs, not with coreboot's
+	 * custom stage/SELF binary formats, so we need to do extra processing
+	 * to turn them back into an ELF.
     */
    if (do_processing) {
-		int (*make_elf)(struct buffer *, uint32_t) = NULL;
+		int (*make_elf)(struct buffer *, uint32_t,
+				struct cbfs_file *) = NULL;
    	switch (ntohl(entry->type)) {
    	case CBFS_TYPE_STAGE:
    		make_elf = cbfs_stage_make_elf;
@@ -1334,7 +1281,7 @@
    		make_elf = cbfs_payload_make_elf;
    		break;
    	}
-		if (make_elf && make_elf(&buffer, arch)) {
+		if (make_elf && make_elf(&buffer, arch, entry)) {
    		ERROR("Failed to write %s into %s.\n",
    		      entry_name, filename);
    		buffer_delete(&buffer);
@@ -1386,17 +1333,29 @@
    return 0;
 }
-static int cbfs_print_stage_info(struct cbfs_stage *stage, FILE* fp)
+static int cbfs_print_stage_info(struct cbfs_file *entry, FILE* fp)
 {
+
+	struct cbfs_file_attr_stageheader *stage = NULL;
+	for (struct cbfs_file_attribute *attr = cbfs_file_first_attr(entry);
+	     attr != NULL; attr = cbfs_file_next_attr(entry, attr)) {
+		if (ntohl(attr->tag) == CBFS_FILE_ATTR_TAG_STAGEHEADER) {
+			stage = (struct cbfs_file_attr_stageheader *)attr;
+			break;
+		}
+	}
+
+	if (stage == NULL) {
+		fprintf(fp, "    ERROR: stage header not found!\n");
+		return -1;
+	}
+
    fprintf(fp,
-		"    %s compression, entry: 0x%" PRIx64 ", load: 0x%" PRIx64 ", "
-		"length: %d/%d\n",
-		lookup_name_by_type(types_cbfs_compression,
-				    stage->compression, "(unknown)"),
-		stage->entry,
-		stage->load,
-		stage->len,
-		stage->memlen);
+		"    entry: 0x%" PRIx64 ", load: 0x%" PRIx64 ", "
+		"memlen: %d\n",
+		ntohll(stage->loadaddr) + ntohl(stage->entry_offset),
+		ntohll(stage->loadaddr),
+		ntohl(stage->memlen));
    return 0;
 }
@@ -1518,8 +1477,7 @@
    /* note the components of the subheader may be in host order ... */
    switch (ntohl(entry->type)) {
    	case CBFS_TYPE_STAGE:
-			cbfs_print_stage_info((struct cbfs_stage *)
-					      CBFS_SUBHEADER(entry), fp);
+			cbfs_print_stage_info(entry, fp);
    		break;
case CBFS_TYPE_SELF:
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index 454aff9..d4d9829 100644
--- a/util/cbfstool/cbfstool.c
+++ b/util/cbfstool/cbfstool.c
@@ -14,7 +14,9 @@
 #include "cbfs_sections.h"
 #include "elfparsing.h"
 #include "partitioned_file.h"
+#include "lz4/lib/xxhash.h"
 #include <commonlib/bsd/cbfs_private.h>
+#include <commonlib/bsd/compression.h>
 #include <commonlib/bsd/metadata_hash.h>
 #include <commonlib/fsp.h>
 #include <commonlib/endian.h>
@@ -681,18 +683,7 @@
    				sizeof(struct cbfs_file_attr_position));
    		if (attrs == NULL)
    			return -1;
-			/* If we add a stage or a payload, we need to take  */
-			/* care about the additional metadata that is added */
-			/* to the cbfs file and therefore set the position  */
-			/* the real beginning of the data. */
-			if (type == CBFS_TYPE_STAGE)
-				attrs->position = htonl(offset +
-					sizeof(struct cbfs_stage));
-			else if (type == CBFS_TYPE_SELF)
-				attrs->position = htonl(offset +
-					sizeof(struct cbfs_payload));
-			else
-				attrs->position = htonl(offset);
+			attrs->position = htonl(offset);
    	}
    	/* Add alignment attribute if used */
    	if (param.alignment) {
@@ -872,6 +863,12 @@
    struct buffer output;
    int ret;
+	struct cbfs_file_attr_stageheader *stageheader = (void *)
+		cbfs_add_file_attr(header, CBFS_FILE_ATTR_TAG_STAGEHEADER,
+				   sizeof(struct cbfs_file_attr_stageheader));
+	if (!stageheader)
+		return -1;
+
    if (param.stage_xip) {
    	int32_t address;
    	size_t data_size;
@@ -881,8 +878,9 @@
    		return 1;
    	}
-		if (do_cbfs_locate(&address, sizeof(struct cbfs_stage),
-			data_size))  {
+		if (do_cbfs_locate(&address,
+				   sizeof(struct cbfs_file_attr_stageheader),
+				   data_size))  {
    		ERROR("Could not find location for XIP stage.\n");
    		return 1;
    	}
@@ -897,18 +895,57 @@
    	*offset = address;
ret = parse_elf_to_xip_stage(buffer, &output, offset,
-						param.ignore_section);
-	} else
-		ret = parse_elf_to_stage(buffer, &output, param.compression,
-					 offset, param.ignore_section);
-
+					     param.ignore_section,
+					     stageheader);
+	} else {
+		ret = parse_elf_to_stage(buffer, &output, offset,
+					 param.ignore_section, stageheader);
+	}
    if (ret != 0)
    	return -1;
+
+	/* Store a hash of original uncompressed stage to compare later. */
+	size_t decmp_size = buffer_size(&output);
+	uint32_t decmp_hash = XXH32(buffer_get(&output), decmp_size, 0);
+
+	/* Chain to base conversion routine to handle compression. */
+	ret = cbfstool_convert_raw(&output, offset, header);
+	if (ret != 0)
+		goto fail;
+
+	/* Special care must be taken for LZ4-compressed stages that the BSS is
+	   large enough to provide scratch space for in-place decompression. */
+	if (!param.precompression && param.compression == CBFS_COMPRESS_LZ4) {
+		size_t memlen = ntohl(stageheader->memlen);
+		size_t compressed_size = buffer_size(&output);
+		uint8_t *compare_buffer = malloc(memlen);
+		uint8_t *start = compare_buffer + memlen - compressed_size;
+		if (!compare_buffer) {
+			ERROR("Out of memory\n");
+			goto fail;
+		}
+		memcpy(start, buffer_get(&output), compressed_size);
+		ret = ulz4fn(start, compressed_size, compare_buffer, memlen);
+		if  (ret == 0) {
+			ERROR("Not enough scratch space to decompress LZ4 in-place -- increase BSS size or disable compression!\n");
+			free(compare_buffer);
+			goto fail;
+		} else if (ret != (int)decmp_size ||
+			   decmp_hash != XXH32(compare_buffer, decmp_size, 0)) {
+			ERROR("LZ4 compression BUG! Report to mailing list.\n");
+			free(compare_buffer);
+			goto fail;
+		}
+		free(compare_buffer);
+	}
+
    buffer_delete(buffer);
-	// Direct assign, no dupe.
-	memcpy(buffer, &output, sizeof(*buffer));
-	header->len = htonl(output.size);
+	buffer_clone(buffer, &output);
    return 0;
+
+fail:
+	buffer_delete(&output);
+	return -1;
 }
static int cbfstool_convert_mkpayload(struct buffer *buffer,
diff --git a/util/cbfstool/common.h b/util/cbfstool/common.h
index f1287e2..7c3539a 100644
--- a/util/cbfstool/common.h
+++ b/util/cbfstool/common.h
@@ -169,11 +169,12 @@
    			 enum cbfs_compression algo);
 /* cbfs-mkstage.c */
 int parse_elf_to_stage(const struct buffer *input, struct buffer *output,
-		       enum cbfs_compression algo, uint32_t *location,
-		       const char *ignore_section);
+		       uint32_t *location, const char *ignore_section,
+		       struct cbfs_file_attr_stageheader *stageheader);
 /* location is TOP aligned. */
 int parse_elf_to_xip_stage(const struct buffer *input, struct buffer *output,
-				uint32_t *location, const char *ignore_section);
+			   uint32_t *location, const char *ignore_section,
+			   struct cbfs_file_attr_stageheader *stageheader);
void print_supported_architectures(void);
 void print_supported_filetypes(void);
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/46484
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8471ad7494b07599e24e82b81e507fcafbad808a
Gerrit-Change-Number: 46484
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner jwerner@chromium.org
Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org
Gerrit-MessageType: newchange



    