Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
ifdtool: Add validate option to ifdtool
Add an option to ifdtool which validates that the flash regions defined in the descriptor match the coresponding areas in the FMAP.
BUG=chromium:992215 TEST=Ran 'ifdtool -t' with a good bios image and verify no issues run 'ifdtool -t' with a bad bios image and verify expected issues
Signed-off-by: Mathew King mathewk@chromium.org Change-Id: Idebf105dee1b8f829d54bd65c82867af7aa4aded Reviewed-on: https://review.coreboot.org/c/coreboot/+/34802 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org --- R Documentation/ifdtool/binary_extraction.md A Documentation/ifdtool/index.md A Documentation/ifdtool/layout.md M Documentation/index.md M Documentation/util.md M util/ifdtool/Makefile M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 8 files changed, 150 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved
diff --git a/Documentation/Binary_Extraction.md b/Documentation/ifdtool/binary_extraction.md similarity index 100% rename from Documentation/Binary_Extraction.md rename to Documentation/ifdtool/binary_extraction.md diff --git a/Documentation/ifdtool/index.md b/Documentation/ifdtool/index.md new file mode 100644 index 0000000..e6e6057 --- /dev/null +++ b/Documentation/ifdtool/index.md @@ -0,0 +1,5 @@ + +Contents: + +* [Intel IFD Binary Extraction](binary_extraction.md) +* [IFD Layout](layout.md) \ No newline at end of file diff --git a/Documentation/ifdtool/layout.md b/Documentation/ifdtool/layout.md new file mode 100644 index 0000000..950db6f --- /dev/null +++ b/Documentation/ifdtool/layout.md @@ -0,0 +1,66 @@ +# IFD Layout + +A coreboot image for an Intel SoC contains two separate definitions of the +layout of the flash. The Intel Flash Descriptor (IFD) which defines offsets and +sizes of various regions of flash and the [coreboot FMAP](../lib/flashmap.md). + +The FMAP should define all of the of the regions defined by the IFD to ensure +that those regions are accounted for by coreboot and will not be accidentally +modified. + +## IFD mapping + +The names of the IFD regions in the FMAP should follow the convention of +starting with the prefix `SI_` which stands for `silicon initialization` as a +way to categorize anything required by the SoC but not provided by coreboot. + +|IFD Region index|IFD Region name|FMAP Name|Notes| +|---|---|---|---| +|0|Flash Descriptor|SI_DESC|Always the top 4KB of flash| +|1|BIOS|SI_BIOS|This is the region that contains coreboot| +|2|Intel ME|SI_ME|| +|3|Gigabit Ethernet|SI_GBE|| +|4|Platform Data|SI_PDR|| +|8|EC Firmware|SI_EC|Most Chrome OS devices do not use this region; EC firmware is stored BIOS region of flash| + +## Validation + +The ifdtool can be used to manipulate a firmware image with a IFD. This tool +will not take into account the FMAP while modifying the image which can lead to +unexpected and hard to debug issues with the firmware image. For example if the +ME region is defined at 6 MB in the IFD but the FMAP only allocates 4 MB for the +ME, then when the ME is added by the ifdtool 6 MB will be written which could +overwrite 2 MB of the BIOS. + +In order to validate that the FMAP and the IFD are compatible the ifdtool +provides --validate (-t) option. `ifdtool -t` will read both the IFD and the +FMAP in the image and for every non empty region in the IFD if that region is +defined in the FMAP but the offset or size is different then the tool will +return an error. + +Example: + +```console +foo@bar:~$ ifdtool -t bad_image.bin +Region mismatch between bios and SI_BIOS + Descriptor region bios: + offset: 0x00400000 + length: 0x01c00000 + FMAP area SI_BIOS: + offset: 0x00800000 + length: 0x01800000 +Region mismatch between me and SI_ME + Descriptor region me: + offset: 0x00103000 + length: 0x002f9000 + FMAP area SI_ME: + offset: 0x00103000 + length: 0x006f9000 +Region mismatch between pd and SI_PDR + Descriptor region pd: + offset: 0x003fc000 + length: 0x00004000 + FMAP area SI_PDR: + offset: 0x007fc000 + length: 0x00004000 +``` \ No newline at end of file diff --git a/Documentation/index.md b/Documentation/index.md index b880c1c..39c8d11 100644 --- a/Documentation/index.md +++ b/Documentation/index.md @@ -169,7 +169,6 @@ * [coreboot at conferences](community/conferences.md) * [Payloads](payloads.md) * [Distributions](distributions.md) -* [Intel IFD Binary Extraction](Binary_Extraction.md) * [Dealing with Untrusted Input in SMM](technotes/2017-02-dealing-with-untrusted-input-in-smm.md) * [GPIO toggling in ACPI AML](acpi/gpio.md) * [Adding devices to a device tree](acpi/devicetree.md) diff --git a/Documentation/util.md b/Documentation/util.md index f8fabc1..3a09b00 100644 --- a/Documentation/util.md +++ b/Documentation/util.md @@ -47,7 +47,7 @@ * __genprof__ - Format function tracing logs `Bash` `C` * __gitconfig__ - Initialize git repository submodules install git hooks `Bash` -* __ifdtool__ - Extract and dump Intel Firmware Descriptor information +* [__ifdtool__](ifdtool/index.md) - Extract and dump Intel Firmware Descriptor information `C` * __intelmetool__ - Dump interesting things about Management Engine even if hidden `C` diff --git a/util/ifdtool/Makefile b/util/ifdtool/Makefile index cc52b1e..a4f0af6 100644 --- a/util/ifdtool/Makefile +++ b/util/ifdtool/Makefile @@ -18,10 +18,16 @@ CC = gcc INSTALL = /usr/bin/env install PREFIX = /usr/local -CFLAGS = -O2 -g -Wall -Wextra -Wmissing-prototypes -Werror -I../../src/commonlib/include +CFLAGS = -O2 -g -Wall -Wextra -Wmissing-prototypes -Werror +CFLAGS += -I../../src/commonlib/include +CFLAGS += -I../cbfstool/flashmap +CFLAGS += -include ../../src/commonlib/include/commonlib/compiler.h LDFLAGS =
OBJS = ifdtool.o +OBJS += fmap.o +OBJS += kv_pair.o +OBJS += valstr.o
all: dep $(PROGRAM)
@@ -38,6 +44,9 @@ %.o: %.c $(CC) $(CFLAGS) -c -o $@ $<
+%.o: ../cbfstool/flashmap/%.c + $(CC) $(CFLAGS) -c -o $@ $< + install: $(PROGRAM) mkdir -p $(DESTDIR)$(PREFIX)/bin $(INSTALL) $(PROGRAM) $(DESTDIR)$(PREFIX)/bin diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 83caa69..0e83c76 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -22,6 +22,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <commonlib/helpers.h> +#include <fmap.h> #include "ifdtool.h"
#ifndef O_BINARY @@ -46,15 +47,15 @@ static int platform = -1;
static const struct region_name region_names[MAX_REGIONS] = { - { "Flash Descriptor", "fd", "flashregion_0_flashdescriptor.bin" }, - { "BIOS", "bios", "flashregion_1_bios.bin" }, - { "Intel ME", "me", "flashregion_2_intel_me.bin" }, - { "GbE", "gbe", "flashregion_3_gbe.bin" }, - { "Platform Data", "pd", "flashregion_4_platform_data.bin" }, - { "Reserved", "res1", "flashregion_5_reserved.bin" }, - { "Reserved", "res2", "flashregion_6_reserved.bin" }, - { "Reserved", "res3", "flashregion_7_reserved.bin" }, - { "EC", "ec", "flashregion_8_ec.bin" }, + { "Flash Descriptor", "fd", "flashregion_0_flashdescriptor.bin", "SI_DESC" }, + { "BIOS", "bios", "flashregion_1_bios.bin", "SI_BIOS" }, + { "Intel ME", "me", "flashregion_2_intel_me.bin", "SI_ME" }, + { "GbE", "gbe", "flashregion_3_gbe.bin", "SI_GBE" }, + { "Platform Data", "pd", "flashregion_4_platform_data.bin", "SI_PDR" }, + { "Reserved", "res1", "flashregion_5_reserved.bin", NULL }, + { "Reserved", "res2", "flashregion_6_reserved.bin", NULL }, + { "Reserved", "res3", "flashregion_7_reserved.bin", NULL }, + { "EC", "ec", "flashregion_8_ec.bin", "SI_EC" }, };
/* port from flashrom */ @@ -804,6 +805,51 @@ } }
+static void validate_layout(char *image, int size) +{ + uint i, errors = 0; + struct fmap *fmap; + long int fmap_loc = fmap_find((uint8_t *)image, size); + const frba_t *frba = find_frba(image, size); + + if (fmap_loc < 0 || !frba) + exit(EXIT_FAILURE); + + fmap = (struct fmap *)(image + fmap_loc); + + for (i = 0; i < max_regions; i++) { + if (region_names[i].fmapname == NULL) + continue; + + region_t region = get_region(frba, i); + + if (region.size == 0) + continue; + + const struct fmap_area *area = + fmap_find_area(fmap, region_names[i].fmapname); + + if (!area) + continue; + + if ((uint)region.base != area->offset || + (uint)region.size != area->size) { + printf("Region mismatch between %s and %s\n", + region_names[i].terse, area->name); + printf(" Descriptor region %s:\n", region_names[i].terse); + printf(" offset: 0x%08x\n", region.base); + printf(" length: 0x%08x\n", region.size); + printf(" FMAP area %s:\n", area->name); + printf(" offset: 0x%08x\n", area->offset); + printf(" length: 0x%08x\n", area->size); + errors++; + } + } + + if (errors > 0) + exit(EXIT_FAILURE); +} + static void write_image(const char *filename, char *image, int size) { char new_filename[FILENAME_MAX]; // allow long file names @@ -1359,6 +1405,7 @@ printf("\n" " -d | --dump: dump intel firmware descriptor\n" " -f | --layout <filename> dump regions into a flashrom layout file\n" + " -t | --validate Validate that the firmware descriptor layout matches the fmap layout\n" " -x | --extract: extract intel fd modules\n" " -i | --inject <region>:<module> inject file <module> into region <region>\n" " -n | --newlayout <filename> update regions using a flashrom layout file\n" @@ -1388,7 +1435,7 @@ { int opt, option_index = 0; int mode_dump = 0, mode_extract = 0, mode_inject = 0, mode_spifreq = 0; - int mode_em100 = 0, mode_locked = 0, mode_unlocked = 0; + int mode_em100 = 0, mode_locked = 0, mode_unlocked = 0, mode_validate = 0; int mode_layout = 0, mode_newlayout = 0, mode_density = 0; int mode_altmedisable = 0, altmedisable = 0; char *region_type_string = NULL, *region_fname = NULL; @@ -1413,10 +1460,11 @@ {"version", 0, NULL, 'v'}, {"help", 0, NULL, 'h'}, {"platform", 0, NULL, 'p'}, + {"validate", 0, NULL, 't'}, {0, 0, 0, 0} };
- while ((opt = getopt_long(argc, argv, "df:D:C:M:xi:n:s:p:eluvh?", + while ((opt = getopt_long(argc, argv, "df:D:C:M:xi:n:s:p:eluvth?", long_options, &option_index)) != EOF) { switch (opt) { case 'd': @@ -1593,6 +1641,9 @@ } fprintf(stderr, "Platform is: %s\n", optarg); break; + case 't': + mode_validate = 1; + break; case 'v': print_version(); exit(EXIT_SUCCESS); @@ -1608,7 +1659,7 @@
if ((mode_dump + mode_layout + mode_extract + mode_inject + mode_newlayout + (mode_spifreq | mode_em100 | mode_unlocked | - mode_locked) + mode_altmedisable) > 1) { + mode_locked) + mode_altmedisable + mode_layout) > 1) { fprintf(stderr, "You may not specify more than one mode.\n\n"); print_usage(argv[0]); exit(EXIT_FAILURE); @@ -1616,7 +1667,7 @@
if ((mode_dump + mode_layout + mode_extract + mode_inject + mode_newlayout + mode_spifreq + mode_em100 + mode_locked + - mode_unlocked + mode_density + mode_altmedisable) == 0) { + mode_unlocked + mode_density + mode_altmedisable + mode_validate) == 0) { fprintf(stderr, "You need to specify a mode.\n\n"); print_usage(argv[0]); exit(EXIT_FAILURE); @@ -1667,6 +1718,9 @@ if (mode_extract) write_regions(image, size);
+ if (mode_validate) + validate_layout(image, size); + if (mode_inject) inject_region(filename, image, size, region_type, region_fname); diff --git a/util/ifdtool/ifdtool.h b/util/ifdtool/ifdtool.h index f3b9a53..195a09c 100644 --- a/util/ifdtool/ifdtool.h +++ b/util/ifdtool/ifdtool.h @@ -165,4 +165,5 @@ const char *pretty; const char *terse; const char *filename; + const char *fmapname; };