Hi Youness,
Am 26.09.18 um 01:30 schrieb Youness Alaoui:
There's a couple of things happening here : First, the FLOCKDN bit is set which prevents us from enabling the write protection. the BIOS Interface lock down is controlled by the chipset_lockdown config variable, but the FLOCKDN is not behind a config variable. The second thing is that if I wanted to use the protected ranges feature to lock specific regions, they are all getting locked using the discrete lock register even while being unused. The locking of the protected ranges was added in this change : https://review.coreboot.org/c/coreboot/+/21064 and it passed without notice among the move that the commit was supposed to do.
The commit states that the lockdown is meant to "support platform security guidelines", but I think that this is not actually good because coreboot leaves everything read-write and locks down the registers so we can't make it read-only. I think that the security guidelines would say to disable the write protection before locking the registers down.
the code is flawed, I'm not sure if I noticed during review, probably only when it was too late. It seems to me that these "security guide- lines" have two issues: 1. They are written with UEFI style security in mind (e.g. updates in SMM). 2. They are meant to be understandable for a humble reader and hence can't comprise all the details (like threat models etc.) for real security. They seem to be for getting-a-green-light-in-CHIPSEC security.
Not sure how CHIPSEC would react if the individual PRR locks aren't set. But feel free to remove the locking of unset PR registers on any path where we don't set FLOCKDN.
Either way, I'm going to need to add a way to make this configurable, so my main questions here are :
- Should I create a new config variable to decide on whether or not to
lock the spi registers and another one to decide on whether or not to lock the protected ranges ?
I would prefer to have a single Kconfig option like INTEL_CHIPSET_ LOCKDOWN (sb/intel/common) but for FLOCKDN only. This option for the whole chipset lockdown on older platforms only made sense because the lockdown was done in SMM to allow the payload to call into coreboot. As SMM is not a good idea and reimplementing the whole lockdown in the pay- load isn't either, just skipping flash locking bits seems to be the right way.
Please take care that this only affects a clean boot, i.e. not a resume when the payload is not going to be executed.
- Should I make the chipset_lockdown (currently used for locking the
BIOS CNTL register from LPC and SPI controllers) into an OR-ed flags variable where we can say : chipset_lockdown = LOCKDOWN_COREBOOT | LOCKDOWN_SPI | LOCKDOWN_PROTECTED_RANGES ?
The current `chipset_lockdown` option is nonsense. 1. during review when I noticed a flaw in the LOCKDOWN_FSP path, the answer was something like "but we set it to coreboot for all boards in the tree". So it seems the author didn't even intend to add an option but just did it because eve- rything else was an option? 2. Having to maintain two variants in core- boot makes it harder to keep it secure. Please just kill this option and all but the LOCKDOWN_COREBOOT path. Then add the Kconfig option men-tioned above on this path.
- Should I make a single new config variable to decide what to
lockdown (LPC_BIOS, SPI_BIOS, SPI_BAR, SPI_PROTECTED_RANGES) and only set them if the CHIPSET_LOCKDOWN_COREBOOT is set ? And if chipset_lockdown is set to CHIPSET_LOCKDOWN_FSP not lockdown anything at all ?
- Do we want to keep the protected ranges locked down at all, have
them configurable or completely remove that as I don't see the point in using the discrete lock register ?
I would prefer to have the PR registers locked right after they are set, in the same function. Also remove fast_spi_pr_dlock() or at least fix the comment above it, it seems the author didn't understand the meaning of "discrete" in this context. Optionally, if CHIPSEC complains other- wise, set the discrete locks for empty PRRs before setting FLOCKDN. AIUI, FLOCKDN always locks the PRRs.
Once I see a consensus on what's the best way to move forward, I'll implement it and push it for review.
Thank you for looking into this it would be very nice to have this cleaned up. I guess if we do it right, we'll have less code that is more useful.
Note: I think these only affect hardware sequencing though so I assume someone could always use software sequencing to do the writes. As long as the FLOCKDN bit isn't set though, I could remove all write-related opcodes from the software sequencing register, which would also prevent someone using swseq to do writes.
In the past, we (at secunet) had FLOCKDN always set in the payload just like you plan to do. We also made it the payload's responsibility to set the OPMENU accordingly before setting FLOCKDN. You can have write op- codes in the menu, they won't work on protected regions. But if you have a partially protected flash (e.g. vboot), they are still useful.
Also note that swseq is not an option anymore since Skylake (IIRC, the ME protects the flash-cycle go bit in its default configuration). With only hwseq, you can only access the flash chip's first status register which makes many security features unusable. So we have to rely on the protected regions :-/ I hope Intel is going to fix that for future plat- forms (or with an ME update if that could add more hwseq commands).
Nico
-
Nico Huber