[flashrom] [PATCH] layout: Verify layout entries before building a new image using them.

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Mon Sep 23 17:48:19 CEST 2013


On Mon, 23 Sep 2013 09:51:03 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <stefan.tauner at student.tuwien.ac.at>
> 
> Thanks for your patch!
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 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




More information about the flashrom mailing list