Attention is currently required from: Tarun Tuli, Subrata Banik, Reka Norman, Kapil Porwal, Arthur Heymans, Lean Sheng Tan.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74873 )
Change subject: cpu/intel/microcode: Implement microcode info caching inside cbmem ......................................................................
Patch Set 3:
(4 comments)
Patchset:
PS3: I assume this patch is in response to b/242473942? And your time savings aren't really from "searching", they're from bypassing CBFS verification?
If we determined that we actually *want* to bypass CBFS verification, and that it is safe to do so, there are more straight-forward ways to do that (e.g. cbfs_unverified_area_map()). But my understanding from the previous discussion on the bug was that we don't actually want to do that because there could actually be some attack vector here (even if it's e.g. just a rollback attack). In that case, we obviously also can't do something like this.
File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h:
https://review.coreboot.org/c/coreboot/+/74873/comment/d3ae97bd_7ef46194 PS3, Line 89: 0x55434f44 Note that this spells DOCU in little endian, if you were aiming for UCOD you need to flip it.
File src/cpu/intel/microcode/microcode.c:
https://review.coreboot.org/c/coreboot/+/74873/comment/21179ead_1325a274 PS3, Line 295: if (ENV_CREATES_CBMEM) Why this? If you want this to only execute in RAMSTAGE, I think just wrapping the whole thing in `#if ENV_RAMSTAGE` would be clearer.
https://review.coreboot.org/c/coreboot/+/74873/comment/23219a91_b10bccde PS3, Line 305: info->microcode_ptr = (uintptr_t)find_cbfs_microcode(); Is this just stashing a pointer to MMIO flash across reboots? That sounds like a bad idea. What if you get a firmware update in between where CBFS files can randomly move around?
Besides, what is the security story for this feature? CBMEM cannot be trusted across reboots, it could have been manipulated by any attacker trying to gain persistence (e.g. just point this at a random unused part of flash outside of CBFS and put a malicious microcode blob there). Is there some separate verification check for this microcode later on that we trust?