Change in coreboot[master]: soc/intel/{skylake,cannonlake}: Co-ordinate lockdown configuration
Attention is currently required from: Benjamin Doron, Angel Pons, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50048 )
Change subject: soc/intel/{skylake,cannonlake}: Co-ordinate lockdown configuration ......................................................................
Patch Set 5:
(1 comment)
File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/50048/comment/749ba848_8ee12a1c PS5, Line 332: }
If we can't, why do we drag get_lockdown_config() around when only one setting is supported? (IIRC, it was already broken when added.)
How broken is it? Later platforms define the else case.
I don't know. I didn't look into it in years. I guess everybody just runs `chipsec` before releasing anything and fixes things up on local branches (or maybe master too). What I seem to remember from the review when the chipset_lockdown distinction was first implemented:
Me: This and that is broken on the CHIPSET_LOCKDOWN_FSP path. Intel: But we set CHIPSET_LOCKDOWN_COREBOOT for all boards in the tree.
See how much they care about security? I don't remember if things were fixed or if I just gave up reviewing. It was a mess and I don't know if CHIPSET_LOCKDOWN_FSP ever made sense / worked properly.
It appears to mostly only break assumptions. Based on coreboot logs, SMM is locked before FSP-S, so the only thing this patch might break is nvramtool. And perhaps the fallback mechanism, which is a big(ger) issue.
It depends on the binary. With the current state of this patch, a binary that defaults to not locking, and coreboot configured to CHIPSET_LOCKDOWN_FSP, security can be compromised. Maybe more than without this patch, I guess one needs to read all the involved code to tell.
-
Nico Huber (Code Review)