Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32704
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/bootmedia.c M src/soc/intel/common/block/fast_spi/Kconfig M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 124 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/1
diff --git a/src/security/Kconfig b/src/security/Kconfig index 6a334ac..99cb054 100644 --- a/src/security/Kconfig +++ b/src/security/Kconfig @@ -14,3 +14,4 @@
source "src/security/vboot/Kconfig" source "src/security/tpm/Kconfig" +source "src/security/lockdown/Kconfig" diff --git a/src/security/Makefile.inc b/src/security/Makefile.inc index a940b82..a74e498 100644 --- a/src/security/Makefile.inc +++ b/src/security/Makefile.inc @@ -1,2 +1,3 @@ subdirs-y += vboot subdirs-y += tpm +subdirs-y += lockdown diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig new file mode 100644 index 0000000..bb4d072 --- /dev/null +++ b/src/security/lockdown/Kconfig @@ -0,0 +1,46 @@ + +config SECURITY_BOOTMEDIA_LOCKDOWN + bool + default n + help + Platform support the locking of boot media. This can be for example + SPI controller protected regions or flash status register locking. + +if SECURITY_BOOTMEDIA_LOCKDOWN + +choice + prompt "Boot media protection" + default BOOTMEDIA_LOCK_NONE + +config BOOTMEDIA_LOCK_NONE + bool "Don't lock boot media sections" + +config BOOTMEDIA_LOCK_RO + bool "Write-protect the whole boot media" + help + Select this if you want to write-protect the whole firmware boot + media. The locking will take place during the chipset lockdown, which + 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). + + NOTE: If you trigger the chipset lockdown unconditionally, + you won't be able to write to the flash chip using the + internal programmer any more. + +config BOOTMEDIA_LOCK_NO_ACCESS + bool "Read- and write-protect the whole boot media" + help + Select this if you want to protect the firmware boot media against + all further accesses. On platforms that memory map a part of the + boot media the corresponding region is still readable. + The locking will take place during the chipset lockdown, which 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). + + NOTE: If you trigger the chipset lockdown unconditionally, + you won't be able to write to the boot media using the + internal programmer any more. + +endchoice + +endif diff --git a/src/security/lockdown/Makefile.inc b/src/security/lockdown/Makefile.inc new file mode 100644 index 0000000..c287b9b --- /dev/null +++ b/src/security/lockdown/Makefile.inc @@ -0,0 +1,16 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2019 9elements Agency GmbH patrick.rudolph@9elements.com +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +ramstage-$(CONFIG_SECURITY_BOOTMEDIA_LOCKDOWN) += bootmedia.c diff --git a/src/security/lockdown/bootmedia.c b/src/security/lockdown/bootmedia.c new file mode 100644 index 0000000..8fb4ae9 --- /dev/null +++ b/src/security/lockdown/bootmedia.c @@ -0,0 +1,58 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 9elements Agency GmbH patrick.rudolph@9elements.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <boot_device.h> +#include <commonlib/region.h> +#include <console/console.h> +#include <bootstate.h> + +/* + * Enable write protection on the WP_RO region of the bootmedia. + */ +static void security_lockdown_bootmedia(void *unused) +{ + static const int wp_prot[] = {MEDIA_WP, CTRLR_WP}; + const struct region_device *rdev; + bool locked = false; + + if (CONFIG(BOOTMEDIA_LOCK_RO)) { + rdev = boot_device_ro(); + + for (size_t i = 0; i < ARRAY_SIZE(wp_prot); i++) { + printk(BIOS_DEBUG, "BM-LOCKDOWN: Trying write-protection" + "#%zu ...\n", i); + if (boot_device_wp_region(rdev, wp_prot[i]) < 0) + continue; + + printk(BIOS_INFO, "BM-LOCKDOWN: Enabled write-protection of" + "whole bootmedia\n"); + locked = true; + } + } else if (CONFIG(BOOTMEDIA_LOCK_NO_ACCESS)) { + rdev = boot_device_ro(); + if (boot_device_wp_region(rdev, CTRLR_RWP) == 0) { + printk(BIOS_INFO, "BM-LOCKDOWN: Enabled read- and write protection" + "of whole bootmedia\n"); + locked = true; + } + } + + if (!locked) + printk(BIOS_INFO, "BM-LOCKDOWN: Didn't enable bootmedia protection\n"); +} + +/* BS_POST_DEVICE will lock the hardware */ +BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, security_lockdown_bootmedia, + NULL); diff --git a/src/soc/intel/common/block/fast_spi/Kconfig b/src/soc/intel/common/block/fast_spi/Kconfig index 4bd1f59..ff02844 100644 --- a/src/soc/intel/common/block/fast_spi/Kconfig +++ b/src/soc/intel/common/block/fast_spi/Kconfig @@ -1,5 +1,6 @@ config SOC_INTEL_COMMON_BLOCK_FAST_SPI bool + select SECURITY_BOOTMEDIA_LOCKDOWN help Intel Processor common FAST_SPI support
diff --git a/src/southbridge/intel/common/Kconfig b/src/southbridge/intel/common/Kconfig index c3bd90d..39234a8 100644 --- a/src/southbridge/intel/common/Kconfig +++ b/src/southbridge/intel/common/Kconfig @@ -20,6 +20,7 @@ config SOUTHBRIDGE_INTEL_COMMON_SPI def_bool n select SPI_FLASH + select SECURITY_BOOTMEDIA_LOCKDOWN
config SOUTHBRIDGE_INTEL_COMMON_PIRQ_ACPI_GEN def_bool n @@ -68,42 +69,3 @@ config SOUTHBRIDGE_INTEL_COMMON_WATCHDOG bool depends on SOUTHBRIDGE_INTEL_COMMON - -if SOUTHBRIDGE_INTEL_COMMON_FINALIZE - -choice - prompt "Flash locking during chipset lockdown" - default LOCK_SPI_FLASH_NONE - -config LOCK_SPI_FLASH_NONE - bool "Don't lock flash sections" - -config LOCK_SPI_FLASH_RO - bool "Write-protect all flash sections" - help - Select this if you want to write-protect the whole firmware flash - chip. The locking will take place during the chipset lockdown, which - 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). - - NOTE: If you trigger the chipset lockdown unconditionally, - you won't be able to write to the flash chip using the - internal programmer any more. - -config LOCK_SPI_FLASH_NO_ACCESS - bool "Write-protect all flash sections and read-protect non-BIOS sections" - help - Select this if you want to protect the firmware flash against all - further accesses (with the exception of the memory mapped BIOS re- - gion which is always readable). The locking will take place during - the chipset lockdown, which 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). - - NOTE: If you trigger the chipset lockdown unconditionally, - you won't be able to write to the flash chip using the - internal programmer any more. - -endchoice - -endif diff --git a/src/southbridge/intel/common/finalize.c b/src/southbridge/intel/common/finalize.c index 80c65bb..6f7934a 100644 --- a/src/southbridge/intel/common/finalize.c +++ b/src/southbridge/intel/common/finalize.c @@ -28,16 +28,6 @@ { const pci_devfn_t lpc_dev = PCI_DEV(0, 0x1f, 0);
- if (CONFIG(LOCK_SPI_FLASH_RO) || - CONFIG(LOCK_SPI_FLASH_NO_ACCESS)) { - int i; - u32 lockmask = 1UL << 31; - if (CONFIG(LOCK_SPI_FLASH_NO_ACCESS)) - lockmask |= 1 << 15; - for (i = 0; i < 20; i += 4) - RCBA32(0x3874 + i) = RCBA32(0x3854 + i) | lockmask; - } - /* Lock SPIBAR */ RCBA32_OR(0x3804, (1 << 15));
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 1: Code-Review-1
(4 comments)
We need documentation about the platform lockdown mechanism used (PRR?)
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig File src/security/lockdown/Kconfig:
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig@13 PS1, Line 13: default BOOTMEDIA_LOCK_NONE vboot isn't covered correctly
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig@22 PS1, Line 22: media. The locking will take place during the chipset lockdown, which too imprecise
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig@35 PS1, Line 35: boot media the corresponding region is still readable. too imprecise
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/bootmedia.c File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/bootmedia.c@57 PS1, Line 57: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, security_lockdown_bootmedia, Can we move this into the core root of trust vboot_logic.c if vboot is enabled?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 1:
(4 comments)
Patch Set 1: Code-Review-1
(4 comments)
We need documentation about the platform lockdown mechanism used (PRR?)
Agree, I'll add documentation.
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig File src/security/lockdown/Kconfig:
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig@13 PS1, Line 13: default BOOTMEDIA_LOCK_NONE
vboot isn't covered correctly
It's not covered at all
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig@22 PS1, Line 22: media. The locking will take place during the chipset lockdown, which
too imprecise
How to be more precise? This is platform specific?
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/Kconfig@35 PS1, Line 35: boot media the corresponding region is still readable.
too imprecise
It's platform specific, but I don't see how to be more precise
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/bootmedia.c File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/#/c/32704/1/src/security/lockdown/bootmedia.c@57 PS1, Line 57: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, security_lockdown_bootmedia,
Can we move this into the core root of trust vboot_logic. […]
No. It's unrelated to vboot.
Christian Walter has uploaded a new patch set (#2) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/bootmedia.c M src/soc/intel/common/block/fast_spi/Kconfig M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 124 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/2
Christian Walter has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/bootmedia.c M src/soc/intel/common/block/fast_spi/Kconfig M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 124 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 3:
(9 comments)
What is the time penalty when enabling this?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN SECURITY_HAVE_BOOTMEDIA_LOCKDOWN
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 6: support supports
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 22: media medium?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 26: NOTE: If you trigger the chipset lockdown unconditionally, I wouldn’t indent the note. `NOTE:` is enough “markup”.
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: NO_ACCESS RW to be consistent with RO?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 36: The locking will take place during the chipset lockdown, which is Add a blank line above?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 40: "whole bootmedia\n"); Won’t this be printed several time?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 49: } Add an else branch to inform the user about an error?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 53: Didn't Didn’t or couldn’t …
I think there should be a separate error message for users explicitly wanting to lock the device, but it did not work.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 18: LOCK_RO Can we name this option with 'whole' in it so it reads straight forward in the code?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: LOCK_NO_ACCESS same as my comment above.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32704/4/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/4/src/security/lockdown/Kconf... PS4, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN What's the point of this option? boot_device_wp_region() is already available everywhere (because there's a default weak implementation). So I'd either just always run this code (accepting that it doesn't do anything on some platforms... maybe add another print to the weak stub for boot_device_wp_region() for that), or rearrange all of that so that this option is directly tied to the availability of that function. The way you have it here is too disconnected.
(Also, there's really no reason the implementation of boot_device_wp_region() is in boot_device_rw_nommap.c right now. It should work just as well on systems with COMMON_CBFS_SPI_WRAPPER. In fact, there's a bunch of unnecessary duplication between boot_device_rw_nommap.c and cbfs_spi.c that should probably be cleaned up.)
Patrick Rudolph has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/spi/Kconfig M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/bootmedia.c M src/soc/amd/common/block/spi/Kconfig M src/soc/intel/common/block/fast_spi/Kconfig M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 10 files changed, 133 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig File src/drivers/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig@64 PS5, Line 64: config SPI_FLASH_CTRL_PROTECT Technically coreboot supports multiple SPI flashes on multiple SPI controllers, so you can't really generalize over all of them. I'm not sure you need this... you already have runtime checks to make sure you print an error and continue gracefully if this is not supported, I think that should be good enough.
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN Again, I'm not really sure what this option is for, I would just display the 'choice' menu below directly. If the selected choice is NONE, you can skip compiling lockdown/bootmedia.c.
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 19: config BOOTMEDIA_LOCK_RO It feels a bit odd that you force both media and controller lockdown into a single option. Do you even need the case where both are tried on any platform? Why not just offer BOOTMEDIA_LOCK_NONE, BOOTMEDIA_LOCK_CHIP_RO, BOOTMEDIA_LOCK_CONTROLLER_RO and BOOTMEDIA_LOCK_CONTROLLER_NO_ACCESS? Or if you do have a platform where you really need to have it try both ways, you could take all of those and add another BOOTMEDIA_LOCK_BOTH_RO. With every platform having its own constraints here I think having that extra granularity would be helpful.
(In order to not blow it up to too many options, I would make a separate 'choice' for the VBOOT_RO in the next patch, which is only shown when a version with chip locking is selected.)
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... PS5, Line 26: nit: one space too many?
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... PS5, Line 35: #%zu nit: can we use meaningful strings here?
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#6).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 125 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig File src/drivers/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig@64 PS5, Line 64: config SPI_FLASH_CTRL_PROTECT
Technically coreboot supports multiple SPI flashes on multiple SPI controllers, so you can't really […]
removed
https://review.coreboot.org/c/coreboot/+/32704/4/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/4/src/security/lockdown/Kconf... PS4, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN
What's the point of this option? boot_device_wp_region() is already available everywhere (because th […]
removed
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN
Again, I'm not really sure what this option is for, I would just display the 'choice' menu below dir […]
removed
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 19: config BOOTMEDIA_LOCK_RO
It feels a bit odd that you force both media and controller lockdown into a single option. […]
Done
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... PS5, Line 26:
nit: one space too many?
removed
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... PS5, Line 35: #%zu
nit: can we use meaningful strings here?
done
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#7).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 108 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 6:
(11 comments)
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN
SECURITY_HAVE_BOOTMEDIA_LOCKDOWN
removed
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 6: support
supports
removed
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 18: LOCK_RO
Can we name this option with 'whole' in it so it reads straight forward in the code?
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 22: media
medium?
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 26: NOTE: If you trigger the chipset lockdown unconditionally,
I wouldn’t indent the note. `NOTE:` is enough “markup”.
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: LOCK_NO_ACCESS
same as my comment above.
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: NO_ACCESS
RW to be consistent with RO?
it's not read-writeable
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 36: The locking will take place during the chipset lockdown, which is
Add a blank line above?
I don't understand
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 40: "whole bootmedia\n");
Won’t this be printed several time?
removed the loop
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 49: }
Add an else branch to inform the user about an error?
Unified the error handling
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 53: Didn't
Didn’t or couldn’t … […]
Isn't printed any more if no bootmedia protection is selected
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#8).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 108 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/8
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN
removed
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 6: support
removed
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: NO_ACCESS
it's not read-writeable
Ack
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 36: The locking will take place during the chipset lockdown, which is
I don't understand
``` Select this if you want to protect the firmware boot media against all further accesses. On platforms that memory map a part of the boot media the corresponding region is still readable.
The locking will take place during the chipset lockdown, which is ```
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 40: "whole bootmedia\n");
removed the loop
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 49: }
Unified the error handling
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 53: Didn't
Isn't printed any more if no bootmedia protection is selected
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 8: Code-Review+2
(4 comments)
LGTM basically, just some documentation stuff left.
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 17: (e.g. by the payload or the OS). The help text should explain that this is only supported on certain controllers (e.g. Intel).
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 28: The locking will take place during the chipset lockdown, which : 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). This is wrong for chip lockdown, isn't it? It happens immediately when the lockdown code runs.
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 39: Select this if you want to protect the firmware boot medium against : all further accesses. On platforms that memory map a part of the : boot medium the corresponding region is still readable. nit: Just curious... why do we need this option at all? If it only really works on Intel controllers, those are all memory-mapped and the memory-mapping still works afterwards anyway... what's the point in enabling this over CONTROLLER_RO?
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/lockd... PS8, Line 21: CTRL nit: why not spell out 'SPI controller'?
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#9).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete.
Tested on Lenovo T520. The whole flash is protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 108 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 17: (e.g. by the payload or the OS).
The help text should explain that this is only supported on certain controllers (e.g. Intel).
Done
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 28: The locking will take place during the chipset lockdown, which : 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).
This is wrong for chip lockdown, isn't it? It happens immediately when the lockdown code runs.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 9: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 9: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/32704/9/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32704/9/src/security/lockdown/lockd... PS9, Line 44: O this is only true on older Intel platforms following coreboot's boot state machine. On newer platforms it's locked before running option roms.
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#10).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete. As every platform locks the SPIBAR in a different stage, directly call it from the corresponding functions. Until now it's hooked up for x86 Intel boards only.
Tested on Supermicro X11SSH-TF. The whole address space is write-protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 10 files changed, 108 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/10
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32704/9/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32704/9/src/security/lockdown/lockd... PS9, Line 44: O
this is only true on older Intel platforms following coreboot's boot state machine. […]
Addressed in latest patchset
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 10: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 10:
Not sure why we changed it away from its own BSM entry and buried it in platform code now? Seems like the previous approach was cleaner.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 10:
Patch Set 10:
Not sure why we changed it away from its own BSM entry and buried it in platform code now? Seems like the previous approach was cleaner.
That's because platform code decides when to lock down SPI flash and there's no gurantee it will happen in BS_POST_DEVICE. Locking it early here will brick the MRC cache...
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 10:
That's because platform code decides when to lock down SPI flash and there's no gurantee it will happen in BS_POST_DEVICE. Locking it early here will brick the MRC cache...
But the MRC cache is also common code? So just pick the latest it can happen from there (looks like that's OS_RESUME_CHECK)? Sounds like this is something we should try to standardize now if it hasn't been before.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 10:
Patch Set 10:
That's because platform code decides when to lock down SPI flash and there's no gurantee it will happen in BS_POST_DEVICE. Locking it early here will brick the MRC cache...
But the MRC cache is also common code? So just pick the latest it can happen from there (looks like that's OS_RESUME_CHECK)? Sounds like this is something we should try to standardize now if it hasn't been before.
that doesn't work because on Intel the spibar is locked before DEV_INIT.
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#11).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete. As every platform locks the SPIBAR in a different stage, directly call it from the corresponding functions. Until now it's hooked up for x86 Intel boards only.
Tested on Supermicro X11SSH-TF. The whole address space is write-protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 132 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/11
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 11:
(3 comments)
But the MRC cache is also common code? So just pick the latest it can happen from there (looks like that's OS_RESUME_CHECK)? Sounds like this is something we should try to standardize now if it hasn't been before.
that doesn't work because on Intel the spibar is locked before DEV_INIT.
I'm a bit confused, you say it doesn't work but it looks like you now changed the code to do that after all?
https://review.coreboot.org/c/coreboot/+/32704/11/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/11/src/security/lockdown/Kcon... PS11, Line 14: On Intel platforms for e.g. this will make use of the SPIBAR PRRs. nit: mention that only some platforms support this?
https://review.coreboot.org/c/coreboot/+/32704/11/src/security/lockdown/Kcon... PS11, Line 21: protection bits. nit: Same... only some chips. (I think we can assume that all chips supporting this would use flash protection bits in some arrangement, so I don't think you need to call Winbond out like that, but mention that protection bits aren't supported on all chips.)
https://review.coreboot.org/c/coreboot/+/32704/11/src/security/lockdown/Kcon... PS11, Line 33: Select this if you want to write-protect the whole firmware boot : medium using the controller. The locking will take place during the : chipset lockdown, which 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). Some of this text needs moving/updating.
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#12).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete. Every platform locks the SPIBAR in a different stage. For align up with the common mrc cache driver and lock after it has been written to.
Tested on Supermicro X11SSH-TF. The whole address space is write-protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 132 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/12
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 12:
(1 comment)
I'm using two options for now that are aligned with the mrccache driver.
https://review.coreboot.org/c/coreboot/+/32704/11/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/11/src/security/lockdown/Kcon... PS11, Line 33: Select this if you want to write-protect the whole firmware boot : medium using the controller. The locking will take place during the : chipset lockdown, which 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).
Some of this text needs moving/updating.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 12: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 12:
(12 comments)
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig File src/drivers/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig@64 PS5, Line 64: config SPI_FLASH_CTRL_PROTECT
removed
Done
https://review.coreboot.org/c/coreboot/+/32704/1/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/1/src/security/lockdown/Kconf... PS1, Line 13: default BOOTMEDIA_LOCK_NONE
It's not covered at all
Done
https://review.coreboot.org/c/coreboot/+/32704/1/src/security/lockdown/Kconf... PS1, Line 22: media. The locking will take place during the chipset lockdown, which
How to be more precise? This is platform specific?
No reply within a year, marking as resolved
https://review.coreboot.org/c/coreboot/+/32704/1/src/security/lockdown/Kconf... PS1, Line 35: boot media the corresponding region is still readable.
It's platform specific, but I don't see how to be more precise
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 18: LOCK_RO
Done
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 22: media
Done
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 26: NOTE: If you trigger the chipset lockdown unconditionally,
Done
Done
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: LOCK_NO_ACCESS
Done
Done
https://review.coreboot.org/c/coreboot/+/32704/4/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/4/src/security/lockdown/Kconf... PS4, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN
removed
Done
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN
removed
Done
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 19: config BOOTMEDIA_LOCK_RO
Done
Done
https://review.coreboot.org/c/coreboot/+/32704/1/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/1/src/security/lockdown/bootm... PS1, Line 57: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, security_lockdown_bootmedia,
No. It's unrelated to vboot.
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Frans Hendriks, Christian Walter, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32704
to look at the new patch set (#13).
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete. Every platform locks the SPIBAR in a different stage. For align up with the common mrc cache driver and lock after it has been written to.
Tested on Supermicro X11SSH-TF. The whole address space is write-protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 135 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/32704/13
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 36: The locking will take place during the chipset lockdown, which is
Done
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 13: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 13: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
security: Add common boot media write protection
Introduce boot media protection settings and use the existing boot_device_wp_region() function to apply settings on all platforms that supports it yet.
Also remove the Intel southbridge code, which is now obsolete. Every platform locks the SPIBAR in a different stage. For align up with the common mrc cache driver and lock after it has been written to.
Tested on Supermicro X11SSH-TF. The whole address space is write-protected.
Change-Id: Iceb3ecf0bde5cec562bc62d1d5c79da35305d183 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32704 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org --- M src/include/boot_device.h M src/security/Kconfig M src/security/Makefile.inc A src/security/lockdown/Kconfig A src/security/lockdown/Makefile.inc A src/security/lockdown/lockdown.c M src/southbridge/intel/common/Kconfig M src/southbridge/intel/common/finalize.c 8 files changed, 135 insertions(+), 49 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/include/boot_device.h b/src/include/boot_device.h index 4707331..3146462 100644 --- a/src/include/boot_device.h +++ b/src/include/boot_device.h @@ -62,4 +62,12 @@ **/ void boot_device_init(void);
+/* + * Restrict read/write access to the bootmedia using platform defined rules. + */ +#if CONFIG(BOOTMEDIA_LOCK_NONE) +static inline void boot_device_security_lockdown(void) {} +#else +void boot_device_security_lockdown(void); +#endif #endif /* _BOOT_DEVICE_H_ */ diff --git a/src/security/Kconfig b/src/security/Kconfig index b967311..65d2def 100644 --- a/src/security/Kconfig +++ b/src/security/Kconfig @@ -15,3 +15,4 @@ source "src/security/tpm/Kconfig" source "src/security/memory/Kconfig" source "src/security/intel/Kconfig" +source "src/security/lockdown/Kconfig" diff --git a/src/security/Makefile.inc b/src/security/Makefile.inc index fd78438..72b87db 100644 --- a/src/security/Makefile.inc +++ b/src/security/Makefile.inc @@ -2,3 +2,4 @@ subdirs-y += tpm subdirs-y += memory subdirs-y += intel +subdirs-y += lockdown diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig new file mode 100644 index 0000000..bfdc984 --- /dev/null +++ b/src/security/lockdown/Kconfig @@ -0,0 +1,62 @@ + + +choice + prompt "Boot media protection mechanism" + default BOOTMEDIA_LOCK_NONE + +config BOOTMEDIA_LOCK_NONE + bool "Don't lock boot media sections" + +config BOOTMEDIA_LOCK_CONTROLLER + bool "Lock boot media using the controller" + help + Select this if you want the controller to lock specific regions. + This only works on some platforms, please check the code or boot log. + On Intel platforms for e.g. this will make use of the SPIBAR PRRs. + +config BOOTMEDIA_LOCK_CHIP + bool "Lock boot media using the chip" + help + Select this if you want the chip to lock specific regions. + This only works on some chips, please check the code or boot log. + +endchoice + +choice + prompt "Boot media protected regions" + depends on !BOOTMEDIA_LOCK_NONE + default BOOTMEDIA_LOCK_WHOLE_RO + +config BOOTMEDIA_LOCK_WHOLE_RO + bool "Write-protect the whole boot medium" + help + Select this if you want to write-protect the whole firmware boot + medium. + + The locking will take place during the chipset lockdown. + Chipset lockdown is platform specific und might be done unconditionally, + when INTEL_CHIPSET_LOCKDOWN is set or has to be triggered later + (e.g. by the payload or the OS). + + NOTE: If you trigger the chipset lockdown unconditionally, + you won't be able to write to the whole flash chip using the + internal controller any more. + +config BOOTMEDIA_LOCK_WHOLE_NO_ACCESS + depends on BOOTMEDIA_LOCK_CONTROLLER + bool "Read- and write-protect the whole boot medium" + help + Select this if you want to protect the firmware boot medium against + all further accesses. On platforms that memory map a part of the + boot medium the corresponding region is still readable. + + The locking will take place during the chipset lockdown. + Chipset lockdown is platform specific und might be done unconditionally, + when INTEL_CHIPSET_LOCKDOWN is set or has to be triggered later + (e.g. by the payload or the OS). + + NOTE: If you trigger the chipset lockdown unconditionally, + you won't be able to write to the whole flash chip using the + internal controller any more. + +endchoice diff --git a/src/security/lockdown/Makefile.inc b/src/security/lockdown/Makefile.inc new file mode 100644 index 0000000..a51a106 --- /dev/null +++ b/src/security/lockdown/Makefile.inc @@ -0,0 +1,6 @@ +## SPDX-License-Identifier: GPL-2.0-or-later +## This file is part of the coreboot project. + +ifneq ($(CONFIG_BOOTMEDIA_LOCK_NONE),y) +ramstage-y += lockdown.c +endif diff --git a/src/security/lockdown/lockdown.c b/src/security/lockdown/lockdown.c new file mode 100644 index 0000000..a8aad9b --- /dev/null +++ b/src/security/lockdown/lockdown.c @@ -0,0 +1,57 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* This file is part of the coreboot project. */ + +#include <boot_device.h> +#include <commonlib/region.h> +#include <console/console.h> +#include <bootstate.h> + +/* + * Enables read- /write protection of the bootmedia. + */ +void boot_device_security_lockdown(void) +{ + const struct region_device *rdev; + enum bootdev_prot_type lock_type; + + printk(BIOS_DEBUG, "BM-LOCKDOWN: Enabling boot media protection scheme "); + + if (CONFIG(BOOTMEDIA_LOCK_CONTROLLER)) { + if (CONFIG(BOOTMEDIA_LOCK_WHOLE_RO)) { + printk(BIOS_DEBUG, "'readonly'"); + lock_type = CTRLR_WP; + } else if (CONFIG(BOOTMEDIA_LOCK_WHOLE_NO_ACCESS)) { + printk(BIOS_DEBUG, "'no access'"); + lock_type = CTRLR_RWP; + } + printk(BIOS_DEBUG, "using CTRL...\n"); + } else { + if (CONFIG(BOOTMEDIA_LOCK_WHOLE_RO)) { + printk(BIOS_DEBUG, "'readonly'"); + lock_type = MEDIA_WP; + } + printk(BIOS_DEBUG, "using flash chip...\n"); + } + + rdev = boot_device_ro(); + + if (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"); +} + +static void lock(void *unused) +{ + boot_device_security_lockdown(); +} + +/* + * Keep in sync with mrc_cache.c + */ + +#if CONFIG(MRC_WRITE_NV_LATE) +BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_EXIT, lock, NULL); +#else +BOOT_STATE_INIT_ENTRY(BS_DEV_RESOURCES, BS_ON_ENTRY, lock, NULL); +#endif diff --git a/src/southbridge/intel/common/Kconfig b/src/southbridge/intel/common/Kconfig index d1b6bf6..9356a2b 100644 --- a/src/southbridge/intel/common/Kconfig +++ b/src/southbridge/intel/common/Kconfig @@ -97,42 +97,3 @@ config SOUTHBRIDGE_INTEL_COMMON_WATCHDOG bool depends on SOUTHBRIDGE_INTEL_COMMON_PMBASE - -if SOUTHBRIDGE_INTEL_COMMON_FINALIZE - -choice - prompt "Flash locking during chipset lockdown" - default LOCK_SPI_FLASH_NONE - -config LOCK_SPI_FLASH_NONE - bool "Don't lock flash sections" - -config LOCK_SPI_FLASH_RO - bool "Write-protect all flash sections" - help - Select this if you want to write-protect the whole firmware flash - chip. The locking will take place during the chipset lockdown, which - 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). - - NOTE: If you trigger the chipset lockdown unconditionally, - you won't be able to write to the flash chip using the - internal programmer any more. - -config LOCK_SPI_FLASH_NO_ACCESS - bool "Write-protect all flash sections and read-protect non-BIOS sections" - help - Select this if you want to protect the firmware flash against all - further accesses (with the exception of the memory mapped BIOS re- - gion which is always readable). The locking will take place during - the chipset lockdown, which 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). - - NOTE: If you trigger the chipset lockdown unconditionally, - you won't be able to write to the flash chip using the - internal programmer any more. - -endchoice - -endif diff --git a/src/southbridge/intel/common/finalize.c b/src/southbridge/intel/common/finalize.c index 4c6cc63..2d66cad 100644 --- a/src/southbridge/intel/common/finalize.c +++ b/src/southbridge/intel/common/finalize.c @@ -15,16 +15,6 @@ { const pci_devfn_t lpc_dev = PCI_DEV(0, 0x1f, 0);
- if (CONFIG(LOCK_SPI_FLASH_RO) || - CONFIG(LOCK_SPI_FLASH_NO_ACCESS)) { - int i; - u32 lockmask = 1UL << 31; - if (CONFIG(LOCK_SPI_FLASH_NO_ACCESS)) - lockmask |= 1 << 15; - for (i = 0; i < 20; i += 4) - RCBA32(0x3874 + i) = RCBA32(0x3854 + i) | lockmask; - } - /* Lock SPIBAR */ RCBA32_OR(0x3804, (1 << 15));
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 14:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2734 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2733 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2732 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2731
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)