Attention is currently required from: Jakub Czapiga, Paul Menzel. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59497 )
Change subject: libpayload: Implement new CBFS access API ......................................................................
Patch Set 5:
(17 comments)
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/0f2979c5_6efe0c17 PS5, Line 15: /* For documentation look in the main coreboot cbfs header files in paths: I think you can just refer to <cbfs.h> directly, that's where the main API is declared.
Also, multi-line comment style is only
/* * Like * this */
or
/* Like this. */
https://review.coreboot.org/c/coreboot/+/59497/comment/54530485_60c879a4 PS5, Line 28: order where possible, since mapping backends often don't support more complicated cases */ The second sentence is not true for libpayload.
https://review.coreboot.org/c/coreboot/+/59497/comment/a8d5680d_29ff3a06 PS5, Line 35: /* Defined in include/cbfs_glue.h */ I think it would be fine to just include <cbfs_glue.h> here, too.
https://review.coreboot.org/c/coreboot/+/59497/comment/aab0d07b_206d22e3 PS5, Line 43: ssize_t _cbfs_boot_lookup(const char *name, bool force_ro, union cbfs_mdata *mdata); You only need this if you also duplicate the cbfs_get_type()/cbfs_get_size()/cbfs_file_exists() APIs.
https://review.coreboot.org/c/coreboot/+/59497/comment/45c6e9ab_57558a71 PS5, Line 63: void *ret = _cbfs_load(name, buf, &size, false); nit: why not just
if (_cbfs_load(...))
?
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/944280f9_f3a54bbd PS5, Line 87: #define CBFS_ATTRIBUTE_ALIGN 4 Not sure why some of this needs to be added here? Since this file is supposed to be deleted anyway, I don't think you should change anything more than necessary to keep things compiling.
File payloads/libpayload/include/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/3dd99319_f7125726 PS3, Line 38: {
Done. I accounted for the dev->offset too, as it might better narrow the range.
Oh right, thanks for paying attention!
https://review.coreboot.org/c/coreboot/+/59497/comment/142353b6_f021de0e PS3, Line 38: {
Done. I accounted for the dev->offset too, as it might better narrow the range.
No, sorry, it actually has to be (offset + size < offset). The point is to check whether the (offset + size) calculation can overflow (because if it can, it might defeat the offset + size > dev->size check). If you're calling this function with offset = 0xfffff000 and size = 0x10f00 on a 32-bit system with a boot device where dev->offset = 0x1000 and dev->size = 0x10000, then offset + size = 0xff00 which is smaller than 0x10000. But we'd end up calling boot_device_read(buffer, 0, 0x10f00) which is reading the wrong region.
Your check would be 0x1000 + 0xfffff000 + 0x10f00 < 0x1000 + 0xfffff000, which is false. My version would be 0xfffff000 + 0x10f00 < 0xfffff000, which catches the issue.
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/94f07fc2_383cd7b8 PS3, Line 60: void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, bool is_ro);
cbfs_boot_device_find_mcache() can be static, yes. […]
Hmm, well... same comment as on the FMAP CL, honestly, I don't think we should make changes to firmware code just to enable testing. If you really think we need to be able to mock static functions, we should introduce some macro like STATIC_TESTABLE that resolves to nothing when building for __TEST__ and to `static` when not. In this case, though, since cbfs_get_boot_device() is a pretty simple function that just returns lib_sysinfo values, can't you just mock it that way?
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/7f6d339c_7e21a049 PS5, Line 10: #define CB_CBFS_CORE_WITH_LZ4 Sorry, I meant these things too when I said "get rid of the cruft". You can just always include <lzma.h> and <lz4.h>, you don't need to guard that. and You can use CONFIG(LP_LZMA) and CONFIG(LP_LZ4) directly in the code, you don't need to define other names for them.
https://review.coreboot.org/c/coreboot/+/59497/comment/6984af9b_87759e9e PS5, Line 23: #ifndef __SMM__ This doesn't belong here either.
https://review.coreboot.org/c/coreboot/+/59497/comment/730a681b_d730662a PS5, Line 34: if (!rw.mcache_size) { I don't think there's a point checking this here?
https://review.coreboot.org/c/coreboot/+/59497/comment/ac71c71c_a2858a76 PS5, Line 45: !lib_sysinfo.fmap_offset Checking this here shouldn't be needed?
https://review.coreboot.org/c/coreboot/+/59497/comment/991f30df_0a4a17b4 PS5, Line 49: if (!ro.mcache_size) { Don't think you need to check this either.
https://review.coreboot.org/c/coreboot/+/59497/comment/aa402016_8e1aa830 PS5, Line 170: *size_inout = MIN(size, *size_inout); You shouldn't need the MIN() here... if the buffer is too small, we need to fail anyway. Also, *size_inout *must* be passed when buf is non-null. So just do
if (!size_inout || *size_inout < size) return NULL;
https://review.coreboot.org/c/coreboot/+/59497/comment/df831081_c6ef1d7f PS5, Line 172: loc = malloc(size); nit: rather than introducing `loc` I think you can just do buf = malloc(...) here and keep using that.
https://review.coreboot.org/c/coreboot/+/59497/comment/772cd4db_ecd3e59c PS5, Line 182: if (cbfs_load_and_decompress(offset, size, loc, size, compression, NULL) != size) { Be careful with the size parameters here. The first size should be the *compressed* file size in CBFS (i.e. be32toh(mdata.h.len)), the second one should be the size of the allocated buffer (I guess you can use `size` here for simplicity, even though that might be smaller, since we know it's gonna be enough anyway.
-
Julius Werner (Code Review)