On Mon, 23 Sep 2013 09:51:03 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 23.09.2013 05:42 schrieb Stefan Tauner: > > This fixes a SEGFAULT if a layout entry is included that addresses memory > > outside the current chip's address range. > > > > Also, abort for non-write operations if a layout file is given. > > > > Signed-off-by: Stefan Tauner <[email protected]> > > Thanks for your patch! > Acked-by: Carl-Daniel Hailfinger <[email protected]> > with one small comment (no need to change the code, just think about it) > > > --- > > cli_classic.c | 6 ++++++ > > flash.h | 11 ++++++++++- > > flashrom.c | 11 ++++++++--- > > layout.c | 33 +++++++++++++++++++++++++++++---- > > 4 files changed, 53 insertions(+), 8 deletions(-) > > > > diff --git a/layout.c b/layout.c > > index 86351b8..5254115 100644 > > --- a/layout.c > > +++ b/layout.c > > @@ -232,7 +232,32 @@ romentry_t *get_next_included_romentry(unsigned int > > start) > > return best_entry; > > } > > > > -int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, > > uint8_t *newcontents) > > +/* Validate and - if needed - normalize layout entries. */ > > +int normalize_romentries(const struct flashctx *flash) > > +{ > > + chipsize_t total_size = flash->chip->total_size * 1024; > > + int ret = 0; > > + > > + int i; > > + for (i = 0; i < num_rom_entries; i++) { > > + if (rom_entries[i].start >= total_size || rom_entries[i].end >= > > total_size) { > > + msg_gwarn("Warning: Address range of region \"%s\" > > exceeds the current chip's " > > + "address space.\n", rom_entries[i].name); > > + if (rom_entries[i].included) > > Shouldn't we warn about invalid rom entries in all cases, not just when > they are active/included? I'm a bit torn on that question. > > > > + ret = 1; > > + } > > + if (rom_entries[i].start > rom_entries[i].end) { > > + msg_gwarn("Warning: Size of the address range of region > > \"%s\" is not positive.\n", > > + rom_entries[i].name); > > + if (rom_entries[i].included) > > Same here. Thanks for the review. As discussed on IRC the patch version for trunk does abort unconditionally in the second case because this is clearly a bug in the layout file. The trunk version was committed in r1751. I have also backported the change to our 0.9.7 branch and committed it in r1752. It will abort if the non-positive region is requested to be used only. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
