[coreboot] SPI controller and Lock bits

[Nico Huber]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0xBD56B4A4138B3CE3.asc
Type: application/pgp-keys
Size: 5227 bytes
Desc: not available
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20180926/cb3045a2/attachment.skr>