Change in coreboot[master]: [WIP]security/intel: Add option to eanble SMM flash access only
Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
[WIP]security/intel: Add option to eanble SMM flash access only
On platforms where the boot media can be updated externally, for e.g. using a BMC, add the possibility to enable writes in SMM only.
This allows to protect the BIOS region even without the use of vboot, but keeps the SMMSTORE working for the use in payloads.
UNTESTED.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/common/spi.c M src/southbridge/intel/common/spi.h M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/smi.c 8 files changed, 52 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/1
diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig index d24c5e5..dbe6840 100644 --- a/src/security/lockdown/Kconfig +++ b/src/security/lockdown/Kconfig @@ -99,3 +99,13 @@ 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. + +config BOOTMEDIA_SMM_BWP + bool "Boot media only writable in SMM" + depends on ARCH_X86 + depends on HAVE_SMI_HANDLER + select SOC_INTEL_COMMON_BLOCK_SMM_TCO_ENABLE if SOC_INTEL_COMMON_BLOCK + help + Enable flash writes in SMM only. Select this if you want to secure + the SMM_STORE and don't need to update coreboot using the + internal controller. diff --git a/src/soc/intel/common/pch/lockdown/lockdown.c b/src/soc/intel/common/pch/lockdown/lockdown.c index a00e5c0..768c4cd 100644 --- a/src/soc/intel/common/pch/lockdown/lockdown.c +++ b/src/soc/intel/common/pch/lockdown/lockdown.c @@ -62,6 +62,12 @@ /* BIOS Interface Lock */ fast_spi_set_bios_interface_lock_down();
+ /* Only allow writes in SMM */ + if (CONFIG(BOOTMEDIA_SMM_BWP)) { + fast_spi_set_eiss(); + fast_spi_enable_wp(); + } + /* BIOS Lock */ fast_spi_set_lock_enable(); } diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index 278e90a..78bfb39 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -416,16 +416,6 @@ } }
-static void pch_disable_smm_only_flashing(struct device *dev) -{ - u8 reg8; - - printk(BIOS_SPEW, "Enabling BIOS updates outside of SMM... "); - reg8 = pci_read_config8(dev, BIOS_CNTL); - reg8 &= ~(1 << 5); - pci_write_config8(dev, BIOS_CNTL, reg8); -} - static void pch_fixups(struct device *dev) { u8 gen_pmcon_2; @@ -579,8 +569,6 @@ /* Interrupt 9 should be level triggered (SCI) */ i8259_configure_irq_trigger(9, 1);
- pch_disable_smm_only_flashing(dev); - pch_set_acpi_mode();
pch_fixups(dev); diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index 5e967af..253d33f 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -35,6 +35,10 @@ #define HSFC_FCYCLE (0x3 << HSFC_FCYCLE_OFF) #define HSFC_FDBC_OFF 8 /* 8-13: Flash Data Byte Count */ #define HSFC_FDBC (0x3f << HSFC_FDBC_OFF) +#define BIOS_CNTL 0xdc +#define LPC_BC_WE (1 << 0) /* WE */ +#define LPC_BC_LE (1 << 1) /* LE */ +#define LPC_BC_EISS (1 << 5) /* EISS */
static int spi_is_multichip(void);
@@ -293,7 +297,6 @@
void spi_init(void) { - uint8_t bios_cntl; struct ich9_spi_regs *ich9_spi; struct ich7_spi_regs *ich7_spi; uint16_t hsfs; @@ -345,10 +348,7 @@
if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX) || CONFIG(SOUTHBRIDGE_INTEL_COMMON_SPI_ICH9)) { /* Disable the BIOS write protect so write commands are allowed. */ - bios_cntl = pci_read_config8(dev, 0xdc); - /* Deassert SMM BIOS Write Protect Disable. */ - bios_cntl &= ~(1 << 5); - pci_write_config8(dev, 0xdc, bios_cntl | 0x1); + spi_smm_only_flashing(0); } }
@@ -1108,12 +1108,35 @@ writeb_(spi_config->ops[i].op, &cntlr.opmenu[i]); } writew_(optype, cntlr.optype); + + spi_smm_only_flashing(CONFIG(BOOTMEDIA_SMM_BWP)); }
__weak void intel_southbridge_override_spi(struct intel_swseq_spi_config *spi_config) { }
+void spi_smm_only_flashing(bool enable) +{ + pci_devfn_t dev = PCI_DEV(0, 31, 0); + u8 reg8 = pci_read_config8(dev, BIOS_CNTL); + + if (reg8 & LPC_BC_LE) + return; + + if (enable) { + reg8 |= LPC_BC_EISS; + reg8 &= ~LPC_BC_WE; + } else { + reg8 &= ~LPC_BC_EISS; + reg8 |= LPC_BC_WE; + } + pci_write_config8(dev, BIOS_CNTL, reg8); + + printk(BIOS_SPEW, "BIOS updates outside of SMM %s\n", + enable ? "enabled" : "disabled"); +} + static const struct spi_ctrlr spi_ctrlr = { .xfer_vector = xfer_vectors, .max_xfer_size = member_size(struct ich9_spi_regs, fdata), diff --git a/src/southbridge/intel/common/spi.h b/src/southbridge/intel/common/spi.h index 3b8410c..ad1cb27 100644 --- a/src/southbridge/intel/common/spi.h +++ b/src/southbridge/intel/common/spi.h @@ -32,6 +32,7 @@ struct intel_spi_op ops[8]; };
+void spi_smm_only_flashing(bool enable); void spi_finalize_ops(void); void intel_southbridge_override_spi(struct intel_swseq_spi_config *spi_config);
diff --git a/src/southbridge/intel/ibexpeak/lpc.c b/src/southbridge/intel/ibexpeak/lpc.c index 395919e..67c03fa 100644 --- a/src/southbridge/intel/ibexpeak/lpc.c +++ b/src/southbridge/intel/ibexpeak/lpc.c @@ -420,16 +420,6 @@ } }
-static void pch_disable_smm_only_flashing(struct device *dev) -{ - u8 reg8; - - printk(BIOS_SPEW, "Enabling BIOS updates outside of SMM... "); - reg8 = pci_read_config8(dev, BIOS_CNTL); - reg8 &= ~(1 << 5); - pci_write_config8(dev, BIOS_CNTL, reg8); -} - static void pch_fixups(struct device *dev) { /* @@ -482,8 +472,6 @@ /* Interrupt 9 should be level triggered (SCI) */ i8259_configure_irq_trigger(9, 1);
- pch_disable_smm_only_flashing(dev); - pch_set_acpi_mode();
pch_fixups(dev); diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index d9ef5cc..dafb901 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -481,16 +481,6 @@ } }
-static void pch_disable_smm_only_flashing(struct device *dev) -{ - u8 reg8; - - printk(BIOS_SPEW, "Enabling BIOS updates outside of SMM... "); - reg8 = pci_read_config8(dev, BIOS_CNTL); - reg8 &= ~(1 << 5); - pci_write_config8(dev, BIOS_CNTL, reg8); -} - static void pch_fixups(struct device *dev) { u8 gen_pmcon_2; @@ -549,8 +539,6 @@ /* Interrupt 9 should be level triggered (SCI) */ i8259_configure_irq_trigger(9, 1);
- pch_disable_smm_only_flashing(dev); - pch_set_acpi_mode();
pch_fixups(dev); diff --git a/src/southbridge/intel/lynxpoint/smi.c b/src/southbridge/intel/lynxpoint/smi.c index 05b5bb5..82e96f3 100644 --- a/src/southbridge/intel/lynxpoint/smi.c +++ b/src/southbridge/intel/lynxpoint/smi.c @@ -40,6 +40,8 @@
void smm_southbridge_enable_smi(void) { + u32 mask; + printk(BIOS_DEBUG, "Enabling SMIs.\n"); /* Configure events */ enable_pm1(PWRBTN_EN | GBL_EN); @@ -49,11 +51,14 @@ * - on APMC writes (io 0xb2) * - on writes to SLP_EN (sleep states) * - on writes to GBL_RLS (bios commands) + * - on TCO events * No SMIs: * - on microcontroller writes (io 0x62/0x66) - * - on TCO events */ - enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS); + mask = APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS; + if (CONFIG(BOOTMEDIA_SMM_BWP)) + mask |= TCO_EN; + enable_smi(mask); }
static void __unused southbridge_trigger_smi(void)
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40830/1/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/40830/1/src/security/lockdown/Kconf... PS1, Line 103: BOOTMEDIA_SMM_BWP The SMM_BWP bit is not being set, which may be necessary as per https://www.kb.cert.org/vuls/id/766164.
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40830/1/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/40830/1/src/security/lockdown/Kconf... PS1, Line 103: BOOTMEDIA_SMM_BWP
The SMM_BWP bit is not being set, which may be necessary as per https://www.kb.cert. […]
My mistake, SMM_BWP is the EISS bit ("Enable InSMM.STS").
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40830/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40830/1//COMMIT_MSG@7 PS1, Line 7: eanble enable
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... PS1, Line 94: soc_lockdown_config Skylake follows the same procedure (_set_bios_interface_lock_down, _set_lock_enable) on the LPC in `soc_lockdown_config` as `fast_spi_lockdown_cfg` above. Is it necessary to implement the above additions over there as well?
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Tested on Skylake. Relevant patches that I have applied include the parent of this change, but as `fast_spi_pr_dlock` completes, I guess it isn't relevant.
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... PS1, Line 66: if (CONFIG(BOOTMEDIA_SMM_BWP)) { Placing debug prints throughout this function shows that boots are failing before executing/completing `fast_spi_set_eiss`.
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... PS1, Line 66: if (CONFIG(BOOTMEDIA_SMM_BWP)) {
Placing debug prints throughout this function shows that boots are failing before executing/completi […]
I was using the SPI flashconsole. While it is included in the SMM stage, I don't think it's executing in SMM context. Maybe this hangs.
Regardless, it seemed to hang even before I tried the flashconsole.
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/40830/1/src/soc/intel/common/pch/lo... PS1, Line 94: soc_lockdown_config
Skylake follows the same procedure (_set_bios_interface_lock_down, _set_lock_enable) on the LPC in ` […]
No, it appears that configuration of the LPC there is to lock it. BIOS accesses go to SPI on my board (likely on most, if not all, boards).
Attention is currently required from: Patrick Rudolph. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1: Could `fast_spi_init()` in "soc/intel/common/block/smm/smihandler.c" at `finalize` be the issue? It attempts to set WPD bit and unset EISS bit, but EISS bit is locked by LE bit.
(Setting WPD should result in a SMI, executing `smihandler_soc_check_illegal_access()`, also in "soc/intel/common/block/smm/smihandler.c" but this should not be an issue.)
Attention is currently required from: Patrick Rudolph. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Could `fast_spi_init()` in "soc/intel/common/block/smm/smihandler. […]
On the LPC (and therefore, is it perhaps not relevant?), the EISS bit is cleared by `lpc_soc_init` in "src/soc/intel/skylake/lpc.c"
BS_DEV_INIT is after BS_DEV_RESOURCES, where the lock would have been set (but it may not apply to LPC).
Attention is currently required from: Patrick Rudolph. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
On the LPC (and therefore, is it perhaps not relevant?), the EISS bit is cleared by `lpc_soc_init` i […]
By patching "src/soc/intel/common/block/smm/smihandler.c" with the below, the system (Skylake-U) will boot. However, it hangs when attempting to write to SPI using flashrom. I'm not certain why, but the bits we might want to set there (prefetch and caching enabled) are set.
Perhaps write protect status must only be modified (done by `smihandler_soc_check_illegal_access()`) along with "Synchronous SMI Status" bit. See https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/Kabyla....
--- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -318,7 +318,7 @@ static void finalize(void) } finalize_done = 1;
- if (CONFIG(SPI_FLASH_SMM)) + if (CONFIG(SPI_FLASH_SMM) && !CONFIG(BOOTMEDIA_SMM_BWP)) /* Re-init SPI driver to handle locked BAR */ fast_spi_init();
File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/16d8aede_a059f8f9 PS1, Line 66: if (CONFIG(BOOTMEDIA_SMM_BWP)) {
I was using the SPI flashconsole. […]
While various coreboot features are incompatible with SPI lockdown, this clearly works. Marking as done.
Attention is currently required from: Patrick Rudolph. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: [WIP]security/intel: Add option to eanble SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
By patching "src/soc/intel/common/block/smm/smihandler. […]
Alternatively, if it's not the "Synchronous SMI Status" bit, based on https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/Kabyla... calling https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/Kabyla..., there's an MSR to set.
I'll test these, write a patch and then maybe something can be merged?
Attention is currently required from: Angel Pons. Angel Pons has uploaded a new patch set (#2) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only.
This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads.
Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4, SMM protection works as expected.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 4 files changed, 50 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/2
Attention is currently required from: Angel Pons. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/40830/comment/39b44e90_e5866611 PS1, Line 7: eanble
enable
Done
Attention is currently required from: Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Patchset:
PS1:
Alternatively, if it's not the "Synchronous SMI Status" bit, based on https://github. […]
I can reproduce the hang on HP 280 G2
Attention is currently required from: Patrick Rudolph, Angel Pons. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 2:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/40830/comment/d621dc38_54f9bd48 PS2, Line 15: Note that this breaks flashconsole, since the flash becomes read-only. Add this to Kconfig help text?
https://review.coreboot.org/c/coreboot/+/40830/comment/0acc1d5a_27d62b45 PS2, Line 17: Tested on Asrock B85M Pro4, SMM protection works as expected. Before this goes in, this should be tested (as working) on PCH platforms as well.
Patchset:
PS1:
I can reproduce the hang on HP 280 G2
While booting or on attempting to flash SPI?
Attention is currently required from: Patrick Rudolph, Angel Pons. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
I can reproduce the hang on HP 280 G2
While booting or on attempting to flash SPI?
Oh, forgot that I only noted here the hang when flashing. Sorry, please disregard my previous comment.
Attention is currently required from: Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/40830/comment/42b73c16_f67e4b8a PS2, Line 17: Tested on Asrock B85M Pro4, SMM protection works as expected.
Before this goes in, this should be tested (as working) on PCH platforms as well.
What's "PCH platforms"?
Attention is currently required from: Patrick Rudolph, Angel Pons. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/40830/comment/8fbb4b9c_1c0b7cd5 PS2, Line 17: Tested on Asrock B85M Pro4, SMM protection works as expected.
What's "PCH platforms"?
I meant platforms using SOC_INTEL_COMMON_PCH_LOCKDOWN, which is most of soc/intel/*lake.
Attention is currently required from: Patrick Rudolph, Angel Pons. Angel Pons has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Do not add the option to soc/intel platforms yet. The SMI# handler does not clear all status bits, and boards lock up because of a SMI# storm. The SMI# storm issues are going to be addressed in subsequent commits.
Tested on Asrock B85M Pro4, SMM protection works as expected.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 4 files changed, 49 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/3
Attention is currently required from: Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 2: -Code-Review
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/40830/comment/b484483b_4d0e0c18 PS2, Line 15: Note that this breaks flashconsole, since the flash becomes read-only.
Add this to Kconfig help text?
Done
https://review.coreboot.org/c/coreboot/+/40830/comment/51be2c50_003c755d PS2, Line 17: Tested on Asrock B85M Pro4, SMM protection works as expected.
I meant platforms using SOC_INTEL_COMMON_PCH_LOCKDOWN, which is most of soc/intel/*lake.
I've removed the option for soc/intel platforms, for now. Once CB:50754 fixes the SMI# handler bugs, I'll re-add the option.
Attention is currently required from: Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
I can reproduce the hang on HP 280 G2 […]
soc/intel will be taken care of in subsequent commits.
Attention is currently required from: Benjamin Doron, Angel Pons. Angel Pons has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4, SMM protection works as expected.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 5 files changed, 69 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/5
Attention is currently required from: Patrick Rudolph, Benjamin Doron, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5: Why cling to the WPD bit? AFAIR, there is an exploitable race, e.g. one core is busy SMI handling and before it is done resetting the WPD bit another core already started a write cycle. And that's why the InSMM.STS thing was invented. I'm not 100% sure. It was said the race was fixed, but my interpretation was that only InSMM.STS fixes it.
In any case I would prefer a warning at the Kconfig option that this probably isn't secure. Or just implement InSMM.STS right away. Then you don't even need to reset WPD, AIUI.
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5:
Why cling to the WPD bit? AFAIR, there is an exploitable race, e.g. one […]
InSMM.STS does not exist on older plaforms. Moreover, InSMM.STS needs special handling when using SMMSTORE, and the point of these patches is to protect the flash chip while allowsing SMMSTORE to work (otherwise I would've simply used protected ranges). Once I have InSMM.STS working, I can enable it by default where supported when one chooses to write-protect the flash through SMM.
Attention is currently required from: Patrick Rudolph, Benjamin Doron, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS5:
InSMM.STS does not exist on older plaforms.
That's right, and it's known that those platforms can't be SMM protected.
Moreover, InSMM.STS needs special handling when using SMMSTORE, and the point of these patches is to protect the flash chip while allowsing SMMSTORE to work (otherwise I would've simply used protected ranges).
I know what you are up to. It just seems to me that all the WPD related patches are effectively about a no-op.
Once I have InSMM.STS working, I can enable it by default where supported when one chooses to write-protect the flash through SMM.
Again, on older platforms it's not really write-protected without InSMM.STS. So you would just leave a Kconfig there that only pretends to protect?
Well, we could also just call it "protection against accidental writes".
Attention is currently required from: Nico Huber, Patrick Rudolph, Angel Pons. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS5:
InSMM.STS does not exist on older plaforms.
That's right, and it's known that those platforms can't be SMM protected.
Moreover, InSMM.STS needs special handling when using SMMSTORE, and the point of these patches is to protect the flash chip while allowsing SMMSTORE to work (otherwise I would've simply used protected ranges).
Is that the case? My understanding is that handling is required for WPD as well. To allow writes (on newer platforms), SPI_BC WPD is set and InSMM.Sts is set in MSR 0x1fe.
I know what you are up to. It just seems to me that all the WPD related patches are effectively about a no-op.
Once I have InSMM.STS working, I can enable it by default where supported when one chooses to write-protect the flash through SMM.
I tried to get it working at CB:50724, but regular SPI write protection wasn't working. I haven't yet tested an update based on CB:50754, but you can try the InSMM.Sts handling there.
Again, on older platforms it's not really write-protected without InSMM.STS. So you would just leave a Kconfig there that only pretends to protect?
Well, we could also just call it "protection against accidental writes".
Attention is currently required from: Nico Huber, Angel Pons. Angel Pons has uploaded a new patch set (#9) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4, SMM protection works as expected.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 5 files changed, 72 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/9
Attention is currently required from: Nico Huber, Patrick Rudolph, Angel Pons. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 9:
(1 comment)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/c91be291_60db0a21 PS9, Line 296: if (CONFIG(SPI_FLASH_SMM)) `fast_spi_init` sets SPI read config bits (to the same value as before, it's called in bootblock too) and disables WP. It shouldn't be necessary here.
So, `fast_spi_enable_wp` shouldn't be necessary too.
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 9:
(1 comment)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/3ecff269_1ec0db8f PS9, Line 296: if (CONFIG(SPI_FLASH_SMM))
`fast_spi_init` sets SPI read config bits (to the same value as before, it's called in bootblock too […]
The call to `fast_spi_enable_wp` is to ensure that WPD does not remain set, which only matters when one wants to restrict write access to flash. Without this, flash remains unprotected until something clears WPD.
In any case, removing the call to `fast_spi_init` as well as the associated discussion does not belong to this patch.
Attention is currently required from: Nico Huber, Patrick Rudolph, Angel Pons. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 9:
(1 comment)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/0ce19f8b_a800e616 PS9, Line 296: if (CONFIG(SPI_FLASH_SMM))
The call to `fast_spi_enable_wp` is to ensure that WPD does not remain set, which only matters when […]
Ack
Attention is currently required from: Nico Huber, Angel Pons. Angel Pons has uploaded a new patch set (#10) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4, SMM protection works as expected.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 5 files changed, 77 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/10
Attention is currently required from: Nico Huber, Patrick Rudolph, Angel Pons. Angel Pons has uploaded a new patch set (#11) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4 and HP 280 G2, SMM BIOS write protection works as expected, and SMMSTORE can still be used.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 7 files changed, 92 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/11
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 11:
(1 comment)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/8c4d0cb3_d7d72c39 PS9, Line 296: if (CONFIG(SPI_FLASH_SMM))
Ack
Actually, not even SMM can write to the flash chip when WPD is cleared.
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron, Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/b376d968_979848e7 PS12, Line 279: fast_spi_disable_wp This should backup the current WPD state and restore it after smmstore_exec. Also smmstore_exec is called from other smihandlers as well, where this code is missing.
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 12:
(2 comments)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/738a2f7b_b9540c5c PS12, Line 279: fast_spi_disable_wp
This should backup the current WPD state and restore it after smmstore_exec. […]
Sounds reasonable. Which other SMI handlers are you referring to?
https://review.coreboot.org/c/coreboot/+/40830/comment/8532f9de_a7a000b6 PS12, Line 283: pmc_clear_smi_status(); Should be dropped
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron. Angel Pons has uploaded a new patch set (#13) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4 and HP 280 G2, SMM BIOS write protection works as expected, and SMMSTORE can still be used.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 7 files changed, 91 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/13
Attention is currently required from: Nico Huber, Benjamin Doron, Angel Pons, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 12:
(2 comments)
File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/40830/comment/eec9eb6d_d0c1fad9 PS12, Line 279: fast_spi_disable_wp
Sounds reasonable. […]
Done. If you mean the southbridge/intel SMI handlers, I think I could use SMMSTORE on B85M Pro4 without having to set the BIOS Write Enable bit.
https://review.coreboot.org/c/coreboot/+/40830/comment/8c4f76e4_b3af649d PS12, Line 283: pmc_clear_smi_status();
Should be dropped
Gone
Attention is currently required from: Nico Huber, Benjamin Doron, Angel Pons, Patrick Rudolph. Angel Pons has uploaded a new patch set (#16) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4 and HP 280 G2, SMM BIOS write protection works as expected, and SMMSTORE can still be used.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 7 files changed, 94 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/16
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron, Angel Pons. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 16: Code-Review+2
(1 comment)
File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/40830/comment/526dfc7c_5604998d PS16, Line 95: flashconsole make this option depend on !CONSOLE_SPI_FLASH ?
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron, Angel Pons. Angel Pons has uploaded a new patch set (#17) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4 and HP 280 G2, SMM BIOS write protection works as expected, and SMMSTORE can still be used.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 7 files changed, 95 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/17
Attention is currently required from: Nico Huber, Benjamin Doron, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 16:
(1 comment)
File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/40830/comment/06ac897c_ed21c105 PS16, Line 95: flashconsole
make this option depend on !CONSOLE_SPI_FLASH ?
Done
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron, Angel Pons. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 18: Code-Review+2
Attention is currently required from: Nico Huber, Benjamin Doron, Angel Pons, Patrick Rudolph. Angel Pons has uploaded a new patch set (#20) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4 and HP 280 G2, SMM BIOS write protection works as expected, and SMMSTORE can still be used.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 7 files changed, 95 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40830/20
Attention is currently required from: Nico Huber, Patrick Rudolph, Benjamin Doron, Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 20: Code-Review+2
Patrick Rudolph has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
security/intel: Add option to enable SMM flash access only
On platforms where the boot media can be updated externally, e.g. using a BMC, add the possibility to enable writes in SMM only. This allows to protect the BIOS region even without the use of vboot, but keeps SMMSTORE working for use in payloads. Note that this breaks flashconsole, since the flash becomes read-only.
Tested on Asrock B85M Pro4 and HP 280 G2, SMM BIOS write protection works as expected, and SMMSTORE can still be used.
Change-Id: I157db885b5f1d0f74009ede6fb2342b20d9429fa Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40830 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/security/lockdown/Kconfig M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/include/intelblocks/fast_spi.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/common/pch/lockdown/lockdown.c M src/southbridge/intel/common/finalize.c M src/southbridge/intel/common/spi.c 7 files changed, 95 insertions(+), 35 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/security/lockdown/Kconfig b/src/security/lockdown/Kconfig index 9d83a45..8d48beb 100644 --- a/src/security/lockdown/Kconfig +++ b/src/security/lockdown/Kconfig @@ -84,6 +84,17 @@ ramstage, like the MRC cache for example. Use this option if you don't trust code running after verstage.
+config BOOTMEDIA_SMM_BWP + bool "Boot media only writable in SMM" + depends on !CONSOLE_SPI_FLASH + depends on BOOT_DEVICE_SPI_FLASH && HAVE_SMI_HANDLER + depends on SOUTHBRIDGE_INTEL_COMMON_SPI || SOC_INTEL_COMMON_BLOCK_SPI + select SOC_INTEL_COMMON_BLOCK_SMM_TCO_ENABLE if SOC_INTEL_COMMON_BLOCK_SPI + help + Only allow flash writes in SMM. Select this if you want to use SMMSTORE + while also preventing unauthorized writes through the internal controller. + Note that this breaks flashconsole, since the flash becomes read-only. + choice prompt "SPI Flash write protection duration" default BOOTMEDIA_SPI_LOCK_REBOOT diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index 15cd26e..843071e 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -418,3 +418,14 @@ bios_cntl &= ~SPI_BIOS_CONTROL_WPD; pci_write_config8(dev, SPI_BIOS_CONTROL, bios_cntl); } + +/* Disable SPI Write Protect. */ +void fast_spi_disable_wp(void) +{ + const pci_devfn_t dev = PCH_DEV_SPI; + uint8_t bios_cntl; + + bios_cntl = pci_read_config8(dev, SPI_BIOS_CONTROL); + bios_cntl |= SPI_BIOS_CONTROL_WPD; + pci_write_config8(dev, SPI_BIOS_CONTROL, bios_cntl); +} diff --git a/src/soc/intel/common/block/include/intelblocks/fast_spi.h b/src/soc/intel/common/block/include/intelblocks/fast_spi.h index d044256..d5d3cd5 100644 --- a/src/soc/intel/common/block/include/intelblocks/fast_spi.h +++ b/src/soc/intel/common/block/include/intelblocks/fast_spi.h @@ -77,6 +77,10 @@ */ void fast_spi_enable_wp(void); /* + * Disable SPI Write protect. + */ +void fast_spi_disable_wp(void); +/* * Get base and size of extended BIOS decode window used at runtime in host address space. If * the BIOS region is not greater than 16MiB, then this function returns 0 for both base and * size. diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index 91142a5..5789b48 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -44,31 +44,6 @@ return 1; }
-/* - * Needs to implement the mechanism to know if an illegal attempt - * has been made to write to the BIOS area. - */ -static void smihandler_soc_check_illegal_access( - uint32_t tco_sts) -{ - if (!((tco_sts & (1 << 8)) && CONFIG(SPI_FLASH_SMM) - && fast_spi_wpd_status())) - return; - - /* - * BWE is RW, so the SMI was caused by a - * write to BWE, not by a write to the BIOS - * - * This is the place where we notice someone - * is trying to tinker with the BIOS. We are - * trying to be nice and just ignore it. A more - * resolute answer would be to power down the - * box. - */ - printk(BIOS_DEBUG, "Switching back to RO\n"); - fast_spi_enable_wp(); -} - /* Mainboard overrides. */
__weak void mainboard_smi_gpi_handler( @@ -301,9 +276,19 @@ /* Parameter buffer in EBX */ reg_ebx = save_state_ops->get_reg(io_smi, RBX);
+ const bool wp_enabled = !fast_spi_wpd_status(); + if (wp_enabled) { + fast_spi_disable_wp(); + /* Not clearing SPI sync SMI status here results in hangs */ + fast_spi_clear_sync_smi_status(); + } + /* drivers/smmstore/smi.c */ ret = smmstore_exec(sub_command, (void *)(uintptr_t)reg_ebx); save_state_ops->set_reg(io_smi, RAX, ret); + + if (wp_enabled) + fast_spi_enable_wp(); }
static void finalize(void) @@ -320,6 +305,9 @@ /* Re-init SPI driver to handle locked BAR */ fast_spi_init();
+ if (CONFIG(BOOTMEDIA_SMM_BWP)) + fast_spi_enable_wp(); + /* * HECI is disabled in smihandler_soc_at_finalize() which also locks down the side band * interface. Some boards may require this interface in mainboard_smi_finalize(), @@ -401,12 +389,26 @@ */ fast_spi_clear_sync_smi_status();
+ /* If enabled, enforce SMM BIOS write protection */ + if (CONFIG(BOOTMEDIA_SMM_BWP) && fast_spi_wpd_status()) { + /* + * BWE is RW, so the SMI was caused by a + * write to BWE, not by a write to the BIOS + * + * This is the place where we notice someone + * is trying to tinker with the BIOS. We are + * trying to be nice and just ignore it. A more + * resolute answer would be to power down the + * box. + */ + printk(BIOS_DEBUG, "Switching SPI back to RO\n"); + fast_spi_enable_wp(); + } + /* Any TCO event? */ if (!tco_sts) return;
- smihandler_soc_check_illegal_access(tco_sts); - if (tco_sts & TCO_TIMEOUT) { /* TIMEOUT */ /* Handle TCO timeout */ printk(BIOS_DEBUG, "TCO Timeout.\n"); diff --git a/src/soc/intel/common/pch/lockdown/lockdown.c b/src/soc/intel/common/pch/lockdown/lockdown.c index d9495a4..87f36fc 100644 --- a/src/soc/intel/common/pch/lockdown/lockdown.c +++ b/src/soc/intel/common/pch/lockdown/lockdown.c @@ -65,6 +65,12 @@ /* BIOS Interface Lock */ fast_spi_set_bios_interface_lock_down();
+ /* Only allow writes in SMM */ + if (CONFIG(BOOTMEDIA_SMM_BWP)) { + //fast_spi_set_eiss(); /* TODO */ + fast_spi_enable_wp(); + } + /* BIOS Lock */ fast_spi_set_lock_enable();
diff --git a/src/southbridge/intel/common/finalize.c b/src/southbridge/intel/common/finalize.c index 975d839..6fb27bb 100644 --- a/src/southbridge/intel/common/finalize.c +++ b/src/southbridge/intel/common/finalize.c @@ -45,6 +45,9 @@ pci_write_config32(PCI_DEV(0, 27, 0), 0x74, pci_read_config32(PCI_DEV(0, 27, 0), 0x74));
+ if (CONFIG(BOOTMEDIA_SMM_BWP)) + write_pmbase16(SMI_EN, read_pmbase16(SMI_EN) | TCO_EN); + write_pmbase16(TCO1_CNT, read_pmbase16(TCO1_CNT) | TCO_LOCK);
post_code(POST_OS_BOOT); diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index 828f3d3..30f7657 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -27,6 +27,8 @@
static int spi_is_multichip(void);
+static void spi_set_smm_only_flashing(bool enable); + struct ich7_spi_regs { uint16_t spis; uint16_t spic; @@ -282,7 +284,6 @@
void spi_init(void) { - uint8_t bios_cntl; struct ich9_spi_regs *ich9_spi; struct ich7_spi_regs *ich7_spi; uint16_t hsfs; @@ -332,13 +333,8 @@
ich_set_bbar(0);
- if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX) || CONFIG(SOUTHBRIDGE_INTEL_COMMON_SPI_ICH9)) { - /* Disable the BIOS write protect so write commands are allowed. */ - bios_cntl = pci_read_config8(dev, 0xdc); - /* Deassert SMM BIOS Write Protect Disable. */ - bios_cntl &= ~(1 << 5); - pci_write_config8(dev, 0xdc, bios_cntl | 0x1); - } + /* Disable the BIOS write protect so write commands are allowed. */ + spi_set_smm_only_flashing(false); }
static int spi_locked(void) @@ -1096,12 +1092,39 @@ writeb_(spi_config->ops[i].op, &cntlr.opmenu[i]); } writew_(optype, cntlr.optype); + + spi_set_smm_only_flashing(CONFIG(BOOTMEDIA_SMM_BWP)); }
__weak void intel_southbridge_override_spi(struct intel_swseq_spi_config *spi_config) { }
+#define BIOS_CNTL 0xdc +#define BIOS_CNTL_BIOSWE (1 << 0) +#define BIOS_CNTL_BLE (1 << 1) +#define BIOS_CNTL_SMM_BWP (1 << 5) + +static void spi_set_smm_only_flashing(bool enable) +{ + if (!(CONFIG(SOUTHBRIDGE_INTEL_I82801GX) || CONFIG(SOUTHBRIDGE_INTEL_COMMON_SPI_ICH9))) + return; + + const pci_devfn_t dev = PCI_DEV(0, 31, 0); + + uint8_t bios_cntl = pci_read_config8(dev, BIOS_CNTL); + + if (enable) { + bios_cntl &= ~BIOS_CNTL_BIOSWE; + bios_cntl |= BIOS_CNTL_BLE | BIOS_CNTL_SMM_BWP; + } else { + bios_cntl &= ~(BIOS_CNTL_BLE | BIOS_CNTL_SMM_BWP); + bios_cntl |= BIOS_CNTL_BIOSWE; + } + + pci_write_config8(dev, BIOS_CNTL, bios_cntl); +} + static const struct spi_ctrlr spi_ctrlr = { .xfer_vector = xfer_vectors, .max_xfer_size = member_size(struct ich9_spi_regs, fdata),
Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 21:
(1 comment)
Patchset:
PS21: make menuconfig and select: 1. Mainboard vendor (Intel) / (Alderlake-P RVP) 2. Security / and select "Intel CBnT support" 3. Security / and select "Boot media only writable in SMM"
then exit. It gives: *** ERROR: 4 warnings encountered, and warnings are errors. Your configuration changes were NOT saved.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40830 )
Change subject: security/intel: Add option to enable SMM flash access only ......................................................................
Patch Set 21:
(1 comment)
Patchset:
PS21:
make menuconfig and select: […]
What does this mean? Tried without CBnT?
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Benjamin Doron (Code Review)
-
Elyes Haouas (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)