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.tau...@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/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
