Attention is currently required from: Andrey Petrov, Jérémy Compostella, Martin L Roth, Maximilian Brune, Nico Huber, Ronak Kanabar.
Julius Werner 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/95cda181_d363b4f3 : PS1, Line 106: offset > SIZE_MAX || size > SIZE_MAX
size_t/SIZE_MAX are implementation defined. I'm rather sure that compiled […]
I agree with Max, I don't really see the point of this (and changing all the types to `unsigned long long`). Are you worried about someone calling `region_create()` with a literal `0x100000000ULL`? That seems unrealistic in practice to me, I can't think of a scenario where that would happen. And in all examples where regions are created from values that are passed in as parameters from elsewhere, those parameters ought to already be `size_t`s.
Also, `SIZE_MAX` may be different in cbfstool than in coreboot, so if this is supposed to protect against larger values from cbfstool getting encoded somewhere that coreboot later interprets as a region, it wouldn't do that either.
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/79905/comment/ca2adaec_86d6b8e5 : PS1, Line 334: if (region_create_untrusted(
These regions can come from the command line. It's a minor difference with […]
They're parsed via `strtol()` so they already cannot overflow a `size_t`. I think we should do all argument validation at the moment of parsing and then we don't need to worry about it later.