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. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email )
Change subject: [RFC] region: Introduce region_create() functions ......................................................................
Patch Set 6:
(3 comments)
Patchset:
PS6: Max, I'll try to push an update that keeps the original two-create-functions idea in the next few days. I don't want to ignore you. But it feels like we are getting nowhere when I try to imagine what you have in mind and probably imagine it wrong.
When the whole patch train is cleaned up, it should be easy to write an alternative version of this patch that uses a separate validity function. Then we could decide which version we like better. Does that sound ok?
File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/c/coreboot/+/79905/comment/2be928b8_76e46bf8 : PS1, Line 103: static inline int region_create_untrusted(
Well you have to repeat that for all sources that we deem "untrusted". I don't think there are many of them in coreboot. I personally can only think of SMM and maybe exception handlers in general (I may be missing a few right now).
Plus potential future cases.
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. We cannot that assume that the input values are only used for a `region_create`. I would feel better to first check all input values and after that point assume that they are "safe" and then use the usual functions. Of course that requires that the input values are always correctly checked, which may be not 100% realistic.
I'm sorry, I really fail to imagine any comprehensive solution in this direction that would make sense. IIUC, you mean to implement an input_is_valid() that is always right, independent from the region_create() use case? But that seems impossible. Validity always requires context. Currently, the region API is limited by `size_t`, but other APIs may have stronger requirements like things always fitting into 32-bit.
I also seems like we'd lose cohesion this way. If we check for an overflow that can happen because of an implementation detail of the region API, that should be obvious and not happen somewhere where the reader doesn't see the region API being used.
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/79905/comment/f243ca76_77dd3825 : PS1, Line 334: if (region_create_untrusted(
IMO command-line input doesn't count as untrusted. […]
We don't have to call it unstrusted. How about `create_region_checked()`? Just anything that says this is the version that checks and can fail.
You could also see it like this: Do you trust the command line to be free of typos?
-
Nico Huber (Code Review)