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 4:
(16 comments)
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 10: cbfs_dev_size(dev) Shouldn't we just call this once? The size of dev shouldn't change.
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 29: ENABLE_HASHING This should probably be namespaced better.
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 54: goto next_file; Shouldn't this be a major error? If we're wanting to ensure we touch every file this algo just bypasses things that aren't the correct size.
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 56: if (offset + data_offset >= cbfs_dev_size(dev)) { Wouldn't this fail on the read()? But if you are wanting to check this shouldn't you be ensuring the data length doesn't exceed the device size too?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 77: (uint8_t *)&mdata Shouldn't we be reading into the byte array here and above? Then interpret through the union for the cbfs_file?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 88: For others, finish the hash first if needed. Could you please elaborate on why?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 102: sizeof(real_hash) sorry for not being familiar w/ vb2 APIs, but it seems weird to pass in sizeof(real_hash) when it's just the possible max -- not the actual digest size.
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/cbfs_priv... PS4, Line 123: There doesn't appear to be any checks against the size of the structures. offset and attributes_offset could exceed the size of cbf_mdata. You're checking data_offset above. But not that the data fits.
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 61: header, filename and attributes Shouldn't we be passing along the metadata size? Why is the callback needing the logic to figure out where the attributes and filename live? I know's it's some addition, but it seems better to calculate that up front?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 69: * the remaining CBFS (but not call |walker| anymore). This function seems a bit overloaded. Can't we wrap all this with some entry points that provide the semantics we need w/o throwing everything into a single call?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 72: union cbfs_mdata *mdata Why isn't this const?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 80: to copy behind that I don't understand what this is saying. If do_hash is true it won't copy additional attributes? What is additional? Is it just omitting the hash attributes?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 83: union cbfs_mdata *src const
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 87: bit shorter than this 'This' being the size returned?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 91: offset to the file data relative or absolute within |dev| ?
Also, what is master_hash at this point? Is the assumption that all of cbfs metadata is checked prior to filling in the parameters? Or is this the hash for the metadata itself?
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h:
https://review.coreboot.org/c/coreboot/+/38421/4/src/commonlib/bsd/include/c... PS4, Line 131: struct vb2_hash hash;
-------union { ------->-------uint8_t raw[0];
#if VB2_SUPPORT_SHA1
------->-------uint8_t sha1[VB2_SHA1_DIGEST_SIZE];
#endif #if VB2_SUPPORT_SHA256
------->-------uint8_t sha256[VB2_SHA256_DIGEST_SIZE];
#endif #if VB2_SUPPORT_SHA512
------->-------uint8_t sha512[VB2_SHA512_DIGEST_SIZE];
#endif
-------} bytes;
How are we envisioning dealing with this? From your added comment it'll be dynamic as before?
The other thing that you've done here is changed the serialized format by changing hash_type from a 32-bit value to a single byte. Shouldn't we add a new type and tag so things can be backwards compatible?