Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39304 )
Change subject: cbfs: Simplify load/map API names, remove type arguments ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39304/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39304/4//COMMIT_MSG@24 PS4, Line 24: known Some of those are only known by way of Kconfig check. I also think allowing leaking all over because the API is not clear of the semantics allows for abuse. The omission of stranded cbfs_map() calls w/o a cbfs_unmap() pairing will likely be copy/pasted which leads to truly leaking resources.
https://review.coreboot.org/c/coreboot/+/39304/4/src/include/cbfs.h File src/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/39304/4/src/include/cbfs.h@27 PS4, Line 27: often "can't". Without knowing which rdev the mapping is connected to then things fall over. Remember you have an ro as well as rw cbfs_boot_device objects.
https://review.coreboot.org/c/coreboot/+/39304/4/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/39304/4/src/lib/cbfs.c@96 PS4, Line 96: cares about which chained subregion something was mapped from. */ I don't think this is always true in practice. I think more commentary covering the diligence of this assumption is warranted.
e.g. src/drivers/spi/boot_device_rw_nommap.c provides rdev ops which do not allow any mmap()ing, but boot_device_ro() using src/arch/x86/mmap_boot.c does allow it. I believe that's the only combination that is potentially problematic, but the assumption above still holds.
https://review.coreboot.org/c/coreboot/+/39304/4/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/39304/4/src/security/vboot/ec_sync.... PS4, Line 397: Why wouldn't we unamp here?