Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38421 )
Change subject: commonlib/bsd: Add new CBFS core implementation ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 75: offset + sizeof(mdata.h), todo) != todo)
How far does one go? And how easily can we identify such conditions early? I very much worry about r […]
Sorry, I'm not really following anymore here... what does reading the message have to do with anything? My code is safe, with or without the change you're asking for... it's not executing the parts that rely on the size limit if that limit is exceeded. What does it matter for safety whether it jumps away towards examining the next entry or towards returning immediately? As long as it doesn't pass the overflowing file entry back out to anywhere, I don't see the difference.
If you're worried about people not noticing that they hit that line, well... first of all, with the new version of cbfstool (https://review.coreboot.org/c/coreboot/+/41117/5/util/cbfstool/cbfs_image.c#...) it becomes impossible to create such an entry. Also, if coreboot itself is actually trying to use this stack to look up that file, it's going to fail with a CB_CBFS_NOT_FOUND (because the entry is skipped before its name can be matched). So the only way this case can be encountered but not fail hard is when that file exists in CBFS, added by an old copy of cbfstool, but isn't directly looked up by coreboot itself and metadata verification is not enabled. That probably means that the user added it themselves outside the coreboot build system to either be read by a payload (likely with an equally old CBFS implementation that could find and read this) or just serve some other informational purpose. In that case, I don't see why I should brick their whole boot when coreboot could just as well just ignore it.
Since I'm the one introducing a (tiny) backwards-incompatible change in the CBFS format here, I think it's just appropriate to try to make a best effort to not break anyone with it where possible.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 4: PRIVATE
Sounds good, added comment. (Note that CB:39327 also adds src/include/cbfs_private. […]
Well, I have the high level design documentation (see links in the commit message), it's not like I'm going in completely blind here. It's just that some of the finer details you only really understand when you try to implement them and see where problems pop up. For example, when implementing the metadata hash generation in cbfstool I noticed that some SoC's have special header checksums for the bootblock that need to be recalculated every time, so I had to implement CB:41122. When trying to pull stage loading into the new API I realized that I can't trust the stage header before I loaded and hashed the whole file, but I can't load the whole file without info from the stage header, so I had to factor the stage header out into a CBFS attribute (patch coming soon). I tried hard to document all possible details beforehand, but you just can't think of everything... that's why I'm saying some decisions (especially about code organization) are still outstanding until I've ported every existing use case and thus know exactly what APIs we need where.