Nico Huber has posted comments on this change. ( https://review.coreboot.org/23021 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 5:
(2 comments)
Generally, I think we should always assume a "sane" layout in the internal code to keep it simple (e.g. in flashrom.c). Regions with start above end should be filtered (warning) / denied (bail out) when reading the layout. Regions partially out of the flash chip's bounds should be sanitized (warning) / denied (bail out). This should be implemented in a follow-up if necessary.
Regarding overlapping regions: I don't see a use case. But if we want to allow them in the layout file, I'm ok with the current solution implemented here.
https://review.coreboot.org/#/c/23021/5/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23021/5/flashrom.c@1387 PS5, Line 1387: if ((entry->start > size) || (entry->end > size)) {
is entry->start always smaller than entry->end? afaik coreboot/util/ifdtool generates regions where […]
For IFD layouts, it's taken care of. For layout files, idk.
https://review.coreboot.org/#/c/23021/5/layout.c File layout.c:
https://review.coreboot.org/#/c/23021/5/layout.c@189 PS5, Line 189: /* Quotes or whitespace are not allowed in region names. */
I suppose this could go in the read_romlayout() function for when a layout file is provided. […]
What is sane? Regarding IFD region names, those are hardcoded, and always sane.
I don't see what we gain from this check here, if somebody wants to name his region "foo bar" or even ""foo bar"", why would it hurt? And if any- where, I'd filter the names when we read the layout file. Then, weird names in a -i argument would just not be found (but why add extra code???). What do I miss?
-
Nico Huber (Code Review)