Attention is currently required from: Martin Roth, Michał Żygowski, Marshall Dawson, Subrata Banik, Nikolai Vyssotski, Andrey Petrov, Patrick Rudolph, Nathaniel L Desimone. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56190 )
Change subject: src/drivers/intel/fsp2_0: allow larger FSP 2.0 header ......................................................................
Patch Set 5:
(4 comments)
File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/56190/comment/e136103c_5fe0a376 PS3, Line 14: looks_like_fsp_header
i believe upper boundary check is always prescribed isn't it. we don't know what for those additional bytes are being introduced?
Prescribed by? If there are additional bytes, it shouldn't really matter for the consumer unless there were assumptions that the payload starts right after the header. But that is not true. There are pointers to get to the required components within the payload. Given that having additional bytes in header which remain unused should be fine.
Assume for this case, if we really want to discard those additional 4 bytes of MultiSi API then can't we check if FSP_INFO_HEADER.HeaderRevision < 5 then FSP_INFO_HEADER..HeaderLength - 4 == 72 would help to find the integrity of the FSP header with FSP 2.0 isn't it ? (in this process we actually knew what we are discarding) vs a minimal boundary check?
But, how would you verify the integrity? There is nothing in the header that provides checksum. So, there is no way for coreboot to perform integrity check. Also, there is no way to know what version of EDK2 the header was built with. So, subtracting 4 works in this case but not for other cases.
If FSP_INFO_HEADER.HeaderRevision >= 5 then we are expecting MultiSi is added and in that case FSP_INFO_HEADER..HeaderLength should be 76 with FSP 2.2 spec?
Are you saying that instead of relying on FSP version, we should check Header version? That should work. But, it would still be the same checks. Currently, FSP version and header version are upreved at the same time. Thus, each header version has a required length field. As long as the provided header is at least as long as the required length, it shouldn't be a problem.
Sorry may be I'm thinking a loud about how someone can explode the situation and introduced few more APIs or some data fields without bootloader knowledge (may be an imaginary situation unless its actually appears)
If someone adds new APIs or new fields to the header without bootloader knowledge, then those fields would be considered invalid. The consumer (bootloader) has no way to know what that means and would simply discard those.
File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/56190/comment/e31a6c28_78223084 PS5, Line 17: if (CONFIG(PLATFORM_USES_FSP2_2))
Somewhat unrelated to this patch, but does it make sense to switch this from a config to a runtime o […]
I think we should evaluate that separately. For now, let's continue with this static selection.
https://review.coreboot.org/c/coreboot/+/56190/comment/eae589cb_55ac6fa8 PS5, Line 30:
Should we check the bytes 10 & 11 as well? […]
Doesn't hurt to check the FSP version against the version that is advertised to ensure Kconfig selection in coreboot is correct. But, like you said it should be done as a follow-up if required.
https://review.coreboot.org/c/coreboot/+/56190/comment/910b2f16_f08eb0a0 PS5, Line 36: FSP 2.0 header can actually have two lengths: 72 and 76. I think this can be dropped since this is the condition today, but might change in the future. Probably say:
"It is possible to build FSP with any version of EDK2 which could have introduced new fields in FSP_INFO_HEADER. The new fields will be ignored based on the reported FSP version. This check ensures that the reported header length is at least what the reported FSP version requires so that we do not access any out-of-bound bytes."
-
Furquan Shaikh (Code Review)