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

[Maximilian Brune (Code Review)]

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

Maximilian Brune 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:

(5 comments)

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

https://review.coreboot.org/c/coreboot/+/79905/comment/2e6e744d_0f63bb43 :
PS1, Line 99: 	assert(offset + size - 1 >= offset);
> But the assert, it does nothing...

I am not sure I understand. I thought it is checking for an overflow which shouldn't happen since region_create is called from "trusted" code?
Looks right to me

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

Personally I like to separate them in order to avoid having to return nested error codes everywhere. I am in favor of checking input values once and as early as possible.

So instead of something like this:
```
int smm_handler_xyx(size_t base, size_t size)
{
    if (function_using_region(base, size)) {
        return -1;
    }
}
int function_using_region(size_t base, size_t size)
{
    struct region r;
    if (region_create_untrusted(&r, base, size)) {
        return -1;
    }
    ...
}
```
I like it more like this:
```
int smm_handler_xyx(size_t base, size_t size)
{
    if (base + size < base) {
        return -1; // overflow detected (possible malicious input values)
    }
    function_using_region(base, size);
}
void function_using_region(size_t base, size_t size)
{
    struct region r = region_create(base, size);
    ...
}
```

https://review.coreboot.org/c/coreboot/+/79905/comment/f3d97699_3a823cd3 :
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?

https://review.coreboot.org/c/coreboot/+/79905/comment/745aac48_6d375b42 :
PS1, Line 106: offset > SIZE_MAX || size > SIZE_MAX 
I am puzzled. Under which circumstances can offset or size be bigger than SIZE_MAX?

File util/cbfstool/cbfstool.c:

https://review.coreboot.org/c/coreboot/+/79905/comment/618d23ac_f77f0c18 :
PS1, Line 334: 	if (region_create_untrusted(
If I understand correctly the only real difference between `region_create` and `region_create_untrusted` is that `region_create` asserts and `region_create_untrusted` gives the opportunity to handle the error?

In that case it seems more reasonable to assert if the regions (ranges) come from coreboot internal code (compared to external) in order for the developer to easier catch the error. And for regions coming from external stuff (e.g. SMM) to just handle the error in order to not crash the system unnecessarily (like you already do).

Having said that it seems to make more sense to just use region_create here?