[M] Change in coreboot[main]: [RFC] region: Introduce region_create() functions

[Nico Huber (Code Review)]

Attention is currently required from: Jérémy Compostella, Martin L Roth, Maximilian Brune.

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 1:

(4 comments)

File src/commonlib/include/commonlib/region.h:

https://review.coreboot.org/c/coreboot/+/79905/comment/0056f63f_7faa6296 :
PS1, Line 103: static inline int region_create_untrusted(
> Does it make sense to mix the constructor and the checking of invalid input values? […]

Certainly possible but we would have to repeat the check for different
callers. And that this mustn't overflow is an implementation detail,
kind of the contract (pre-condition) of region_create() with the assert()
in place. Though, we have no way to enforce such contracts in C.

https://review.coreboot.org/c/coreboot/+/79905/comment/4deac652_ee637d70 :
PS1, Line 104: 		struct region *r, unsigned long long offset, unsigned long long size)
> Is there a specific reason why you use "unsigned long long" type?

It's the biggest there is. The idea is to ensure that values are not
accidentally truncated when calling the function. The _unstrusted
function should work 100%ly no matter what the caller looks like.

https://review.coreboot.org/c/coreboot/+/79905/comment/dbb41d18_081a2f40 :
PS1, Line 106: offset > SIZE_MAX || size > SIZE_MAX
> I am puzzled. […]

size_t/SIZE_MAX are implementation defined. I'm rather sure that compiled
for x86_32 size_t is 32 bits wide. Still, somebody would call the function
with a value >= 4G. (C compilers don't warn about implicit truncation, and
static analyzers often only do so when they have reason to believe that
the value is too big.)

File util/cbfstool/cbfstool.c:

https://review.coreboot.org/c/coreboot/+/79905/comment/25f26489_20406243 :
PS1, Line 334: 	if (region_create_untrusted(
> If I understand correctly the only real difference between `region_create` and `region_create_untrus […]

These regions can come from the command line. It's a minor difference with
the default behavior of assert(). Though, if I'd have a typo in my command line,
I'd prefer a proper error message over an annoying "assertion failed".

And generally, seeing command-line input as unstrusted seems right IMO.