Re: [flashrom] [PATCH] layout: Verify layout entries before building a new image using them.
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@student.tuwien.ac.at>
Thanks for your patch! Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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.
+ 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;
Regards, Carl-Daniel -- http://www.hailfinger.org/
On Mon, 23 Sep 2013 09:51:03 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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@student.tuwien.ac.at>
Thanks for your patch! Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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
participants (2)
-
Carl-Daniel Hailfinger -
Stefan Tauner