Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38421 )
Change subject: commonlib/bsd: Add new CBFS core implementation ......................................................................
Patch Set 11:
(15 comments)
https://review.coreboot.org/c/coreboot/+/38421/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38421/11//COMMIT_MSG@19 PS11, Line 19: y e
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 8: size_t devsize = cbfs_dev_size(dev); Maybe not terrible, but I think you should call this in cbfs_walk() and pass the value to this function. Otherwise, we'll be doing the size lookup for every file.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 52: be32toh(mdata.h.type) You already have local variable, 'type', for this.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 70: goto next_file; I can see where this might be the correct action for certain circumstances. Wouldn't we want to this to be fatal in others?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 75: offset + sizeof(mdata.h), todo) != todo) I have a comment below related to this as well, but I don't see how we're guaranteed the mdata has enough buffer.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 117: const size_t todo = be32toh(src->h.offset) - already_read; How do we know that: 1. cbfs_mdata is sufficiently large? 2. src->h.offset is >= already_read?
For the 2nd, I think we're relying on cbfs_walk() implementation to be the only sequence one would call copy_fill_metadata?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 135: args->namesize > already_read - offsetof(union cbfs_mdata, filename) Separate this out into its own condition with a comment? It seems this check doesn't concern itself with the attributes -- only the filename. I'm not understanding the logic. And the || below then assumes we can read into filename when it's not entirely clear. Were you intending || to be &&?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 136: memcmp(args->name, mdata->filename, args->namesize) != 0) We aren't ensuring filename (and CBFS_METADATA_MAX_SIZE coverage) is not being exceeded for out of bounds accesses.
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 What is the intention of private? Only a predetermined set of users should call these functions?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 14: This Perhaps call out the name (cbfs_glue.h) since this comment is large and it makes it clear what 'this' is referring to?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 18: * CBFS_ENABLE_HASHING Should be 0 to avoid linking hashing features, 1 otherwise. I think this should be more explicit as to what hashing is covered. It's just the metadata.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 49: empty entries What exactly does this mean? cbfs_file w/ no data, only metadata?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 82: enum cbfs_walk_flags Since this is a bit field shouldn't we be passing in an unsigned type for the flags? Those flags are not mutually exclusive.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 87: * |dst| and load remaining metadata from |dev| as required. I know we say copy here, but I think we should note that the fields are still in serialized format, i.e. big endian. I guess we should note the same is true everywhere w.r.t. this API. All cbfs_mdata is in serialized form?
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 94: Verify the metadata with |master_hash| if provided. From this description it's not clear what master_hash is representing for the target file. If it's truly the master hash for the entirety of cbfs metadata that would imply a full read of cbfs metadata to perform the calculation.
Also, if this is about verification why isn't master_hash object marked const?