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

[Carl-Daniel Hailfinger]

Am 22.09.2013 21:10 schrieb Stefan Tauner:
> This fixes a SEGFAULT if a layout entry is included that addresses memory
> outside the current chip's address range.
>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> ---
>
> Before continuing with the other layout patches, I'd like to fix this
> issue (and copy the patch that fixes this also to the .7 stable branch).

Good idea.


> The macros are open for discussion not only regarding naming...
> e.g. it might be easier to declare a macro that provides the full printf
> format specification for the address, '0x06" PRIx32 "' to make the
> resulting code more readable.
>
>  flash.h    | 11 ++++++++++-
>  flashrom.c | 11 ++++++++---
>  layout.c   | 29 +++++++++++++++++++++++++++--
>  3 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/flash.h b/flash.h
> index 7b88477..a5033ef 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -45,6 +45,14 @@
>  typedef uintptr_t chipaddr;
>  #define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2))
>  
> +/* To be used in technology-independent code. */
> +#define FL_MAX_CHIPADDR (24)

This looks suspiciously like the number of address bits. It's not, and
that confused me.


> +#define FL_MAX_CHIPSIZE ((1<<FL_MAX_CHIPADDR)-1)

AFAICS it should be 1ULL, not 1.


> +#define PRIxCHIPADDR_WIDTH ((int)(FL_MAX_CHIPADDR/4))

Incorrect rounding. If FL_MAX_CHIPADDR=23, PRIxCHIPADDR_WIDTH is 5, but
it should be 6. Suggested fix: Replace FL_MAX_CHIPADDR/4 with
(FL_MAX_CHIPADDR+3)/4.


> +typedef uint32_t chipoff_t; /* Able to store any addressable offset within a supported flash memory. */
> +typedef uint32_t chipsize_t; /* Able to store the number of bytes of any supported flash memory. */
> +#define PRIxCHIPADDR PRIx32
> +
>  int register_shutdown(int (*function) (void *data), void *data);
>  void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len);
>  void programmer_unmap_flash_region(void *virt_addr, size_t len);
> @@ -319,7 +327,8 @@ __attribute__((format(printf, 2, 3)));
>  int register_include_arg(char *name);
>  int process_include_args(void);
>  int read_romlayout(char *name);
> -int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents);
> +int normalize_romentries(const struct flashctx *flash);
> +int build_new_image(const struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents);

Good idea.


>  void layout_cleanup(void);
>  
>  /* spi.c */
> diff --git a/flashrom.c b/flashrom.c
> index 9169620..afab57c 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1916,6 +1916,12 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>  		goto out_nofree;
>  	}
>  
> +	if (normalize_romentries(flash)) {
> +		msg_cerr("Requested regions can not be handled. Aborting.\n");
> +		ret = 1;
> +		goto out_nofree;
> +	}
> +
>  	/* Given the existence of read locks, we want to unlock for read,
>  	 * erase and write.
>  	 */
> @@ -1995,9 +2001,8 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>  	}
>  	msg_cinfo("done.\n");
>  
> -	// This should be moved into each flash part's code to do it 
> -	// cleanly. This does the job.
> -	handle_romentries(flash, oldcontents, newcontents);
> +	/* Build a new image taking the given layout into account. */
> +	build_new_image(flash, oldcontents, newcontents);
>  
>  	// ////////////////////////////////////////////////////////////
>  
> diff --git a/layout.c b/layout.c
> index 86351b8..5adec67 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -37,7 +37,7 @@ typedef struct {
>  
>  /* rom_entries store the entries specified in a layout file and associated run-time data */
>  static romentry_t rom_entries[MAX_ROMLAYOUT];
> -static int num_rom_entries = 0; /* the number of valid rom_entries */
> +static int num_rom_entries = 0; /* the number of successfully parsed rom_entries */
>  
>  /* include_args holds the arguments specified at the command line with -i. They must be processed at some point
>   * so that desired regions are marked as "included" in the rom_entries list. */
> @@ -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) {

If you want to do this, please make sure struct romentry has members
start/end with type chipoff_t. Otherwise that check may backfire eventually.


> +			msg_gwarn("Warning: Address range of region \"%s\" exeeds the current chip's "
> +				  "address space.\n", rom_entries[i].name);
> +			if (rom_entries[i].included)
> +				ret = 1;
> +		}
> +		if (rom_entries[i].start >= rom_entries[i].end) {

Off-by-one: If start==end, the length is 1, but this check will trigger.
The check should be > instead of >=.


> +			msg_gwarn("Warning: Size of the address range of region \"%s\" is not positive.\n",
> +				  rom_entries[i].name);
> +			if (rom_entries[i].included)
> +				ret = 1;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +int build_new_image(const struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents)
>  {
>  	unsigned int start = 0;
>  	romentry_t *entry;

Looks good otherwise.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/