Attention is currently required from: Tarun Tuli, Reka Norman, Kapil Porwal, Julius Werner, Arthur Heymans, Lean Sheng Tan.
Subrata Banik 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 4:
(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.
yes, your understanding is correct that we don't want to bypass the CBFS verification check
File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h:
https://review.coreboot.org/c/coreboot/+/74873/comment/52db6492_7d2fadff PS3, Line 89: 0x55434f44
Note that this spells DOCU in little endian, if you were aiming for UCOD you need to flip it.
Ack
File src/cpu/intel/microcode/microcode.c:
https://review.coreboot.org/c/coreboot/+/74873/comment/5529cdf6_5f3df87a 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.
should be avoided. done with latest patch set
https://review.coreboot.org/c/coreboot/+/74873/comment/b28ca67d_85193481 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?
Added the checksum now, hopefully this is good to avoid a scenario where `just point this at a random unused part of flash outside of CBFS and put a malicious microcode blob there`.
Please let me know your thoughts