Change in coreboot[master]: soc/intel/{skylake,cannonlake}: Co-ordinate lockdown configuration
Attention is currently required from: Nico Huber, Angel Pons, Patrick Rudolph. Benjamin Doron 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/7156ee0d_d8894eeb PS5, Line 332: }
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.
Perhaps the chipset is fairly well locked-down, but currently, coreboot has no SPI protection. In that regard, CHIPSET_LOCKDOWN_FSP could be superior.
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.
I suppose that could be the case in some instances, and it definitely would be a concern. I will define the else case and set CHIPSET_LOCKDOWN_COREBOOT for the two boards I mentioned. Then, perhaps CHIPSET_LOCKDOWN_FSP should be dropped entirely.
IMHO, it's easier to drop support for FSP lockdown.
Has anybody ever expressed interest in it?
-
Benjamin Doron (Code Review)