Hello Daniel Gröber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41747
to review the following change.
Change subject: lockdown: Add Kconfigs for SPI media protection mode ......................................................................
lockdown: Add Kconfigs for SPI media protection mode
SPI_WRITE_PROTECTION_REBOOT seems to be a Winbond thing, other vendors such as Macronix only support permanent protection but conditional on the WP# pin state.
Change-Id: Iba7c1229c82c86e1303d74c7bc8f89662b5bb58c Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/boot_device_rw_nommap.c M src/security/lockdown/Kconfig 2 files changed, 37 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/41747/1
diff --git a/src/drivers/spi/boot_device_rw_nommap.c b/src/drivers/spi/boot_device_rw_nommap.c index ba11d05..1d68285 100644 --- a/src/drivers/spi/boot_device_rw_nommap.c +++ b/src/drivers/spi/boot_device_rw_nommap.c @@ -96,9 +96,16 @@ if (type == MEDIA_WP) { if (spi_flash_is_write_protected(boot_dev, region_device_region(rd)) != 1) { + enum spi_flash_status_reg_lockdown lock; + if (CONFIG(BOOTMEDIA_SPI_LOCK_REBOOT)) + lock = SPI_WRITE_PROTECTION_REBOOT; + else if (CONFIG(BOOTMEDIA_SPI_LOCK_PIN)) + lock = SPI_WRITE_PROTECTION_PIN; + else if (CONFIG(BOOTMEDIA_SPI_LOCK_PERMANENT)) + lock = SPI_WRITE_PROTECTION_PERMANENT; + return spi_flash_set_write_protected(boot_dev, - region_device_region(rd), - SPI_WRITE_PROTECTION_REBOOT); + region_device_region(rd), lock); }
/* Already write protected */ diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig index 30b5237..97094ff 100644 --- a/src/security/lockdown/Kconfig +++ b/src/security/lockdown/Kconfig @@ -82,3 +82,31 @@ possible. This option prevents using write protecting facilities in ramstage, like the MRC cache for example. Use this option if you don't trust code running after verstage. + +choice + prompt "SPI Flash write protection duration" + default BOOTMEDIA_SPI_LOCK_REBOOT + depends on BOOTMEDIA_LOCK_CHIP + depends on BOOT_DEVICE_SPI_FLASH + +config BOOTMEDIA_SPI_LOCK_REBOOT + bool "Lock SPI flash until next reboot" + help + The SPI chip is locked until power is removed and re-applied. + Supported by Winbond parts. + +config BOOTMEDIA_SPI_LOCK_PIN + bool "Lock SPI flash using WP# pin" + help + The SPI chip is locked using a non-volatile configuration bit. Writes + are only possible if the WP# is not asserted. Supported by Winbond + and Macronix parts. + +config BOOTMEDIA_SPI_LOCK_PERMANENT + bool "Lock SPI flash permanently" + help + The SPI chip is permanently locked using a non-volatile configuration + bit. No writes are ever possible again after we perform the lock. + Supported by Winbond parts. + +endchoice
Hello build bot (Jenkins), Daniel Gröber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41747
to look at the new patch set (#3).
Change subject: lockdown: Add Kconfigs for SPI media protection mode ......................................................................
lockdown: Add Kconfigs for SPI media protection mode
SPI_WRITE_PROTECTION_REBOOT seems to be a Winbond thing, other vendors such as Macronix only support permanent protection but conditional on the WP# pin state.
Change-Id: Iba7c1229c82c86e1303d74c7bc8f89662b5bb58c Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/boot_device_rw_nommap.c M src/security/lockdown/Kconfig 2 files changed, 38 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/41747/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41747 )
Change subject: lockdown: Add Kconfigs for SPI media protection mode ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41747/5/src/drivers/spi/boot_device... File src/drivers/spi/boot_device_rw_nommap.c:
https://review.coreboot.org/c/coreboot/+/41747/5/src/drivers/spi/boot_device... PS5, Line 105: else if (CONFIG(BOOTMEDIA_SPI_LOCK_PERMANENT)) : lock = SPI_WRITE_PROTECTION_PERMANENT; Is this something the firmware (as opposed to the OS, ensuring the system actually boots) should ever do?
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41747 )
Change subject: lockdown: Add Kconfigs for SPI media protection mode ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41747/5/src/drivers/spi/boot_device... File src/drivers/spi/boot_device_rw_nommap.c:
https://review.coreboot.org/c/coreboot/+/41747/5/src/drivers/spi/boot_device... PS5, Line 105: else if (CONFIG(BOOTMEDIA_SPI_LOCK_PERMANENT)) : lock = SPI_WRITE_PROTECTION_PERMANENT;
Is this something the firmware (as opposed to the OS, ensuring the system actually boots) should eve […]
I really only added it since the `SPI_WRITE_PROTECTION_PERMANENT` flag already existed, so I figured why not add them all as options.
IMO it wouldn't be entirely unreasonable to want the firmware to lock itself on first boot. I don't think the OS should be involved in that honestly. If you really want that level of "security" why not trigger it right in the firmware. Though you could always unsolder the SPI chip, so what additional level of security this actually buys over just _PIN is questionable but why not give users the option?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41747 )
Change subject: lockdown: Add Kconfigs for SPI media protection mode ......................................................................
Patch Set 6: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41747 )
Change subject: lockdown: Add Kconfigs for SPI media protection mode ......................................................................
Patch Set 7: Code-Review+1
-
Arthur Heymans (Code Review) -
Daniel Gröber (dxld) (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)