>> I think we still need to have a difference between hacky vendor stuff >> and normal coreboot code. For example, the Eltan mboot stuff is >> something we didn't really want to have in coreboot in that form, and >> so they kinda put it in vendorcode as a compromise. We should make >> sure it remains clear that that code isn't "proper" coreboot code and >> didn't go through the same level of review. > > It might have started that way, but I don't think that's an accurate > portrayal of eltan's work at this point: > The eltan code uses vboot for the cryptographic primitives these days and as > far as I can see, extends it for measured boot - which vboot itself doesn't > do, ever.
No, that wasn't the only problem with that code. It is implementing things that we already have an official solution for and doing that in ways that aren't really safe and hinder development on core components. For example, this is the only user of prog_locate_hook(), a function that doesn't even really make sense anymore because I recently removed the prog_locate() function it was based on, and that was always a bad design to begin with because it fundamentally requires the same data to be loaded more than once to do the stuff that Eltan's mboot is trying to do with it (which is both inefficient and an opening for TOCTOU attacks). We already have a canonical verification solution (CONFIG_VBOOT) and a canonical measurement solution (CONFIG_TPM_MEASURED_BOOT), and we should be aiming to make those flexible enough to solve everyone's use case rather than have everyone implement the same thing slightly differently. It's much better to have one flexible, well-maintained solution than a dozen different, poorly-maintained solutions. Those problems I mentioned about double loading and TOCTOU safety can all be solved, and I'm aiming to do that with the CBFS verification work I'm currently doing, but it's complicated and it requires integration into CBFS internals. I only want to do and maintain that once and not once for every different vendor implementation. And doing development on this stuff is a lot easier when you don't need to maintain weird, badly designed hooks that someone once crammed in there to support a one-off solution. Eltan already signaled they were going to deprecate the mboot stuff for future platforms anyway (see https://review.coreboot.org/c/coreboot/+/38590/comment/7f384f61_597e79c9/), so I'm hoping we can eventually get rid of it (and just drag it along until then by hiding it in vendorcode and trying to maintain prog_locate_hook() with minimal effort). In the future, we will hopefully have good and flexible measurement and verification solutions ready to use (IIRC this was one of the reasons Eltan implemented the mboot stuff in the first place, because Philipp's measurement stuff wasn't quite finished back then yet), and then nobody will feel the need to implement their own thing again. > Also, regarding your point on gerrit (collecting arguments in this thread) > that we don't have duplicate things, look no further than graphics init: > - src/device/oprom/realmode > - src/device/oprom/yabel > - src/drivers/intel/gma > - FSP can do the graphics init, too (and report it in cbtables) Don't most of those target different hardware generations? Then I don't really think that's comparable... of course if we're trying to support hardware with widely different graphics interfaces that may result in different driver code. But for pure, hardware-independent feature stuff like verification or measurement, I think we should aim to converge into a single implementation that can support everyone's use cases. We don't support two different kinds of timestamp tables or two dynamic persistent memory allocators (CBMEM) either. (Also, like I mentioned the problem with this security stuff is just that it needs to be so tightly integrated with the CBFS stack. In a different, more isolated area I might consider this less of a problem, but the Eltan mboot stuff has actually been causing practical problems a couple of times already, in ways that we mostly already pointed out as concerns when it was added. I'd prefer to get rid of it long-term, not make it any more official.) _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
