Philipp Deppenwiese submitted this change.

View Change

Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Julius Werner: Looks good to me, approved
security/lockdown: Write-protect WP_RO

Allow to write protect only the WP_RO region in case of enabled VBOOT.
One can either lock the boot device in VERSTAGE early if VBOOT is enabled,
or late in RAMSTAGE. Both options have their downsides as explained below.

Lock early if you don't trust the code that's stored in the writeable
flash partition. This prevents write-protecting the MRC cache, which
is written in ramstage. In case the contents of the MRC cache are
corrupted this can lead to system instability or trigger unwanted code
flows inside the firmware.

Lock late if you trust the code that's stored in the writeable
flash partition. This allows write-protecting the MRC cache, but
if a vulnerability is found in the code of the writeable partition
an attacker might be able to overwrite the whole flash as it hasn't
been locked yet.

Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Christian Walter <christian.walter@9elements.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32705
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Reviewed-by: Philipp Deppenwiese <zaolin.daisuki@gmail.com>
---
M src/include/boot_device.h
M src/security/lockdown/Kconfig
M src/security/lockdown/Makefile.inc
M src/security/lockdown/lockdown.c
M src/security/vboot/vboot_logic.c
5 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/src/include/boot_device.h b/src/include/boot_device.h
index 3146462..d5237cd4 100644
--- a/src/include/boot_device.h
+++ b/src/include/boot_device.h
@@ -65,7 +65,7 @@
/*
* Restrict read/write access to the bootmedia using platform defined rules.
*/
-#if CONFIG(BOOTMEDIA_LOCK_NONE)
+#if CONFIG(BOOTMEDIA_LOCK_NONE) || (CONFIG(BOOTMEDIA_LOCK_IN_VERSTAGE) && ENV_RAMSTAGE)
static inline void boot_device_security_lockdown(void) {}
#else
void boot_device_security_lockdown(void);
diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig
index bfdc984..30b5237 100644
--- a/src/security/lockdown/Kconfig
+++ b/src/security/lockdown/Kconfig
@@ -59,4 +59,26 @@
you won't be able to write to the whole flash chip using the
internal controller any more.

+config BOOTMEDIA_LOCK_WPRO_VBOOT_RO
+ bool "Write-protect WP_RO FMAP region in boot medium"
+ depends on VBOOT
+ help
+ Select this if you want to write-protect the WP_RO region as specified
+ in the VBOOT FMAP. You will be able to write every region outside
+ of WP_RO using the internal controller (eg. FW_MAIN_A/FW_MAIN_B).
+ In case of BOOTMEDIA_LOCK_IN_VERSTAGE the locking will take place
+ early, preventing locking of facilities used in ramstage, like the
+ MRC cache. If not using BOOTMEDIA_LOCK_IN_VERSTAGE the chipset lockdown
+ is either triggered by coreboot (when INTEL_CHIPSET_LOCKDOWN is set) or
+ has to be triggered later (e.g. by the payload or the OS).
+
endchoice
+
+config BOOTMEDIA_LOCK_IN_VERSTAGE
+ depends on BOOTMEDIA_LOCK_WPRO_VBOOT_RO
+ bool "Lock boot media down in verstage"
+ help
+ Select this if you want to write-protect the WP_RO region as soon as
+ 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.
diff --git a/src/security/lockdown/Makefile.inc b/src/security/lockdown/Makefile.inc
index a51a106..6ccc557 100644
--- a/src/security/lockdown/Makefile.inc
+++ b/src/security/lockdown/Makefile.inc
@@ -2,5 +2,11 @@
## This file is part of the coreboot project.

ifneq ($(CONFIG_BOOTMEDIA_LOCK_NONE),y)
+
+ifeq ($(CONFIG_BOOTMEDIA_LOCK_IN_VERSTAGE),y)
+verstage-y += lockdown.c
+else
ramstage-y += lockdown.c
endif
+
+endif
diff --git a/src/security/lockdown/lockdown.c b/src/security/lockdown/lockdown.c
index a8aad9b..62d0a29 100644
--- a/src/security/lockdown/lockdown.c
+++ b/src/security/lockdown/lockdown.c
@@ -5,13 +5,15 @@
#include <commonlib/region.h>
#include <console/console.h>
#include <bootstate.h>
+#include <fmap.h>

/*
* Enables read- /write protection of the bootmedia.
*/
void boot_device_security_lockdown(void)
{
- const struct region_device *rdev;
+ const struct region_device *rdev = NULL;
+ struct region_device dev;
enum bootdev_prot_type lock_type;

printk(BIOS_DEBUG, "BM-LOCKDOWN: Enabling boot media protection scheme ");
@@ -23,19 +25,32 @@
} else if (CONFIG(BOOTMEDIA_LOCK_WHOLE_NO_ACCESS)) {
printk(BIOS_DEBUG, "'no access'");
lock_type = CTRLR_RWP;
+ } else if (CONFIG(BOOTMEDIA_LOCK_WPRO_VBOOT_RO)) {
+ printk(BIOS_DEBUG, "'WP_RO only'");
+ lock_type = CTRLR_WP;
}
printk(BIOS_DEBUG, "using CTRL...\n");
} else {
if (CONFIG(BOOTMEDIA_LOCK_WHOLE_RO)) {
printk(BIOS_DEBUG, "'readonly'");
lock_type = MEDIA_WP;
+ } else if (CONFIG(BOOTMEDIA_LOCK_WPRO_VBOOT_RO)) {
+ printk(BIOS_DEBUG, "'WP_RO only'");
+ lock_type = MEDIA_WP;
}
printk(BIOS_DEBUG, "using flash chip...\n");
}

- rdev = boot_device_ro();
+ if (CONFIG(BOOTMEDIA_LOCK_WPRO_VBOOT_RO)) {
+ if (fmap_locate_area_as_rdev("WP_RO", &dev) < 0)
+ printk(BIOS_ERR, "BM-LOCKDOWN: Could not find region 'WP_RO'\n");
+ else
+ rdev = &dev;
+ } else {
+ rdev = boot_device_ro();
+ }

- if (boot_device_wp_region(rdev, lock_type) >= 0)
+ if (rdev && boot_device_wp_region(rdev, lock_type) >= 0)
printk(BIOS_INFO, "BM-LOCKDOWN: Enabled bootmedia protection\n");
else
printk(BIOS_ERR, "BM-LOCKDOWN: Failed to enable bootmedia protection\n");
diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index 8e82e40..e1c77b6 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -14,6 +14,7 @@
#include <string.h>
#include <timestamp.h>
#include <vb2_api.h>
+#include <boot_device.h>

#include "antirollback.h"

@@ -296,6 +297,10 @@

timestamp_add_now(TS_START_VBOOT);

+ /* Lockdown SPI flash controller if required */
+ if (CONFIG(BOOTMEDIA_LOCK_IN_VERSTAGE))
+ boot_device_security_lockdown();
+
/* Set up context and work buffer */
ctx = vboot_get_context();


To view, visit change 32705. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2
Gerrit-Change-Number: 32705
Gerrit-PatchSet: 17
Gerrit-Owner: Patrick Rudolph <patrick.rudolph@9elements.com>
Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org>
Gerrit-Reviewer: Christian Walter <christian.walter@9elements.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com>
Gerrit-Reviewer: Julius Werner <jwerner@chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth@google.com>
Gerrit-Reviewer: Nico Huber <nico.h@gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Arthur Heymans <arthur@aheymans.xyz>
Gerrit-CC: Benjamin Doron <benjamin.doron00@gmail.com>
Gerrit-CC: Matt DeVillier <matt.devillier@gmail.com>
Gerrit-CC: Michael Niewöhner
Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net>
Gerrit-MessageType: merged