Attention is currently required from: Andrey Petrov, Julius Werner, Jérémy Compostella, Martin L Roth, Maximilian Brune, Ronak Kanabar.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email )
Change subject: [RFC] region: Introduce region_create() functions ......................................................................
Patch Set 6:
(2 comments)
File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/c/coreboot/+/79905/comment/b7adbd88_8d799ff0 : PS1, Line 106: offset > SIZE_MAX || size > SIZE_MAX
I agree with Max, I don't really see the point of this (and changing all the types to `unsigned long […]
Well, the _unstrusted version is supposed to not make assumptions, but here we are, making assumptions. We can also _Static_assert() some of them, e.g. `SIZE_MAX >= ULONG_MAX`. I guess the current usage is that we deal with input from pointers and strtoul(). And that would be covered. But here is a (for now hypothetical) scenario that wouldn't be: somebody reads a 64-bit BAR and feeds us that.
If we add an _untrusted version, we should cover for all eventualities. If we decide for the alternative to always validate inputs manually before calling into the region API (and pray that everybody always gets it right), we don't have to discuss this further.
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/79905/comment/c59a7198_ff7169a4 : PS1, Line 334: if (region_create_untrusted(
They're parsed via `strtol()` so they already cannot overflow a `size_t`.
It's good that you know that. But IMHO too much to expect every developer and reviewer to have in mind. The idea of region_create_untrusted() is to provide something that is safe to use, no matter what. And to have a common way to validate unstrusted input. I don't like the alternative we currently live. Which I guess is, assume the developer does the right checks ahead, wait for somebody to perform some verification, file a bug, and only then fix things.