[coreboot] SPI controller and Lock bits

[Patrick Rudolph]

Hi Youness,
On 2018-09-26 01:30 AM, Youness Alaoui wrote:
> Hi,
> 
> I'm trying to add a way to lock the SPI flash to be read-only via
> software *after* coreboot boots. The scenario is basically with using
> Heads, you could authenticate to it (with a yubikey/nitrokey/librem
> key) then be able to flash a new rom (update your BIOS), but once you
> boot an OS, Heads would first lock the flash so it can't be written
> to.
> This should add some security to avoid any malware writing to the
> flash, or someone booting into a USB stick and using that to flash a
> malicious BIOS, but still gives the user the freedom of updating their
> flash whenever they want to.
> 
> The problem is that I can't make the flash read-only because the SPI
> interface is already locked down by coreboot (see
> src/soc/intel/skylake/lockdown.c and
> src/soc/intel/common/block/fast_spi/fast_spi.c).
> 
> 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.
> 
Feel free to propose a new "security guideline", but document it in the
tree.

A similar mechanism is already implemented on Intel:
https://review.coreboot.org/#/c/coreboot/+/21327/

Feel free to make it configurable on intel/soc, too.

> 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 ?
> - 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 ?
> - 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 ?
> 

The protected ranges will be used on non Chromeos devices to support
vboot. A common interface is being worked on right now, but it'll take
some time.

> Once I see a consensus on what's the best way to move forward, I'll
> implement it and push it for review.
> 
> 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.
> 
> Thanks,
> Youness.