Change in coreboot[master]: boot_device: Add boot_device_lockdown

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39925 )

Change subject: boot_device: Add boot_device_lockdown
......................................................................

boot_device: Add boot_device_lockdown

Add a new method to lock the SPI protected range registers.

This allows to lock the SPI controller early, which can be used to
write-protect the WP_RO region in bootblock before handing of control
to the later stages not protected by WP_RO.

In conjunction with VBOOT and BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO this
enables a secure boot mechanism on non CHROMEOS enabled devices.

Change-Id: I9d3a80a2e278c77212e1fba5236ea639ea018837
Signed-off-by: Patrick Rudolph 
---
M src/drivers/spi/boot_device_rw_nommap.c
M src/drivers/spi/spi_flash.c
M src/include/boot_device.h
M src/include/spi-generic.h
M src/include/spi_flash.h
M src/lib/boot_device.c
M src/security/lockdown/lockdown.c
M src/soc/intel/common/block/fast_spi/fast_spi_flash.c
M src/soc/intel/common/pch/lockdown/lockdown.c
M src/southbridge/intel/bd82x6x/lpc.c
M src/southbridge/intel/common/finalize.c
M src/southbridge/intel/common/spi.c
M src/southbridge/intel/ibexpeak/lpc.c
M src/southbridge/intel/lynxpoint/lpc.c
14 files changed, 82 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39925/1

diff --git a/src/drivers/spi/boot_device_rw_nommap.c b/src/drivers/spi/boot_device_rw_nommap.c
index 5de9a71..be90318 100644
--- a/src/drivers/spi/boot_device_rw_nommap.c
+++ b/src/drivers/spi/boot_device_rw_nommap.c
@@ -133,3 +133,12 @@
    return spi_flash_ctrlr_protect_region(boot_dev,
    				region_device_region(rd), ctrlr_pr);
 }
+
+int boot_device_lockdown(void)
+{
+	const struct spi_flash *boot_dev = boot_device_spi_flash();
+	if (boot_dev == NULL)
+		return -1;
+
+	return spi_flash_ctrlr_lockdown(boot_dev);
+}
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c
index 5149829..6fdda2c 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -707,6 +707,17 @@
    return -1;
 }

+int spi_flash_ctrlr_lockdown(const struct spi_flash *flash)
+{
+	if (!flash || !flash->spi.ctrlr)
+		return -1;
+
+	if (flash->spi.ctrlr->lockdown)
+		return flash->spi.ctrlr->lockdown(flash);
+
+	return -1;
+}
+
 int spi_flash_vector_helper(const struct spi_slave *slave,
    struct spi_op vectors[], size_t count,
    int (*func)(const struct spi_slave *slave, const void *dout,
diff --git a/src/include/boot_device.h b/src/include/boot_device.h
index a31fdff..d813f16 100644
--- a/src/include/boot_device.h
+++ b/src/include/boot_device.h
@@ -68,6 +68,11 @@
    			const enum bootdev_prot_type type);

/*
+ * Lock the controller after all WP regions have been set.
+ */
+int boot_device_lockdown(void);
+
+/*
  * Initialize the boot device. This may be called multiple times within
  * a stage so boot device implementations should account for this behavior.
  **/
diff --git a/src/include/spi-generic.h b/src/include/spi-generic.h
index 3022886..7c2a9d8 100644
--- a/src/include/spi-generic.h
+++ b/src/include/spi-generic.h
@@ -156,6 +156,8 @@
  * flash_probe:	Specialized probe function provided by SPI flash
  *			controllers.
  * flash_protect: Protect a region of flash using the SPI flash controller.
+ * lockdown: Enable protected regions. flash_protect will return an error after
+ * 		calling this function.
  */
 struct spi_ctrlr {
    int (*claim_bus)(const struct spi_slave *slave);
@@ -174,6 +176,7 @@
    int (*flash_protect)(const struct spi_flash *flash,
    			const struct region *region,
    			const enum ctrlr_prot_type type);
+	int (*lockdown)(const struct spi_flash *flash);
 };

/*-----------------------------------------------------------------------
diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h
index c74ceae..9c1a5f7 100644
--- a/src/include/spi_flash.h
+++ b/src/include/spi_flash.h
@@ -227,6 +227,12 @@
    			   const enum ctrlr_prot_type type);

/*
+ * Lock down the SPI controller.
+ * Modifying protected protected regions will become impossible.
+ */
+int spi_flash_ctrlr_lockdown(const struct spi_flash *flash);
+
+/*
  * This function is provided to support spi flash command-response transactions.
  * Only 2 vectors are supported and the 'func' is called with appropriate
  * write and read buffers together. This can be used for chipsets that
diff --git a/src/lib/boot_device.c b/src/lib/boot_device.c
index dfb4066..81b1825 100644
--- a/src/lib/boot_device.c
+++ b/src/lib/boot_device.c
@@ -26,6 +26,12 @@
    return -1;
 }

+int __weak boot_device_lockdown(void)
+{
+	/* return a failure, make aware lockdown is not implemented */
+	return -1;
+}
+
 static int boot_device_subregion(const struct region *sub,
    			struct region_device *subrd,
    			const struct region_device *parent)
diff --git a/src/security/lockdown/lockdown.c b/src/security/lockdown/lockdown.c
index 3bbf75f..0396ee7 100644
--- a/src/security/lockdown/lockdown.c
+++ b/src/security/lockdown/lockdown.c
@@ -46,6 +46,11 @@
    	printk(BIOS_INFO, "BM-LOCKDOWN: Enabled bootmedia protection\n");
    else
    	printk(BIOS_ERR, "BM-LOCKDOWN: Failed to enable bootmedia protection\n");
+
+	if (CONFIG(BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO)) {
+		printk(BIOS_INFO, "BM-LOCKDOWN: Locking down controller\n");
+		boot_device_lockdown();
+	}
 }

#if ENV_RAMSTAGE
diff --git a/src/soc/intel/common/block/fast_spi/fast_spi_flash.c b/src/soc/intel/common/block/fast_spi/fast_spi_flash.c
index a8afd48..4428740 100644
--- a/src/soc/intel/common/block/fast_spi/fast_spi_flash.c
+++ b/src/soc/intel/common/block/fast_spi/fast_spi_flash.c
@@ -422,9 +422,16 @@
    return 0;
 }

+static int fast_spi_flash_lockdown(const struct spi_flash *flash)
+{
+	fast_spi_pr_dlock();
+	return 0;
+}
+
 const struct spi_ctrlr fast_spi_flash_ctrlr = {
    .setup = fast_spi_flash_ctrlr_setup,
    .max_xfer_size = SPI_CTRLR_DEFAULT_MAX_XFER_SIZE,
    .flash_probe = fast_spi_flash_probe,
    .flash_protect = fast_spi_flash_protect,
+	.lockdown = fast_spi_flash_lockdown,
 };
diff --git a/src/soc/intel/common/pch/lockdown/lockdown.c b/src/soc/intel/common/pch/lockdown/lockdown.c
index 036cccc..de45643 100644
--- a/src/soc/intel/common/pch/lockdown/lockdown.c
+++ b/src/soc/intel/common/pch/lockdown/lockdown.c
@@ -20,6 +20,7 @@
 #include <soc/pci_devs.h>
 #include <soc/pcr_ids.h>
 #include <soc/soc_chip.h>
+#include <boot_device.h>

#define PCR_DMI_GCS		0x274C
 #define PCR_DMI_GCS_BILD	(1 << 0)
@@ -63,7 +64,7 @@
    fast_spi_set_opcode_menu();

/* Discrete Lock Flash PR registers */
-	fast_spi_pr_dlock();
+	boot_device_lockdown();

/* Lock FAST_SPIBAR */
    fast_spi_lock_bar();
diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c
index 46ab5fc..ae0b70f 100644
--- a/src/southbridge/intel/bd82x6x/lpc.c
+++ b/src/southbridge/intel/bd82x6x/lpc.c
@@ -38,6 +38,7 @@
 #include <southbridge/intel/common/pmutil.h>
 #include <southbridge/intel/common/rtc.h>
 #include <southbridge/intel/common/spi.h>
+#include <boot_device.h>

#define NMI_OFF	0

@@ -849,12 +850,14 @@
 {
    spi_finalize_ops();

+	/* Lock SPIBAR */
+	if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3())
+		boot_device_lockdown();
+
    /* Call SMM finalize() handlers before resume */
    if (CONFIG(HAVE_SMI_HANDLER)) {
-		if (CONFIG(INTEL_CHIPSET_LOCKDOWN) ||
-		    acpi_is_wakeup_s3()) {
+		if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3())
    		outb(APM_CNT_FINALIZE, APM_CNT);
-		}
    }
 }

diff --git a/src/southbridge/intel/common/finalize.c b/src/southbridge/intel/common/finalize.c
index 2d4d4a3..e5aa983 100644
--- a/src/southbridge/intel/common/finalize.c
+++ b/src/southbridge/intel/common/finalize.c
@@ -27,9 +27,6 @@
 {
    const pci_devfn_t lpc_dev = PCI_DEV(0, 0x1f, 0);

-	/* Lock SPIBAR */
-	RCBA32_OR(0x3804, (1 << 15));
-
    if (CONFIG(SPI_FLASH_SMM))
    	/* Re-init SPI driver to handle locked BAR */
    	spi_init();
diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c
index 5e967af..ffa283b 100644
--- a/src/southbridge/intel/common/spi.c
+++ b/src/southbridge/intel/common/spi.c
@@ -1110,6 +1110,16 @@
    writew_(optype, cntlr.optype);
 }

+static int spi_flash_lockdown(const struct spi_flash *flash)
+{
+	if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) {
+		writew_(readw_(&cntlr.ich7_spi->spis) | HSFS_FLOCKDN, &cntlr.ich7_spi->spis);
+	} else {
+		writew_(readw_(&cntlr.ich9_spi->spis) | HSFS_FLOCKDN, &cntlr.ich9_spi->spis);
+	}
+	return 0;
+}
+
 __weak void intel_southbridge_override_spi(struct intel_swseq_spi_config *spi_config)
 {
 }
@@ -1119,7 +1129,7 @@
    .max_xfer_size = member_size(struct ich9_spi_regs, fdata),
    .flash_probe = spi_flash_programmer_probe,
    .flash_protect = spi_flash_protect,
-};
+	.lockdown = spi_flash_lockdown,

const struct spi_ctrlr_buses spi_ctrlr_bus_map[] = {
    {
diff --git a/src/southbridge/intel/ibexpeak/lpc.c b/src/southbridge/intel/ibexpeak/lpc.c
index 0062e09..c1e7904 100644
--- a/src/southbridge/intel/ibexpeak/lpc.c
+++ b/src/southbridge/intel/ibexpeak/lpc.c
@@ -38,6 +38,7 @@
 #include <southbridge/intel/common/pciehp.h>
 #include <southbridge/intel/common/acpi_pirq_gen.h>
 #include <southbridge/intel/common/spi.h>
+#include <boot_device.h>

#define NMI_OFF	0

@@ -748,12 +749,14 @@
 {
    spi_finalize_ops();

+	/* Lock SPIBAR */
+	if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3())
+		boot_device_lockdown();
+
    /* Call SMM finalize() handlers before resume */
    if (CONFIG(HAVE_SMI_HANDLER)) {
-		if (CONFIG(INTEL_CHIPSET_LOCKDOWN) ||
-		    acpi_is_wakeup_s3()) {
+		if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3())
    		outb(APM_CNT_FINALIZE, APM_CNT);
-		}
    }
 }

diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c
index 745c231..e4ab43d 100644
--- a/src/southbridge/intel/lynxpoint/lpc.c
+++ b/src/southbridge/intel/lynxpoint/lpc.c
@@ -35,6 +35,7 @@
 #include <southbridge/intel/common/acpi_pirq_gen.h>
 #include <southbridge/intel/common/rtc.h>
 #include <southbridge/intel/common/spi.h>
+#include <boot_device.h>

#define NMI_OFF	0

@@ -951,6 +952,10 @@
 {
    spi_finalize_ops();

+	/* Lock SPIBAR */
+	if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3())
+		boot_device_lockdown();
+
    if (acpi_is_wakeup_s3() || CONFIG(INTEL_CHIPSET_LOCKDOWN))
    	outb(APM_CNT_FINALIZE, APM_CNT);
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39925 )

Change subject: boot_device: Add boot_device_lockdown
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39925/1/src/include/spi-generic.h 
File src/include/spi-generic.h:

https://review.coreboot.org/c/coreboot/+/39925/1/src/include/spi-generic.h@1... 
PS1, Line 160:  * 		calling this function.
please, no space before tabs

https://review.coreboot.org/c/coreboot/+/39925/1/src/southbridge/intel/commo... 
File src/southbridge/intel/common/spi.c:

https://review.coreboot.org/c/coreboot/+/39925/1/src/southbridge/intel/commo... 
PS1, Line 1115: 	if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) {
braces {} are not necessary for any arm of this statement

[Patrick Rudolph (Code Review)]

Hello Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39925

to look at the new patch set (#2).

Change subject: boot_device: Add boot_device_lockdown
......................................................................

boot_device: Add boot_device_lockdown

Add a new method to lock the SPI protected range registers.

This allows to lock the SPI controller early, which can be used to
write-protect the WP_RO region in bootblock before handing of control
to the later stages not protected by WP_RO.

In conjunction with VBOOT and BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO this
enables a secure boot mechanism on non CHROMEOS enabled devices.

Change-Id: I9d3a80a2e278c77212e1fba5236ea639ea018837
Signed-off-by: Patrick Rudolph 
---
M src/drivers/spi/boot_device_rw_nommap.c
M src/drivers/spi/spi_flash.c
M src/include/boot_device.h
M src/include/spi-generic.h
M src/include/spi_flash.h
M src/lib/boot_device.c
M src/security/lockdown/lockdown.c
M src/soc/intel/common/block/fast_spi/fast_spi_flash.c
M src/soc/intel/common/pch/lockdown/lockdown.c
M src/southbridge/intel/bd82x6x/lpc.c
M src/southbridge/intel/common/finalize.c
M src/southbridge/intel/common/spi.c
M src/southbridge/intel/ibexpeak/lpc.c
M src/southbridge/intel/lynxpoint/lpc.c
14 files changed, 82 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39925/2

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39925

to look at the new patch set (#4).

Change subject: boot_device: Add boot_device_lockdown
......................................................................

boot_device: Add boot_device_lockdown

Add a new method to lock the SPI protected range registers.

This allows to lock the SPI controller early, which can be used to
write-protect the WP_RO region in bootblock before handing of control
to the later stages not protected by WP_RO.

In conjunction with VBOOT and BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO this
enables a secure boot mechanism on non CHROMEOS enabled devices.

Change-Id: I9d3a80a2e278c77212e1fba5236ea639ea018837
Signed-off-by: Patrick Rudolph 
---
M src/drivers/spi/boot_device_rw_nommap.c
M src/drivers/spi/spi_flash.c
M src/include/boot_device.h
M src/include/spi-generic.h
M src/include/spi_flash.h
M src/lib/boot_device.c
M src/security/lockdown/lockdown.c
M src/soc/intel/common/block/fast_spi/fast_spi_flash.c
M src/soc/intel/common/pch/lockdown/lockdown.c
M src/southbridge/intel/bd82x6x/lpc.c
M src/southbridge/intel/common/finalize.c
M src/southbridge/intel/common/spi.c
M src/southbridge/intel/ibexpeak/lpc.c
M src/southbridge/intel/lynxpoint/lpc.c
14 files changed, 82 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39925/4

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39925

to look at the new patch set (#5).

Change subject: boot_device: Add boot_device_lockdown
......................................................................

boot_device: Add boot_device_lockdown

Add a new method to lock the SPI protected range registers.

This allows to lock the SPI controller early, which can be used to
write-protect the WP_RO region in bootblock before handing of control
to the later stages not protected by WP_RO.

In conjunction with VBOOT and BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO this
enables a secure boot mechanism on non CHROMEOS enabled devices.

Change-Id: I9d3a80a2e278c77212e1fba5236ea639ea018837
Signed-off-by: Patrick Rudolph 
---
M src/drivers/spi/boot_device_rw_nommap.c
M src/drivers/spi/spi_flash.c
M src/include/boot_device.h
M src/include/spi-generic.h
M src/include/spi_flash.h
M src/lib/boot_device.c
M src/security/lockdown/lockdown.c
M src/soc/intel/common/block/fast_spi/fast_spi_flash.c
M src/soc/intel/common/pch/lockdown/lockdown.c
M src/southbridge/intel/bd82x6x/lpc.c
M src/southbridge/intel/common/finalize.c
M src/southbridge/intel/common/spi.c
M src/southbridge/intel/ibexpeak/lpc.c
M src/southbridge/intel/lynxpoint/lpc.c
14 files changed, 82 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39925/5

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39925

to look at the new patch set (#6).

Change subject: boot_device: Add boot_device_lockdown
......................................................................

boot_device: Add boot_device_lockdown

Add a new method to lock the SPI protected range registers.

This allows to lock the SPI controller early, which can be used to
write-protect the WP_RO region in bootblock before handing of control
to the later stages not protected by WP_RO.

In conjunction with VBOOT and BOOTMEDIA_LOCK_CONTROLLER_RO_VBOOT_RO this
enables a secure boot mechanism on non CHROMEOS enabled devices.

Also move the SPIBAR locking on older Intel platforms into ring0, keeping
the current locking logic the same. Only the dependency to SMI_HANDLER is
dropped.

Change-Id: I9d3a80a2e278c77212e1fba5236ea639ea018837
Signed-off-by: Patrick Rudolph 
---
M src/drivers/spi/boot_device_rw_nommap.c
M src/drivers/spi/spi_flash.c
M src/include/boot_device.h
M src/include/spi-generic.h
M src/include/spi_flash.h
M src/lib/boot_device.c
M src/security/lockdown/lockdown.c
M src/soc/intel/braswell/southcluster.c
M src/soc/intel/broadwell/finalize.c
M src/soc/intel/common/block/fast_spi/fast_spi_flash.c
M src/soc/intel/common/pch/lockdown/lockdown.c
M src/southbridge/intel/common/finalize.c
M src/southbridge/intel/common/spi.c
M src/southbridge/intel/i82801gx/lpc.c
14 files changed, 71 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39925/6

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39925 )

Change subject: boot_device: Add boot_device_lockdown
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39925/6/src/southbridge/intel/commo... 
File src/southbridge/intel/common/finalize.c:

https://review.coreboot.org/c/coreboot/+/39925/6/src/southbridge/intel/commo... 
PS6, Line 30: /* Lock SPIBAR */
            : 	RCBA32_OR(0x3804, (1 << 15));
This removes the option to lock SPIBAR via SMM. Not sure if anyone cares to do it that way anymore...

[Philipp Deppenwiese (Code Review)]

Attention is currently required from: Patrick Rudolph, Arthur Heymans.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39925 )

Change subject: boot_device: Add boot_device_lockdown
......................................................................

Patch Set 6: Code-Review+2

(1 comment)

File src/southbridge/intel/common/finalize.c:

https://review.coreboot.org/c/coreboot/+/39925/comment/6b2605d0_0ea4f5a2 
PS6, Line 30: /* Lock SPIBAR */
            : 	RCBA32_OR(0x3804, (1 << 15));
> This removes the option to lock SPIBAR via SMM. Not sure if anyone cares to do it that way anymore.. […]

¯_(ツ)_/¯

[Patrick Rudolph (Code Review)]

Attention is currently required from: Arthur Heymans.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39925 )

Change subject: boot_device: Add boot_device_lockdown
......................................................................

Patch Set 6:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/39925/comment/15618104_89702ebe 
PS6, Line 18: Also move the SPIBAR locking on older Intel platforms into ring0, keeping
line length

[M] Change in coreboot[main]: boot_device: Add boot_device_lockdown

[Marshall Dawson (Code Review)]

Attention is currently required from: Arthur Heymans, Patrick Rudolph.

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39925?usp=email )

Change subject: boot_device: Add boot_device_lockdown
......................................................................

Patch Set 6:

(2 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/39925/comment/655069da_f34a2c5b :
PS6, Line 12: before handing of control
nit: you probably meant "before the handing of control" or "before handing off control".

Patchset:

PS6: 
Not sure I remember enough Intel stuff to provide much additional critique (conveniently)