Attention is currently required from: Nico Huber.
Julius Werner 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/3492bc7f_f992d52e : PS3, Line 16: if (region_end(c) < region_offset(c))
It's not 100% the same thing, and IMHO, it looked like it's in the wrong place. […]
Yeah okay, fair enough, I definitely wasn't trying to say that this code is the end all, be all already. I just meant we already have this here, we don't need the same check again below. I believe once you replace `region_end()` with `region_last()`, the check here should be safe for all cases?
If we want to move the wraparound check to `region_create()` and just assume that all created regions are valid that's fine by me too, but then we need to make sure the remaining uses in this file (e.g. in `rdev_mmap()`) adhere to that as well.
https://review.coreboot.org/c/coreboot/+/79945/comment/ea0f481c_c202292c : 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 […]
I haven't been able to think of a use case for it yet, so I'd say let's try forbidding it and see if anything breaks. (We definitely *do* have use cases of regions ending at the end of the address space, so since we basically have to decide between only allowing one or the other, prioritizing that seems to make more sense.)