Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c@28 PS10, Line 28: if (err == CB_CBFS_CACHE_FULL)
- The whole point of the mcache is *only* to be a performance improvement, *nothing* else. None of the security guarantees provided by this design fundamentally rely on the mcache in any way. The system is just as secure when CONFIG_NO_CBFS_MCACHE is enabled, and lookups are just as secure when they fall back to flash because the RW mcache is full. The only thing you pay is extra time for reloading or reverifying metadata again. (CONFIG_TOCTOU_SAFETY currently depends on the mcache in Kconfig, and requires the RO mcache to be big enough to fit all files. That is a practical simplification for me, not a fundamental requirement. I could work around that requirement but to do that I would have to introduce a separate memlayout section to pass the RO metadata hash anchor forward to later stages. I don't expect there to really be a practical need for that specific configuration to work, so I decided to simplify things and save some implementation/maintenance effort by making the RO mcache a requirement for CONFIG_TOCTOU_SAFETY. This is *only* the RO mcache, you can set the RW mcache size to zero and still have a perfectly secure, perfectly TOCTOU-safe system that simply reverifies all the metadata on every RW file access.)
It was not clear to me at all that we were incrementally only doing this for RO verification. Yes, I agree if you don't want to maintain the bits necessary for closing known-but-closeable TOCTOU windows then you don't need to worry about the things you mentioned above. To me, it seemed we have that ability, but we're choosing not to which I don't understand why. I get that one can argue that the first window is not closed so don't close any others, but the building blocks are there -- especially if one wanted to use the chipset proprietary implementations to anchor root of trust as well. It would work without much more effort. But we're purposefully not going down that path.
Sorry, I really don't see the relation between what I wrote and what you wrote here, I don't get the impression you understood what I was trying to say. Again, I am *not leaving any gaps, holes or known-but-closeable windows open* in my design. If you enable the CONFIG_TOCTOU_SAFETY option in Kconfig, the design *will be TOCTOU-safe*! Period. This is not a temporary incremental RO-only thing, this will hold true for the final design for both the RO and RW CBFS. If you verify the anchor in a TOCTOU-safe way (e.g. using fused SoC hashing) and you run this on a non-XIP platform, you will have an entirely TOCTOU-safe boot flow start to end, RO and RW, no other open windows remaining. Even when accessing a file that did not fit in the mcache, this implementation will do so in an entirely TOCTOU-safe manner. The mcache overflow issue does *not* threaten TOCTOU-safety or any other security guarantee in *any* way. I keep saying that but I don't get the impression you hear me to be honest.
I don't understand the complexity assertion nor backwards compatibility. It's re-steering to another cbfs in time assuming a linear bootflow. That fits just fine in the existing code by updating rdev of the cbfs in the cbfs boot device thing. Anyway it'll need to be addressed eventually, and all this code can definitely be used for whatever solution needed in the future.
What you're proposing requires every platform to carefully separate its CBFS files based on what stage they are needed in. Some files are needed in both pre-RAM and post-RAM stages, so then what -- either you have to store them twice or you have to provide extra APIs to allow one side to access the CBFS of the other, and place the cognitive burden of figuring out what to use where onto each platform maintainer. It also requires a bunch extra plumbing to set up yet another mcache instance and reserve space for that in memlayout and...
I disagree that this "needs to be addressed eventually", because my solution doesn't need this, not now and not in the future. My solution can solve this problem and still provide all the same security guarantees you want without adding any of this stuff. That is why I think it is better.