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 10:
(3 comments)
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/ce24f562_d16bb0bc PS8, Line 211: config CBFS
As you suggested in CB:60080, I moved VBOOT_LIB here. […]
I mean they're still in the same menu in menuconfig (since you're including libcbfs/Kconfig right here), it just means that all the CBFS stuff would be together.
File payloads/libpayload/include/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/4949e8c7_d86203ba PS3, Line 38: {
Thanks for explaining it. I didn't look at it that way. […]
Well they are unsigned so this should work, right? Not quite sure what you mean by "before".
I think those two checks should be enough to catch problems, do you see a specific issue with dev->offset that needs to be checked separately? Specifically, I think you have to assume that dev->offset and dev->size are "valid" (i.e. any offset smaller than dev->size is a legal offset), because those are just given here from elsewhere, we don't have any further way to check them. The functions generating the cbfs_dev_t have to make sure those are correct. The reason offset and size need to be checked is that they can come from (potentially malicious) metadata in the CBFS and the CBFS device implementation needs to make sure it's not possible to read over the end of the device it defines (see also the requirements of cbfs_dev_read() documented in <commonlib/bsd/cbfs_private.h>).
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/23b08105_7f8ae88f PS10, Line 17: static struct cbfs_boot_device rw; nit: Don't really see why you moved these out of the function? I mean it works this way, but it worked the other way too. (Hiding it in there makes for a bit better isolation, because cbfs_get_boot_device() only returns a const pointer.)
-
Julius Werner (Code Review)