15 comments:
e
Done
File src/commonlib/bsd/cbfs_private.c:
Patch Set #11, 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 funct […]
It doesn't, actually, both cbfs_dev_size() and everything called by it are static inlines, so the compiler can fold the whole thing (including this function) and should be able to hoist it out of the loop if that's an improvement.
Patch Set #11, Line 52: be32toh(mdata.h.type)
You already have local variable, 'type', for this.
Done
Patch Set #11, Line 70: goto next_file;
I can see where this might be the correct action for certain circumstances. […]
I'm just trying to continue as far as possible here. I think that's probably the best philosophy for the classic, basic coreboot user who has no recovery mode and might accidentally incur some flash corruption somehow. Of course the chance that this particular check fails but the rest is good enough to boot is still tiny, so it doesn't really matter much either way.
Note that when hashing is enabled, this skips the digest_extend() for this file header which guarantees it will result in a hash failure at the end.
Patch Set #11, 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 e […]
Line 53 guarantees sizeof(mdata) >= data_offset
Then either:
Line 65 makes todo = data_offset - sizeof(mdata.h), or
Line 67 makes todo = attr_offset - sizeof(mdata.h) and line 68 guarantees attr_offset <= data_offset
Therefore todo <= data_offset - sizeof(mdata.h), which means we know we can fit todo bytes behind mdata.raw + sizeof(mdata.h).
Patch Set #11, Line 117: const size_t todo = be32toh(src->h.offset) - already_read;
How do we know that: […]
cbfs_copy_fill_metadata() is only meant to be called from a |walker| callback to cbfs_walk(), where |src| and |already_read| must be the same that cbfs_walk() passed to |walker| (see docstrings in cbfs_private.h... let me know if you think something needs to be more explicit). So already_read is (sizeof(mdata.h) + todo) from cbfs_walk(), and cbfs_walk() guarantees that (todo <= data_offset - sizeof(mdata.h)) (see above) and that data_offset <= sizeof(union cbfs_mdata). data_offset in cbfs_walk() is be32toh(src->h.offset) here.
Patch Set #11, 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 […]
No, both of these checks together form the filename check. |already_read| is the amount of valid bytes in |mdata| (and we know that this should include the full filename for a well-formed CBFS header), so
args->namesize > already_read - offsetof(union cbfs_mdata, filename)
checks if the name we're looking for is so large that it doesn't even fit in the header we have -- in that case we can fastpath out to NOT_FOUND. If it does fit, we can do
memcmp(args->name, mdata->filename, args->namesize)
to check if the name actually matches. We know that there's args->namesize bytes in args->name because that's where that number comes from, and we know that there's that many bytes in mdata->filename because that's what the first check was about. So we cannot run over the end of a buffer. We _can_ technically run over the filename in the CBFS header and into the attribute part (if that's part of |already_read|), but I don't think that's a problem... if the filename is malformed (not null-terminated) the header is garbage anyway, then attr_offset might have been garbage too.
Added a comment to clarify what this is checking and why.
Patch Set #11, 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 b […]
See above.
File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
Patch Set #11, Line 4: PRIVATE
Or are these considered to be implementation details?
It's supposed to be APIs private to the CBFS stack that shouldn't be called directly by any other parts of coreboot. I could also call it <cbfs_internal.h> or maybe <cbfs_unsafe.h> if you prefer?
The core issue here is how to make sure people don't accidentally break verification by using APIs in a way they weren't intended. The per-file verification approach absolutely requires that every file accessed (whether it's a stage, an SPD blob, an FSP image, etc.) is actually hashed and verified, or your whole verification system could be insecure because of one small mistake buried deep in src/soc/xxx code. I'm really trying to come up with the best way to design the code so that this can't happen by accident.
The basic idea is that all other coreboot code accessing CBFS will call high level functions like cbfs_read() or cbfs_map() (see later patches) which are implemented in src/lib/cbfs.c and will automatically hash and verify the file contents if CONFIG(CBFS_VERIFICATION) is on. src/lib/cbfs.c should be the only file that actually includes <cbfs_private.h> and calls the low-level APIs implemented here. That way we only have to monitor and carefully review changes to one file.
I haven't finished porting all use cases to the new API yet so I'm not sure how ultimately feasible this will be, but that's the rough goal. Some of them will definitely be tricky (e.g. that idea of streaming decompression for pre-RAM EC updates) so I'll have to come up with new high-level APIs to properly encapsulate something like that. Of course, the other issue is that there's no easy way to prevent someone from including the low-level header anyway. I've thought about maybe also adding something like
#if CONFIG(CBFS_VERIFICATION) &&
!defined(THIS_FILE_WAS_SECURITY_REVIEWED_FOR_DIRECT_LOW_LEVEL_CBFS_ACCESS)
#error "Please don't access low-level CBFS APIs directly."
#endif
so that there's at least one extra conscious step you need to take when including this to clarify that you're really aware of the risks involved. I've also considered creating another Kconfig like CONFIG(CBFS_VERIFICATION_STRICT) to block certain APIs and/or enable the extra check above, so it wouldn't stop everyone from playing with CBFS stuff but it would alert extra paranoid users (like us) that some dangerous API use might have not been fully reviewed.
I'm not really sure what the best answers here are yet, let me know what you think!
Perhaps call out the name (cbfs_glue. […]
Done
Patch Set #11, 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.
Done. (This is just about how the code can be statically configured anyway, the exact explanation of when what is hashed is in the docstrings for cbfs_walk()/cbfs_lookup() below.)
Patch Set #11, Line 49: empty entries
What exactly does this mean? cbfs_file w/ no data, only metadata?
It's about entries with the DELETED type (i.e. free space). Clarified comment.
Patch Set #11, 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 […]
Is it illegal / frowned upon to use an enum type for values that are ORed together from valid enum values? I wasn't aware. I think using the enum type serves as a little extra self-documentation, but I can make it unsigned int if you prefer. (Note that GCC always makes enum types that don't have negative members unsigned, so I don't think there could be a signedness concern.)
Patch Set #11, 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, […]
Yes... I've thought about this for a bit and my take is that it's less confusing to just always leave the data big endian while it's in the header struct. If you try to rewrite the header in memory to convert stuff to CPU byte order I think you just risk introducing a ton of confusion about at what point it's still big-endian and when it isn't. It's easier to say "everything in a union cbfs_mdata is always big-endian".
I've added a comment to the structure definition above to clarify.
Patch Set #11, 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. […]
Yes, I always use the term "master hash" only for the hash over the metadata of the whole CBFS (the others are "file hashes"... don't have much code related to them yet). With the whole inclusive language thing I wanted to rename it everywhere to metadata_hash anyway, that should be a bit clearer too. (And yes, when master_hash is provided it always needs to read the full metadata for every lookup, that's a core constraint of this design.)
The reason it's not const is just API weirdness, basically. The master_hash argument to cbfs_walk() cannot be const because if CBFS_WALK_WRITEBACK_HASH is set it will get modified. So since cbfs_lookup() calls cbfs_walk() it was easiest to not make it const here as well. I could also make it const here and then cast the const away inside the function if you prefer?
To view, visit change 38421. To unsubscribe, or for help writing mail filters, visit settings.