Mathew King has uploaded this change for review. ( 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
Change-Id: Idebf105dee1b8f829d54bd65c82867af7aa4aded --- M util/ifdtool/Makefile M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 3 files changed, 79 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/1
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..eb9b420 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,8 @@
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 +1719,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; };
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34802/1/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/34802/1/util/ifdtool/ifdtool.c@811 PS1, Line 811: struct fmap* fmap; "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/34802/1/util/ifdtool/ifdtool.c@1670 PS1, Line 1670: mode_unlocked + mode_density + mode_altmedisable + trailing whitespace
Hello Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34802
to look at the new patch set (#2).
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
Change-Id: Idebf105dee1b8f829d54bd65c82867af7aa4aded --- M util/ifdtool/Makefile M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 3 files changed, 79 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34802/2/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/34802/2/util/ifdtool/ifdtool.c@811 PS2, Line 811: struct fmap* fmap; "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/34802/2/util/ifdtool/ifdtool.c@1670 PS2, Line 1670: mode_unlocked + mode_density + mode_altmedisable + trailing whitespace
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34802
to look at the new patch set (#3).
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
Change-Id: Idebf105dee1b8f829d54bd65c82867af7aa4aded --- M util/ifdtool/Makefile M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 3 files changed, 79 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/3
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34802
to look at the new patch set (#4).
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 --- M util/ifdtool/Makefile M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 3 files changed, 79 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
Hello Mathew,
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
Patch Set 4:
Hello Mathew,
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
Patch Set 4:
Hello Mathew,
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Nico, what are you requesting exactly? I'm not entirely sure, but it sounds like you are assuming coreboot shouldn't care about other regions covered by the descriptor. As system builders we definitely care about managing all regions/assets that comprise the spi flash. That may not be true for all users of coreboot, but it's definitely true for Chrome OS.
As for SI prefix, I actually don't know. Silicon Initialization? That sounds like an intel term enough that it might be that. I never bothered to look up where that came from. Perhaps Duncan remembers?
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
That's assuming one only cares about the BIOS region. And the check is that things are hierarchical within those regions w/o overlaps. If you care about covering every region then those same checks would apply to other regions covered in the descriptor.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Hello Mathew,
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Nico, what are you requesting exactly? I'm not entirely sure, but it sounds like you are assuming coreboot shouldn't care about other regions covered by the descriptor. As system builders we definitely care about managing all regions/assets that comprise the spi flash. That may not be true for all users of coreboot, but it's definitely true for Chrome OS.
Yep this is exactly why I added SI_DESC and SI_ME initially. We debated just having one region to cover everything, but since the descriptor is always 4K it seemed easy to keep it separate. Now of course we've started having some systems with SI_GBE and SI_EC regions which are also needed for init but not provided by coreboot. Nothing says these all have to be within the same region of flash, but that is how it has ended up so far.
As for SI prefix, I actually don't know. Silicon Initialization? That sounds like an intel term enough that it might be that. I never bothered to look up where that came from. Perhaps Duncan remembers?
Yes it is for Silicon Init, we wanted to categorize anything required by the silicon but not provided by coreboot.
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
That's assuming one only cares about the BIOS region. And the check is that things are hierarchical within those regions w/o overlaps. If you care about covering every region then those same checks would apply to other regions covered in the descriptor.
In theory we could auto-generate the FMAP regions for this based on the descriptor provided at build time. But we would need to figure out how to mesh that with the static FMAP definition.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Nico, what are you requesting exactly?
A definition of what this validation actually means. Preferably something in Documentation/ that the help text of ifdtool can refer to?
I'm not entirely sure, but it sounds like you are assuming coreboot shouldn't care about other regions covered by the descriptor. As system builders we definitely care about managing all regions/assets that comprise the spi flash. That may not be true for all users of coreboot, but it's definitely true for Chrome OS.
Sure. I meant coreboot doesn't care (at runtime). The build infrastructure does, ofc.
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
That's assuming one only cares about the BIOS region. And the check is that things are hierarchical within those regions w/o overlaps. If you care about covering every region then those same checks would apply to other regions covered in the descriptor.
Just wanted to give an example, what people might expect if they read "validate that the firmware descriptor layout matches the fmap layout". As the whole thing is not impli- citly defined by coreboot's semantics, I think it's worth some documentation.
IMO, it doesn't assume that one only cares about the BIOS region, btw. There are other ways to discover descriptor regions beside copying them into fmap.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
Patch Set 4:
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Nico, what are you requesting exactly?
A definition of what this validation actually means. Preferably something in Documentation/ that the help text of ifdtool can refer to?
I'm not entirely sure, but it sounds like you are assuming coreboot shouldn't care about other regions covered by the descriptor. As system builders we definitely care about managing all regions/assets that comprise the spi flash. That may not be true for all users of coreboot, but it's definitely true for Chrome OS.
Sure. I meant coreboot doesn't care (at runtime). The build infrastructure does, ofc.
Yes, coreboot doesn't proper. But many other components can and do.
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
That's assuming one only cares about the BIOS region. And the check is that things are hierarchical within those regions w/o overlaps. If you care about covering every region then those same checks would apply to other regions covered in the descriptor.
Just wanted to give an example, what people might expect if they read "validate that the firmware descriptor layout matches the fmap layout". As the whole thing is not impli- citly defined by coreboot's semantics, I think it's worth some documentation.
Your request makes sense.
IMO, it doesn't assume that one only cares about the BIOS region, btw. There are other ways to discover descriptor regions beside copying them into fmap.
That's true. We're in the position of needing to manage a heterogeneous fleet of systems, and the tooling being consistent is helpful there. That's where our Chrome OS reliance on FMAP is helpful as opposed to querying registers at runtime, e.g.. I hope that provides more context (if that was the request aside from adding documentation).
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Nico, what are you requesting exactly?
A definition of what this validation actually means. Preferably something in Documentation/ that the help text of ifdtool can refer to?
I'm not entirely sure, but it sounds like you are assuming coreboot shouldn't care about other regions covered by the descriptor. As system builders we definitely care about managing all regions/assets that comprise the spi flash. That may not be true for all users of coreboot, but it's definitely true for Chrome OS.
Sure. I meant coreboot doesn't care (at runtime). The build infrastructure does, ofc.
Yes, coreboot doesn't proper. But many other components can and do.
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
That's assuming one only cares about the BIOS region. And the check is that things are hierarchical within those regions w/o overlaps. If you care about covering every region then those same checks would apply to other regions covered in the descriptor.
Just wanted to give an example, what people might expect if they read "validate that the firmware descriptor layout matches the fmap layout". As the whole thing is not impli- citly defined by coreboot's semantics, I think it's worth some documentation.
Your request makes sense.
IMO, it doesn't assume that one only cares about the BIOS region, btw. There are other ways to discover descriptor regions beside copying them into fmap.
That's true. We're in the position of needing to manage a heterogeneous fleet of systems, and the tooling being consistent is helpful there. That's where our Chrome OS reliance on FMAP is helpful as opposed to querying registers at runtime, e.g.. I hope that provides more context (if that was the request aside from adding documentation).
I will add documentation for this change that includes motivation and examples.
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34802
to look at the new patch set (#5).
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 --- R Documentation/ifdtool/binary_extraction.md A Documentation/ifdtool/index.md M Documentation/index.md M Documentation/util.md M util/README.md M util/ifdtool/Makefile M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 8 files changed, 86 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/5
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34802
to look at the new patch set (#6).
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 --- 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, 136 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/6
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 6:
Patch Set 4:
Patch Set 4:
Patch Set 4:
usually I'm asked to write documentation, but now I'm the one that actually misses some information. I know it is common for chromium firmware to have rendundant FMAP entries for the descriptor regions. But, AFAICS, this is nowhere documented. Especially the reason for these entries would be nice to know, as coreboot it- self doesn't care about them. Oh, and what does the SI prefix stand for?
Nico, what are you requesting exactly?
A definition of what this validation actually means. Preferably something in Documentation/ that the help text of ifdtool can refer to?
I'm not entirely sure, but it sounds like you are assuming coreboot shouldn't care about other regions covered by the descriptor. As system builders we definitely care about managing all regions/assets that comprise the spi flash. That may not be true for all users of coreboot, but it's definitely true for Chrome OS.
Sure. I meant coreboot doesn't care (at runtime). The build infrastructure does, ofc.
Yes, coreboot doesn't proper. But many other components can and do.
Without such documentation, a validation option can be confusing. For instance, if I were told to imple- ment such an option in ifdtool, I would rather check that all FMAP regions are within the IFD BIOS region, because I wouldn't expect redundancy.
That's assuming one only cares about the BIOS region. And the check is that things are hierarchical within those regions w/o overlaps. If you care about covering every region then those same checks would apply to other regions covered in the descriptor.
Just wanted to give an example, what people might expect if they read "validate that the firmware descriptor layout matches the fmap layout". As the whole thing is not impli- citly defined by coreboot's semantics, I think it's worth some documentation.
Your request makes sense.
IMO, it doesn't assume that one only cares about the BIOS region, btw. There are other ways to discover descriptor regions beside copying them into fmap.
That's true. We're in the position of needing to manage a heterogeneous fleet of systems, and the tooling being consistent is helpful there. That's where our Chrome OS reliance on FMAP is helpful as opposed to querying registers at runtime, e.g.. I hope that provides more context (if that was the request aside from adding documentation).
I will add documentation for this change that includes motivation and examples.
PTAL I added some documentation about the validation.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... File Documentation/ifdtool/layout.md:
PS6: I think there's a line length limit for .md files, but I am not sure if it's specified anywhere
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... PS6, Line 9: SI_ Maybe add quotes or backticks to highlight this prefix
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... PS6, Line 9: silicon initialization Same as SI_
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... PS6, Line 18: elsewhere I guess this implies the EC firmware is either on the EC itself or on a dedicated flash chip?
https://review.coreboot.org/c/coreboot/+/34802/6/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/34802/6/util/ifdtool/ifdtool.c@1671 PS6, Line 1671: mode_validate) == 0) { Very minor: Looks like this would fit on the previous line without exceeding the 96-character length limit
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34802
to look at the new patch set (#7).
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 --- 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(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/34802/7
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... File Documentation/ifdtool/layout.md:
PS6:
I think there's a line length limit for . […]
Done
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... PS6, Line 9: SI_
Maybe add quotes or backticks to highlight this prefix
Done
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... PS6, Line 9: silicon initialization
Same as SI_
Done
https://review.coreboot.org/c/coreboot/+/34802/6/Documentation/ifdtool/layou... PS6, Line 18: elsewhere
I guess this implies the EC firmware is either on the EC itself or on a dedicated flash chip?
Done
https://review.coreboot.org/c/coreboot/+/34802/6/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/34802/6/util/ifdtool/ifdtool.c@1671 PS6, Line 1671: mode_validate) == 0) {
Very minor: Looks like this would fit on the previous line without exceeding the 96-character length […]
Done
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34802 )
Change subject: ifdtool: Add validate option to ifdtool ......................................................................
Patch Set 7: Code-Review+2
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; };
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Duncan Laurie (Code Review)
-
Mathew King (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)