Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32705
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
security/lockdown: Write-protect WP_RO
Add another choice to boot media protection and write-protect WP_RO in case VBOOT is enabled.
Tested on Lenovo T520: The WP_RO region is write-protected.
Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/security/lockdown/Kconfig M src/security/lockdown/bootmedia.c 2 files changed, 29 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/1
diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig index bb4d072..1e982d8 100644 --- a/src/security/lockdown/Kconfig +++ b/src/security/lockdown/Kconfig @@ -15,6 +15,18 @@ config BOOTMEDIA_LOCK_NONE bool "Don't lock boot media sections"
+config BOOTMEDIA_LOCK_VBOOT_RO + bool "Write-protect WP_RO region in boot media" + depends on VBOOT + help + Select this if you want to write-protect the WP_RO region as specified + in the VBOOT FMAP. You will only be able to write the regions + FW_MAIN_A/FW_MAIN_B, which are not write-protected using the internal + programmer. + 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). + config BOOTMEDIA_LOCK_RO bool "Write-protect the whole boot media" help diff --git a/src/security/lockdown/bootmedia.c b/src/security/lockdown/bootmedia.c index 8fb4ae9..6fa2de2 100644 --- a/src/security/lockdown/bootmedia.c +++ b/src/security/lockdown/bootmedia.c @@ -17,6 +17,7 @@ #include <commonlib/region.h> #include <console/console.h> #include <bootstate.h> +#include <fmap.h>
/* * Enable write protection on the WP_RO region of the bootmedia. @@ -47,8 +48,23 @@ "of whole bootmedia\n"); locked = true; } - } + } else if (CONFIG(BOOTMEDIA_LOCK_VBOOT_RO)) { + struct region_device dev; + if (fmap_locate_area_as_rdev("WP_RO", &dev) < 0) { + printk(BIOS_ERR, "BM-LOCKDOWN: Could not find region 'WP_RO'\n"); + } else { + 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(&dev, wp_prot[i]) < 0) + continue;
+ printk(BIOS_INFO, "BM-LOCKDOWN: Enabled write-protection of" + "WP_RO region\n"); + locked = true; + } + } + } if (!locked) printk(BIOS_INFO, "BM-LOCKDOWN: Didn't enable bootmedia protection\n"); }
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
See previous CL, we need some way of doing the lockdown in verstage for vboot and otherwise in ramstage/payload
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Patch Set 1:
See previous CL, we need some way of doing the lockdown in verstage for vboot and otherwise in ramstage/payload
I don't see why. Ramstage is verified in case vboot is used, so all code run here is trusted.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
TOCTOU attacks are possible, also you should always directly lockdown the WP_RO is executed. It is never a good idea to do that in a later stage. So please add a hook for vboot_logic.c in the earliest possible part
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Patch Set 1:
TOCTOU attacks are possible, also you should always directly lockdown the WP_RO is executed. It is never a good idea to do that in a later stage. So please add a hook for vboot_logic.c in the earliest possible part
Please give an example. I don't see how that's possible.
Locking it early will break other functionally in ramstage, like protecting mrc cache.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
The mrc cache has nothing to do with WP_RO AFAIK it is its own FMAP partition.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Patrick and Nico what do you say?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Patch Set 1:
The mrc cache has nothing to do with WP_RO AFAIK it is its own FMAP partition.
You could set FPR in verstage, but it doesn't improve security as the SPIBAR is still locked at end of ramstage. Locking the SPIBAR earlier is not possible as said MRC cache needs to be protected at end of ramstage as well.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Got it what if we use a different locking mechanism for the WP_RO then. I mean do we have different mechanisms on the intel platform?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1: Code-Review-1
I think the concepts clashing here are too different to be easily aligned: One idea is to write-protect the whole flash *after* coreboot is done. The other is the vboot idea of a readonly, trusted partition. The latter has to be read-only *before* we jump to an untrusted RW partition.
So while these two concepts can share the underlying infrastructure to commit the write protection, IMO, their setup and hookup should stay independent.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
I think the concepts clashing here are too different to be easily aligned: One idea is to write-protect the whole flash *after* coreboot is done. The other is the vboot idea of a readonly, trusted partition. The latter has to be read-only *before* we jump to an untrusted RW partition.
So while these two concepts can share the underlying infrastructure to commit the write protection, IMO, their setup and hookup should stay independent.
As vboot verifies the RW partitions, it's trusted code. Locking in ramstage is thus OK following your argumentation. Shouldn't that be +1 then?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Got it what if we use a different locking mechanism for the WP_RO then. I mean do we have different mechanisms on the intel platform?
There are different options depending on the generation and flash chip.
Before Skylake, we had direct access to the flash chip and its status (configuration) registers. Some SPI flashes have an option to lock the block protection (that cros firmware uses with the /WP pin) until the next reset. Requirement here is that the flash chip is known and compatible. What to do if we encounter an incompatible flash? => always run recovery mode?
Since Skylake, access to the flash chip is limited. AFAIK, we can only access the first status register of the flash. Flash chips I know and that have the until-reset feature as described above, need access to a secondary status register, so this won't work here. OTOH, also since Skylake, we have indidual locking bits for the protected region registers in the SPI controller. NB. I don't trust this mechanism, yet. It makes the already too complex SPI controller even harder to reason about. For instance what about other interfaces that you could grant access via FRACC before the global FLOCKDN? is any of them accessible by the host CPU? (earlier there was one for the GbE device)
However, what we could do to retrofit vboot: Ignore all that! Either leave the MRC cache unprotected or limit the setup to a semi-RO MRC cache that can only be filled by the RO coreboot. Once that's done or when the RO coreboot decides that an update isn't necessary, lock the RO par- tition including the MRC cache. Now that I wrote it, it gets too complex. Maybe just ignore the cache.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
I think the concepts clashing here are too different to be easily aligned: One idea is to write-protect the whole flash *after* coreboot is done. The other is the vboot idea of a readonly, trusted partition. The latter has to be read-only *before* we jump to an untrusted RW partition.
So while these two concepts can share the underlying infrastructure to commit the write protection, IMO, their setup and hookup should stay independent.
As vboot verifies the RW partitions, it's trusted code. Locking in ramstage is thus OK following your argumentation. Shouldn't that be +1 then?
Um, no. Nothing is trusted that isn't in RO, afaiui. Consider this scenario: You issue an update that con- tains an exploitable bug in some parser for disk con- tents. Then, if not write-protected early, your root of trust could be compromised. Recovery mode and fur- ther updates won't help.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Can we at least make the flash protections mechanisms via an interface available instead of hardcoding a solution for vboot and mrc cache ?? @Nico
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 1:
Can we at least make the flash protections mechanisms via an interface available instead of hardcoding a solution for vboot and mrc cache ?? @Nico
I don't follow. What interface? we already have boot_device_wp_region().
Christian Walter has uploaded a new patch set (#2) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
security/lockdown: Write-protect WP_RO
Add another choice to boot media protection and write-protect WP_RO in case VBOOT is enabled. Also add ability to choose when to lock bootmedia, either in VERSTAGE if VBOOT is enabled - otherwise in RAMSTAGE.
Tested on Lenovo T520: The WP_RO region is write-protected.
Tested on Up Sqaured: THe WP_RO region is write-protected in the verstage/ramstage.
Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc M src/security/lockdown/bootmedia.c A src/security/lockdown/bootmedia.h M src/security/vboot/verstage.c 5 files changed, 80 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG@7 PS2, Line 7: security/lockdown: Write-protect WP_RO I think this about *when* to perform those actions. I think the subject could be more precise.
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.h:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... PS2, Line 16: void security_lockdown_bootmedia(void *unused); Should we move this to boot_device.h ? And how is this fundamentally different than boot_device_wp_region() ?
After digging into the patch series I see this is more of a wrapper for boot_device_wp_region(). I feel like we should provide more consistency, though. A first reading doesn't immediately explain the relation. I do think it'd be better to add directly to boot_device.h API. Thoughts?
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... File src/security/vboot/verstage.c:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 35: #if CONFIG(BOOTMEDIA_LOCK_IN_VERSTAGE) What do we have #if guards?
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 36: security_lockdown_bootmedia((void *) NULL); NULL is already void * ?
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG@7 PS2, Line 7: security/lockdown: Write-protect WP_RO
I think this about *when* to perform those actions. I think the subject could be more precise.
Isnt it enough to explain in the details that we also added the ability to choose the *when*?
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.h:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... PS2, Line 16: void security_lockdown_bootmedia(void *unused);
Should we move this to boot_device. […]
It's more or less only a wrapper true. I can explain it in more detail in the commit message. But still I would rather not put it directly into boot_device.h. I like functions being implemented where the .h file is. Just my personal 2 cents. :)
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... File src/security/vboot/verstage.c:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 35: #if CONFIG(BOOTMEDIA_LOCK_IN_VERSTAGE)
What do we have #if guards?
Changed it to "normal" if CONFIG(..).
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 36: security_lockdown_bootmedia((void *) NULL);
NULL is already void * ?
Ack
Christian Walter has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
security/lockdown: Write-protect WP_RO
Add another choice to boot media protection and write-protect WP_RO in case VBOOT is enabled. Also add ability to choose when to lock bootmedia, either in VERSTAGE if VBOOT is enabled - otherwise in RAMSTAGE.
Tested on Lenovo T520: The WP_RO region is write-protected.
Tested on Up Sqaured: THe WP_RO region is write-protected in the verstage/ramstage.
Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc M src/security/lockdown/bootmedia.c A src/security/lockdown/bootmedia.h M src/security/vboot/verstage.c 5 files changed, 79 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.h:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... PS2, Line 16: void security_lockdown_bootmedia(void *unused);
It's more or less only a wrapper true. I can explain it in more detail in the commit message. […]
Yes, but you created this header file. My point was more: why have this header file at all and introduce another noun, boot media, when there's already a boot device? It reduces the clarity of the connection, imo.
Christian Walter has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
security/lockdown: Write-protect WP_RO
Add a wrapper function boot_device_security_lockdown which wraps boot_device_wp_region to either lock (read/write) the WP_RO region or the complete boot device depending on the Kconfig. One can either lock the boot device in VERSTAGE if VBOOT is enabled, or in RAMSTAGE.
Tested on Lenovo T520: The WP_RO region is write-protected.
Tested on Up Sqaured: THe WP_RO region is write-protected in the verstage/ramstage.
Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/include/boot_device.h M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc R src/security/lockdown/lockdown.c M src/security/vboot/verstage.c 5 files changed, 68 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/4
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG@7 PS2, Line 7: security/lockdown: Write-protect WP_RO
Isnt it enough to explain in the details that we also added the ability to choose the *when*?
Done
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.h:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... PS2, Line 16: void security_lockdown_bootmedia(void *unused);
Yes, but you created this header file. […]
I moved the declaration to boot_device.h and renamed the source file to lockdown.c. Also changed the function name to fit more to the convention. Hope this resolves this here :)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/4/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/4/src/security/lockdown/lockd... PS4, Line 59: printk(BIOS_DEBUG, "BM-LOCKDOWN: Trying write-protection " format strings don't need to be broken up.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/32705/5/src/include/boot_device.h File src/include/boot_device.h:
https://review.coreboot.org/c/coreboot/+/32705/5/src/include/boot_device.h@7... PS5, Line 78: Looks Locks
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/Kconf... PS5, Line 61: chip nit: should probably say "boot media", not "chip"
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/vboot/verstage... File src/security/vboot/verstage.c:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/vboot/verstage... PS5, Line 35: boot_device_security_lockdown(NULL); Note that this code is only executed in CONFIG_SEPARATE_VERSTAGE builds. You probably want to put it in verstage_main() instead.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/lockd... PS5, Line 76: security_lockdown_bootmedia Is there an expectation to add this to a BS entry some place else? Otherwise you could drop the void *unused parameter from the public API and handle that requirement with a static function here.
Patrick Rudolph has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
security/lockdown: Write-protect WP_RO
Add a wrapper function boot_device_security_lockdown which wraps boot_device_wp_region to either lock (read/write) the WP_RO region or the complete boot device depending on the Kconfig. One can either lock the boot device in VERSTAGE if VBOOT is enabled, or in RAMSTAGE.
Tested on Lenovo T520: The WP_RO region is write-protected.
Tested on Up Sqaured: THe WP_RO region is write-protected in the verstage/ramstage.
Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/include/boot_device.h M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc R src/security/lockdown/lockdown.c M src/security/vboot/vboot_logic.c 5 files changed, 69 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... PS6, Line 26: programmer controller
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... PS6, Line 27: chipset lockdown Then what about the LOCK_IN_VERSTAGE and LOCK_IN_RAMSTAGE options?
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... PS6, Line 56: for (size_t i = 0; i < ARRAY_SIZE(wp_prot); i++) { This is the same sequence as above in the BOOTMEDIA_LOCK_RO path. You should have a helper that fills out a region_device based on the Kconfig:
static int helper(struct region_device *rdev) { if (CONFIG(BOOTMEDIA_LOCK_RO) || CONFIG(BOOTMEDIA_LOCK_NO_ACCESS)) { const struct region_device *boot_rdev = bootdevice_ro(); return rdev_chain_full(rdev, boot_rdev); } else if (CONFIG(BOOTMEDIA_LOCK_VBOOT_RO)) return fmap_locate_area_as_rdev("WP_RO", rdev); }
return -1; }
And add another helper for going through the wp_prot sequence for ones you care about:
static int another_helper(const struct region_device *rdev, const int *wp_prot, size_t wp_num) { for (size_t i = 0; i < wp_num; i++) { boot_device_wp_region(rdev, wp_prot[i]) } }
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... PS6, Line 75: security_lockdown_bootmedia This is calling a symbol that doesn't exist?
Philipp Deppenwiese has uploaded a new patch set (#7) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
security/lockdown: Write-protect WP_RO
Add a wrapper function boot_device_security_lockdown which wraps boot_device_wp_region to either lock (read/write) the WP_RO region or the complete boot device depending on the Kconfig. One can either lock the boot device in VERSTAGE if VBOOT is enabled, or in RAMSTAGE.
Tested on Lenovo T520: The WP_RO region is write-protected.
Tested on Up Sqaured: THe WP_RO region is write-protected in the verstage/ramstage.
Change-Id: I72c3e1a0720514b9b85b0433944ab5fb7109b2a2 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/include/boot_device.h M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc R src/security/lockdown/lockdown.c M src/security/vboot/vboot_logic.c 5 files changed, 70 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/32705/7/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/32705/7/src/security/vboot/vboot_lo... PS7, Line 317: trailing whitespace
https://review.coreboot.org/c/coreboot/+/32705/7/src/security/vboot/vboot_lo... PS7, Line 317: please, no spaces at the start of a line
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/7/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/7/src/security/lockdown/lockd... PS7, Line 40: nit: Would be easier to review if you could already pull back the unrelated fixes here into the previous CL
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#8).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc M src/security/lockdown/lockdown.c M src/security/vboot/vboot_logic.c 4 files changed, 67 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/8/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/8/src/security/lockdown/lockd... PS8, Line 36: if (fmap_locate_area_as_rdev("WP_RO", &dev) < 0) { braces {} are not necessary for any arm of this statement
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#9).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc M src/security/lockdown/lockdown.c M src/security/vboot/vboot_logic.c 4 files changed, 57 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/32705/5/src/include/boot_device.h File src/include/boot_device.h:
https://review.coreboot.org/c/coreboot/+/32705/5/src/include/boot_device.h@7... PS5, Line 78: Looks
Locks
Done
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/Kconf... PS5, Line 61: chip
nit: should probably say "boot media", not "chip"
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... PS6, Line 26: programmer
controller
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... PS6, Line 27: chipset lockdown
Then what about the LOCK_IN_VERSTAGE and LOCK_IN_RAMSTAGE options?
Done
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/lockd... PS5, Line 76: security_lockdown_bootmedia
Is there an expectation to add this to a BS entry some place else? Otherwise you could drop the void […]
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... PS6, Line 56: for (size_t i = 0; i < ARRAY_SIZE(wp_prot); i++) {
This is the same sequence as above in the BOOTMEDIA_LOCK_RO path. […]
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... PS6, Line 75: security_lockdown_bootmedia
This is calling a symbol that doesn't exist?
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Kcon... PS10, Line 6: default BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO if VBOOT && !CHROMEOS Since controller locks are not supported on all platforms, I don't think we want to set a default here. I think this decision is too complicated and too dependent on the specific board layout to really make a general recommendation for everyone, so I'd leave it at NONE.
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Kcon... PS10, Line 56: write the regions : FW_MAIN_A/FW_MAIN_B, which are not write-protected using the internal : controller. nit: well, technically you may write everything that's outside WP_RO which usually also contains some common shared data outside of the RW A/B sections.
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Make... File src/security/lockdown/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Make... PS10, Line 12: bootblock-$(VBOOT_STARTS_IN_BOOTBLOCK) += lockdown.c You should not need this. We already merge verstage-srcs into the appropriate stage when SEPARATE_VERSTAGE=n. You should only need
verstage-$(CONFIG_BOOTMEDIA_LOCK_IN_VERSTAGE) += lockdown.c
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#11).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc M src/security/lockdown/lockdown.c M src/security/vboot/vboot_logic.c 4 files changed, 54 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/11
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#12).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- M src/security/lockdown/Kconfig M src/security/lockdown/Makefile.inc M src/security/lockdown/lockdown.c M src/security/vboot/vboot_logic.c 4 files changed, 49 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/12
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Kcon... PS10, Line 6: default BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO if VBOOT && !CHROMEOS
Since controller locks are not supported on all platforms, I don't think we want to set a default he […]
Done
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Kcon... PS10, Line 56: write the regions : FW_MAIN_A/FW_MAIN_B, which are not write-protected using the internal : controller.
nit: well, technically you may write everything that's outside WP_RO which usually also contains som […]
Done
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Make... File src/security/lockdown/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32705/10/src/security/lockdown/Make... PS10, Line 12: bootblock-$(VBOOT_STARTS_IN_BOOTBLOCK) += lockdown.c
You should not need this. […]
Done
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 12:
In cases where vboot isn't used, could this be extended later to only cover the COREBOOT region (decision in config), or is there a reason why this wouldn't work?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 12: Code-Review+2
In cases where vboot isn't used, could this be extended later to only cover the COREBOOT region (decision in config), or is there a reason why this wouldn't work?
I would stick to the name WP_RO since COREBOOT doesn't always contain all the code. For example, on non-x86 devices the bootblock is usually stored in a separate BOOTBLOCK section. But if there's desire for it we could update the util/cbfstool/default.fmd and .../default-x86.fmd maps to include a WP_RO wrapper for the relevant sections (COREBOOT, BOOTBLOCK and FMAP, I'd say) and then we can make it available without CONFIG_VBOOT. (One issue with this is that if you want to use chip-based lockdown, the WP_RO vs. rest split must be on a power-of-two boundary. I guess this patch only implements controller-based lockdown so it's not relevant there, but it might become so if it were expanded later.)
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#13).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- 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, 49 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/13
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 13: Code-Review+2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... PS13, Line 65: BOOTMEDIA_LOCK_IN_VERSTAGE why is this option outside of the 'choice' menu?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/13/src/include/boot_device.h File src/include/boot_device.h:
https://review.coreboot.org/c/coreboot/+/32705/13/src/include/boot_device.h@... PS13, Line 79: ENV_VERSTAGE This doesn't work because ENV_VERSTAGE isn't always a real stage (if CONFIG_SEPARATE_VERSTAGE=n this may be ENV_BOOTBLOCK or ENV_ROMSTAGE). I think the simplest solution would be
#if CONFIG(BOOTMEDIA_LOCK_NONE) || (CONFIG(BOOTMEDIA_LOCK_IN_VERSTAGE) && ENV_RAMSTAGE)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/32705/13/src/include/boot_device.h File src/include/boot_device.h:
https://review.coreboot.org/c/coreboot/+/32705/13/src/include/boot_device.h@... PS13, Line 79: ENV_VERSTAGE
This doesn't work because ENV_VERSTAGE isn't always a real stage (if CONFIG_SEPARATE_VERSTAGE=n this […]
Ok, will change.
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... PS13, Line 65: BOOTMEDIA_LOCK_IN_VERSTAGE
why is this option outside of the 'choice' menu?
It depends on BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO. So it can't be in the list as separate option.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... PS13, Line 65: BOOTMEDIA_LOCK_IN_VERSTAGE
It depends on BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO. So it can't be in the list as separate option.
FWIW I think this would be a bit cleaner by moving even more options out of the choice block, because it's a bit overloaded. The choice really just needs to be about the basic locking mechanism (NONE, CHIP or CONTROLLER). Then you can have a separate choice about what to lock (ALL or VBOOT_RO, depends on mechanism != NONE), the LOCK_IN_VERSTAGE option here like it is (depends on lock area VBOOT_RO) and another boolean LOCK_BOTH_READ_AND_WRITE option (depends on lock type CONTROLLER) to cover the NO_ACCESS case (which you could combine with VBOOT_RO if you wanted, after all).
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#14).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- 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(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/14
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32705/14/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/14/src/security/lockdown/Kcon... PS14, Line 60: fmap capitalize
Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Frans Hendriks, Christian Walter, Julius Werner, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32705
to look at the new patch set (#15).
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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 --- 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(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32705/15
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/32705/13/src/include/boot_device.h File src/include/boot_device.h:
https://review.coreboot.org/c/coreboot/+/32705/13/src/include/boot_device.h@... PS13, Line 79: ENV_VERSTAGE
Ok, will change.
Done
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/13/src/security/lockdown/Kcon... PS13, Line 65: BOOTMEDIA_LOCK_IN_VERSTAGE
FWIW I think this would be a bit cleaner by moving even more options out of the choice block, becaus […]
Done
https://review.coreboot.org/c/coreboot/+/32705/14/src/security/lockdown/Kcon... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/14/src/security/lockdown/Kcon... PS14, Line 60: fmap
capitalize
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 15: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 15:
(9 comments)
https://review.coreboot.org/c/coreboot/+/32705/5/src/include/boot_device.h File src/include/boot_device.h:
https://review.coreboot.org/c/coreboot/+/32705/5/src/include/boot_device.h@7... PS5, Line 78: Looks
Done
Done
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/Kconf... PS5, Line 61: chip
Done
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... PS6, Line 26: programmer
Done
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/Kconf... PS6, Line 27: chipset lockdown
Done
Done
https://review.coreboot.org/c/coreboot/+/32705/4/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/4/src/security/lockdown/lockd... PS4, Line 59: printk(BIOS_DEBUG, "BM-LOCKDOWN: Trying write-protection "
format strings don't need to be broken up.
Done
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/lockdown/lockd... PS5, Line 76: security_lockdown_bootmedia
Done
Done
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... PS6, Line 56: for (size_t i = 0; i < ARRAY_SIZE(wp_prot); i++) {
Done
Simplified the code.
https://review.coreboot.org/c/coreboot/+/32705/6/src/security/lockdown/lockd... PS6, Line 75: security_lockdown_bootmedia
Done
Done
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/vboot/verstage... File src/security/vboot/verstage.c:
https://review.coreboot.org/c/coreboot/+/32705/5/src/security/vboot/verstage... PS5, Line 35: boot_device_security_lockdown(NULL);
Note that this code is only executed in CONFIG_SEPARATE_VERSTAGE builds. […]
Done
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 15: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
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(-)
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 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();
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 17:
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/2740 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2739 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2738 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2737
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Arthur Heymans (Code Review)
-
Benjamin Doron (Code Review)
-
build bot (Jenkins) (Code Review)
-
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)