Attention is currently required from: Jérémy Compostella, Martin L Roth, Nico Huber.
5 comments:
File src/commonlib/include/commonlib/region.h:
Patch Set #1, 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
Patch Set #1, 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);
...
}
```
Patch Set #1, 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?
Patch Set #1, 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:
Patch Set #1, 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?
To view, visit change 79905. To unsubscribe, or for help writing mail filters, visit settings.