[PATCHes] cbfstool fixes

[Stefan Reinauer]

See attachments

* guard all mallocs in cbfstool
* fix an issue that could lead to cbfstool writing outside of its allocated
  memory

Signed-off-by: Stefan Reinauer

--- util/cbfstool/common.c	(.../branches/upstream/coreboot-v2)	
+++ util/cbfstool/common.c	(.../trunk/coreboot-v2)	
@@ -36,10 +36,16 @@
    fseek(file, 0, SEEK_END);
    *romsize_p = ftell(file);
    fseek(file, 0, SEEK_SET);
-	if (!content)
+	if (!content) {
    	content = malloc(*romsize_p);
-	else if (place == SEEK_END)
+		if (!content) {
+			printf("Could not get %d bytes for file %s\n",
+					*romsize_p, filename);
+			exit(1);
+		}
+	} else if (place == SEEK_END)
    	content -= *romsize_p;
+
    if (!fread(content, *romsize_p, 1, file)) {
    	printf("failed to read %s\n", filename);
    	return NULL;
@@ -255,6 +261,11 @@
    	*location -= headersize;
    }
    void *newdata = malloc(*datasize + headersize);
+	if (!newdata) {
+		printf("Could not get %d bytes for CBFS file.\n", *datasize +
+				headersize);
+		exit(1);
+	}
    struct cbfs_file *nextfile = (struct cbfs_file *)newdata;
    strncpy(nextfile->magic, "LARCHIVE", 8);
    nextfile->len = htonl(*datasize);
@@ -272,9 +283,16 @@
 {
    romsize = _romsize;
    unsigned char *romarea = malloc(romsize);
+	if (!romarea) {
+		printf("Could not get %d bytes of memory for CBFS image.\n",
+				romsize);
+		exit(1);
+	}
    memset(romarea, 0xff, romsize);
-	recalculate_rom_geometry(romarea);

+	// Set up physical/virtual mapping
+	offset = romarea + romsize - 0x100000000ULL;
+
    if (align == 0)
    	align = 64;

@@ -291,6 +309,9 @@
    master_header->offset = htonl(0);
    ((uint32_t *) phys_to_virt(0xfffffffc))[0] =
        virt_to_phys(master_header);
+
+	recalculate_rom_geometry(romarea);
+
    struct cbfs_file *one_empty_file =
        cbfs_create_empty_file((0 - romsize) & 0xffffffff,
    			   romsize - bootblocksize -
--- util/cbfstool/common.h	(.../branches/upstream/coreboot-v2)	
+++ util/cbfstool/common.h	(.../trunk/coreboot-v2)	
@@ -29,7 +29,7 @@

static uint32_t virt_to_phys(void *addr)
 {
-	return (long)(addr - offset) & 0xffffffff;
+	return (unsigned long)(addr - offset) & 0xffffffff;
 }

#define ALIGN(val, by) (((val) + (by)-1)&~((by)-1))
@@ -61,3 +61,5 @@

int add_file_to_cbfs(void *content, uint32_t contentsize, uint32_t location);
 void print_cbfs_directory(const char *filename);
+
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))

This separates the code for each command in cbfstool. For the good and for the
bad: It brings a certain amount of code duplication (some of which can be
cleaned up again, or get rid of by proper refactoring).
On the other hand now there's a very simple code flow for each command, rather
than for each operation. ie.

adding a file to a cbfs means:
 - open the cbfs
    - add the file
       - close the cbfs

rather than

open the cbfs:
 - do this for add, remove, but not for create

create a new lar
 - if we don't have an open one yet

add a file:
 - if we didn't bail out before

close the file:
 - if we didn't bail out before

The short term benefit is that this fixes a problem where cbfstool was trying
to add a file if you gave a non-existing command because it bailed out on
known, not on unknown commands.

Signed-off-by: Stefan Reinauer

--- util/cbfstool/cbfstool.c	(.../branches/upstream/coreboot-v2)	
+++ util/cbfstool/cbfstool.c	(.../trunk/coreboot-v2)	
@@ -23,50 +23,79 @@
 #include "common.h"
 #include "cbfs.h"

-int main(int argc, char **argv)
+typedef enum {
+	CMD_ADD,
+	CMD_ADD_PAYLOAD,
+	CMD_ADD_STAGE,
+	CMD_CREATE,
+	CMD_PRINT
+} cmd_t;
+
+struct command {
+	cmd_t id;
+	const char *name;
+	int (*function) (int argc, char **argv);
+};
+
+static int cbfs_add(int argc, char **argv)
 {
-	if (argc < 3) {
-		printf
-		    ("cbfstool: Management utility for CBFS formatted ROM images\n"
-		     "USAGE:\n" "cbfstool [-h]\n"
-		     "cbfstool FILE COMMAND [PARAMETERS]...\n\n" "OPTIONs:\n"
-		     " -h		Display this help message\n\n"
-		     "COMMANDs:\n"
-		     "add FILE NAME TYPE [base address]    Add a component\n"
-		     "add-payload FILE NAME [COMP] [base]  Add a payload to the ROM\n"
-		     "add-stage FILE NAME [COMP] [base]    Add a stage to the ROM\n"
-		     "create SIZE BSIZE BOOTBLOCK [ALIGN]  Create a ROM file\n"
-		     "print                                Show the contents of the ROM\n");
-		return 1;
-	}
    char *romname = argv[1];
    char *cmd = argv[2];
+	void *rom = loadrom(romname);

-	if (strcmp(cmd, "create") == 0) {
-		if (argc < 6) {
-			printf("not enough arguments to 'create'.\n");
-			return 1;
-		}
-		uint32_t size = strtoul(argv[3], NULL, 0);
-		/* ignore bootblock size. we use whatever we get and won't allocate any larger */
-		char *bootblock = argv[5];
-		uint32_t align = 0;
-		if (argc > 6)
-			align = strtoul(argv[6], NULL, 0);
-		return create_cbfs_image(romname, size, bootblock, align);
+	if (rom == NULL) {
+		printf("Could not load ROM image '%s'.\n", romname);
+		return 1;
    }

+	if (argc < 5) {
+		printf("not enough arguments to '%s'.\n", cmd);
+		return 1;
+	}
+
+	char *filename = argv[3];
+	char *cbfsname = argv[4];
+
+	uint32_t filesize = 0;
+	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	if (filedata == NULL) {
+		printf("Could not load file '%s'.\n", filename);
+		return 1;
+	}
+
+	uint32_t base = 0;
+	void *cbfsfile = NULL;
+
+	if (argc < 6) {
+		printf("not enough arguments to 'add'.\n");
+		return 1;
+	}
+	uint32_t type;
+	if (intfiletype(argv[5]) != ((uint64_t) - 1))
+		type = intfiletype(argv[5]);
+	else
+		type = strtoul(argv[5], NULL, 0);
+	if (argc > 6) {
+		base = strtoul(argv[6], NULL, 0);
+	}
+	cbfsfile =
+	    create_cbfs_file(cbfsname, filedata, &filesize, type, &base);
+	add_file_to_cbfs(cbfsfile, filesize, base);
+	writerom(romname, rom, romsize);
+	return 0;
+}
+
+static int cbfs_add_payload(int argc, char **argv)
+{
+	char *romname = argv[1];
+	char *cmd = argv[2];
    void *rom = loadrom(romname);
+
    if (rom == NULL) {
    	printf("Could not load ROM image '%s'.\n", romname);
    	return 1;
    }

-	if (strcmp(cmd, "print") == 0) {
-		print_cbfs_directory(romname);
-		return 0;
-	}
-
    if (argc < 5) {
    	printf("not enough arguments to '%s'.\n", cmd);
    	return 1;
@@ -83,59 +112,150 @@
    }

uint32_t base = 0;
-	void *cbfsfile;
+	void *cbfsfile = NULL;

-	if (strcmp(cmd, "add") == 0) {
-		if (argc < 6) {
-			printf("not enough arguments to 'add'.\n");
-			return 1;
-		}
-		uint32_t type;
-		if (intfiletype(argv[5]) != ((uint64_t) - 1))
-			type = intfiletype(argv[5]);
-		else
-			type = strtoul(argv[5], NULL, 0);
-		if (argc > 6) {
-			base = strtoul(argv[6], NULL, 0);
-		}
-		cbfsfile =
-		    create_cbfs_file(cbfsname, filedata, &filesize, type,
-				     &base);
+	comp_algo algo = CBFS_COMPRESS_NONE;
+	if (argc > 5) {
+		if (argv[5][0] == 'l')
+			algo = CBFS_COMPRESS_LZMA;
    }
+	if (argc > 6) {
+		base = strtoul(argv[6], NULL, 0);
+	}
+	unsigned char *payload;
+	filesize = parse_elf_to_payload(filedata, &payload, algo);
+	cbfsfile =
+	    create_cbfs_file(cbfsname, payload, &filesize,
+			     CBFS_COMPONENT_PAYLOAD, &base);
+	add_file_to_cbfs(cbfsfile, filesize, base);
+	writerom(romname, rom, romsize);
+	return 0;
+}

-	if (strcmp(cmd, "add-payload") == 0) {
-		comp_algo algo = CBFS_COMPRESS_NONE;
-		if (argc > 5) {
-			if (argv[5][0] == 'l')
-				algo = CBFS_COMPRESS_LZMA;
-		}
-		if (argc > 6) {
-			base = strtoul(argv[6], NULL, 0);
-		}
-		unsigned char *payload;
-		filesize = parse_elf_to_payload(filedata, &payload, algo);
-		cbfsfile =
-		    create_cbfs_file(cbfsname, payload, &filesize,
-				     CBFS_COMPONENT_PAYLOAD, &base);
+static int cbfs_add_stage(int argc, char **argv)
+{
+	char *romname = argv[1];
+	char *cmd = argv[2];
+	void *rom = loadrom(romname);
+
+	if (rom == NULL) {
+		printf("Could not load ROM image '%s'.\n", romname);
+		return 1;
    }

-	if (strcmp(cmd, "add-stage") == 0) {
-		comp_algo algo = CBFS_COMPRESS_NONE;
-		if (argc > 5) {
-			if (argv[5][0] == 'l')
-				algo = CBFS_COMPRESS_LZMA;
-		}
-		if (argc > 6) {
-			base = strtoul(argv[6], NULL, 0);
-		}
-		unsigned char *stage;
-		filesize = parse_elf_to_stage(filedata, &stage, algo, &base);
-		cbfsfile =
-		    create_cbfs_file(cbfsname, stage, &filesize,
-				     CBFS_COMPONENT_STAGE, &base);
+	if (argc < 5) {
+		printf("not enough arguments to '%s'.\n", cmd);
+		return 1;
    }

+	char *filename = argv[3];
+	char *cbfsname = argv[4];
+
+	uint32_t filesize = 0;
+	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	if (filedata == NULL) {
+		printf("Could not load file '%s'.\n", filename);
+		return 1;
+	}
+
+	uint32_t base = 0;
+	void *cbfsfile = NULL;
+
+	comp_algo algo = CBFS_COMPRESS_NONE;
+	if (argc > 5) {
+		if (argv[5][0] == 'l')
+			algo = CBFS_COMPRESS_LZMA;
+	}
+	if (argc > 6) {
+		base = strtoul(argv[6], NULL, 0);
+	}
+	unsigned char *stage;
+	filesize = parse_elf_to_stage(filedata, &stage, algo, &base);
+	cbfsfile =
+	    create_cbfs_file(cbfsname, stage, &filesize,
+			     CBFS_COMPONENT_STAGE, &base);
+
    add_file_to_cbfs(cbfsfile, filesize, base);
    writerom(romname, rom, romsize);
    return 0;
 }
+
+static int cbfs_create(int argc, char **argv)
+{
+	char *romname = argv[1];
+	char *cmd = argv[2];
+	if (argc < 6) {
+		printf("not enough arguments to 'create'.\n");
+		return 1;
+	}
+
+	uint32_t size = strtoul(argv[3], NULL, 0);
+	/* ignore bootblock size. we use whatever we get and won't allocate any larger */
+	char *bootblock = argv[5];
+	uint32_t align = 0;
+
+	if (argc > 6)
+		align = strtoul(argv[6], NULL, 0);
+
+	return create_cbfs_image(romname, size, bootblock, align);
+}
+
+static int cbfs_print(int argc, char **argv)
+{
+	char *romname = argv[1];
+	char *cmd = argv[2];
+	void *rom = loadrom(romname);
+
+	if (rom == NULL) {
+		printf("Could not load ROM image '%s'.\n", romname);
+		return 1;
+	}
+
+	print_cbfs_directory(romname);
+	return 0;
+}
+
+struct command commands[] = {
+	{CMD_ADD, "add", cbfs_add},
+	{CMD_ADD_PAYLOAD, "add-payload", cbfs_add_payload},
+	{CMD_ADD_STAGE, "add-stage", cbfs_add_stage},
+	{CMD_CREATE, "create", cbfs_create},
+	{CMD_PRINT, "print", cbfs_print}
+};
+
+void usage(void)
+{
+	printf
+	    ("cbfstool: Management utility for CBFS formatted ROM images\n"
+	     "USAGE:\n" "cbfstool [-h]\n"
+	     "cbfstool FILE COMMAND [PARAMETERS]...\n\n" "OPTIONs:\n"
+	     " -h		Display this help message\n\n"
+	     "COMMANDs:\n"
+	     "add FILE NAME TYPE [base address]    Add a component\n"
+	     "add-payload FILE NAME [COMP] [base]  Add a payload to the ROM\n"
+	     "add-stage FILE NAME [COMP] [base]    Add a stage to the ROM\n"
+	     "create SIZE BSIZE BOOTBLOCK [ALIGN]  Create a ROM file\n"
+	     "print                                Show the contents of the ROM\n");
+}
+
+int main(int argc, char **argv)
+{
+	int i;
+
+	if (argc < 3) {
+		usage();
+		return 1;
+	}
+
+	char *cmd = argv[2];
+
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		if (strcmp(cmd, commands[i].name) != 0)
+			continue;
+		return commands[i].function(argc, argv);
+	}
+
+	printf("Unknown command '%s'.\n", cmd);
+	usage();
+	return 1;
+}

[Peter Stuge]

Stefan Reinauer wrote:
> guard all mallocs in cbfstool
> fix an issue that could lead to cbfstool writing outside of its allocated
> memory
>
> Signed-off-by: Stefan Reinauer

Acked-by: Peter Stuge