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 by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email )
Change subject: region: Introduce region_create() functions ......................................................................
Patch Set 7:
(4 comments)
Patchset:
PS6:
Max, I'll try to push an update that keeps the original two-create-functions […]
So, looks like I forgot to push ._. found an updated version locally that I only had to rebase (and handle one more new case). Please tell me next time before I embarrass myself. (Now I even see old comment drafts /o\ )
File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/c/coreboot/+/79905/comment/6f33e9b9_a78bcf17?usp... : PS6, Line 103: int
nit: should return value be `enum cb_err`?
Done
File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/c/coreboot/+/79905/comment/e8bbd5ee_e5813b36?usp... : PS1, Line 99: assert(offset + size - 1 >= offset);
But the assert, it does nothing... […]
We decided not to allow empty regions. The assertion will trigger on empty regions and overflows.
https://review.coreboot.org/c/coreboot/+/79905/comment/11148613_00701a6b?usp... : PS1, Line 103: static inline int region_create_untrusted(
I think I am missing an actual example where the separation of the validity check and the region_create() can lead to issues.
You provided this yourself already:
But I would like to see a input_is_valid function of some kind at the beginning of all our untrusted sources anyway. Because these input values may be used for something else than region_create and are then unsafe again.
This would reduce cohesion which is considered bad software design. Knowing that input is safe to be used with the region API requires internal knowlegde of the region implementation (e.g. how size_t is used). If you drag this knowledge to the point where you encounter the inputs, everything gets coupled: The code that calls/performs the check would have to know that some layers deeper the region API is used and in what way; the code that actually uses the API would have to know that its caller already performed the necessary checks. Like spaghetti code at a design level.