Re: [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
>
> 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