Attention is currently required from: Julius Werner.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79945?usp=email )
Change subject: [NEEDS TEST] region: Check for overflows after offset calculation ......................................................................
Patch Set 3:
(2 comments)
File src/commonlib/region.c:
https://review.coreboot.org/c/coreboot/+/79945/comment/2426522a_4293acaf : PS3, Line 16: if (region_end(c) < region_offset(c))
I don't think this patch is necessary because this check here basically already checks for exactly t […]
It's not 100% the same thing, and IMHO, it looked like it's in the wrong place. This version would wrongfully bail out for a child region that ends exactly at the end of the address space. It's a good thing, though, because now we know we don't have rdev chains in our codebase that would do this (I actually suspected this might be the case on x86).
I saw your patch earlier, and thank you for that! I was very puzzled by the code before I saw it.
About wrong place or not, IMO, we should decide first if we want to allow only legal regions to be passed between functions. See also https://ticket.coreboot.org/issues/522
From my point of view, we should probably put asserts everywhere (assuming that's not too much overhead). Or at least in all non-static APIs, and act as if they were there in the static functions.
https://review.coreboot.org/c/coreboot/+/79945/comment/527567f0_232110d1 : PS3, Line 25: return region_end(inner) - 1 >= region_offset(inner) && This version of the check would allow an inner region at the end of the address space, but not an empty one. I guess that's also something to decide: is an empty region legal? So far the opinions I've heard said no. And I agree.