Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39305 )
Change subject: cbfs: Port cbfs_load() and cbfs_map() to new API ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39305/7/src/commonlib/bsd/cbfs_priv... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/39305/7/src/commonlib/bsd/cbfs_priv... PS7, Line 172: return attr;
Should we be checking length of attribute actually fits within mdata? i.e. […]
Hmm... hmmmmmmmm... yeah, you know what, that's a very good point. And then the caller also needs to check the length again before it can assume it can safely dereference attribute-specific fields. And in fact the while loop here also needs to make sure tag and len can be safely dereferenced. I guess I really didn't pay enough attention here. (Maybe I thought it's fine because verification would've already failed at this point? But still, we should always do proper bounds checking.)
Ideally we should have an API that automatically checks the whole thing is safe to use without requiring extra action from the caller. But some attributes like hashes have variable size... hmm... I think I'll add an optional size field to the function so callers can pass sizeof(*attr) to get automatic checking for the common case of fixed-size attributes, but also leave it out in cases where the size may be variable.