Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user provided pointers ......................................................................
SMM: Validate more user provided pointers
Validate user pointer using the introduced functions.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 80 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/1
diff --git a/src/mainboard/lenovo/t60/smihandler.c b/src/mainboard/lenovo/t60/smihandler.c index cb25590..58a2d11 100644 --- a/src/mainboard/lenovo/t60/smihandler.c +++ b/src/mainboard/lenovo/t60/smihandler.c @@ -3,6 +3,7 @@
#include <arch/io.h> #include <device/pci_ops.h> +#include <device/pci_def.h> #include <console/console.h> #include <cpu/x86/smm.h> #include <southbridge/intel/i82801gx/nvs.h> @@ -17,24 +18,32 @@
static void mainboard_smi_brightness_down(void) { - u8 *bar; - if ((bar = (u8 *)pci_read_config32(PCI_DEV(1, 0, 0), 0x18))) { - printk(BIOS_DEBUG, "bar: %08X, level %02X\n", (unsigned int)bar, *(bar+LVTMA_BL_MOD_LEVEL)); - *(bar+LVTMA_BL_MOD_LEVEL) &= 0xf0; - if (*(bar+LVTMA_BL_MOD_LEVEL) > 0x10) - *(bar+LVTMA_BL_MOD_LEVEL) -= 0x10; - } + uint32_t reg32 = pci_read_config32(PCI_DEV(1, 0, 0), PCI_BASE_ADDRESS_2) & ~0xf; + u8 *bar = (void *)(uintptr_t)reg32; + + /* Validate pointer before using it */ + if (!bar || smm_validate_pointer(bar)) + return; + + printk(BIOS_DEBUG, "bar: %p, level %02X\n", bar, *(bar+LVTMA_BL_MOD_LEVEL)); + *(bar+LVTMA_BL_MOD_LEVEL) &= 0xf0; + if (*(bar+LVTMA_BL_MOD_LEVEL) > 0x10) + *(bar+LVTMA_BL_MOD_LEVEL) -= 0x10; }
static void mainboard_smi_brightness_up(void) { - u8 *bar; - if ((bar = (u8 *)pci_read_config32(PCI_DEV(1, 0, 0), 0x18))) { - printk(BIOS_DEBUG, "bar: %08X, level %02X\n", (unsigned int)bar, *(bar+LVTMA_BL_MOD_LEVEL)); - *(bar+LVTMA_BL_MOD_LEVEL) |= 0x0f; - if (*(bar+LVTMA_BL_MOD_LEVEL) < 0xf0) - *(bar+LVTMA_BL_MOD_LEVEL) += 0x10; - } + uint32_t reg32 = pci_read_config32(PCI_DEV(1, 0, 0), PCI_BASE_ADDRESS_2) & ~0xf; + u8 *bar = (void *)(uintptr_t)reg32; + + /* Validate pointer before using it */ + if (!bar || smm_validate_pointer(bar)) + return; + + printk(BIOS_DEBUG, "bar: %p, level %02X\n", bar, *(bar+LVTMA_BL_MOD_LEVEL)); + *(bar+LVTMA_BL_MOD_LEVEL) |= 0x0f; + if (*(bar+LVTMA_BL_MOD_LEVEL) < 0xf0) + *(bar+LVTMA_BL_MOD_LEVEL) += 0x10; }
int mainboard_io_trap_handler(int smif) diff --git a/src/soc/intel/baytrail/smihandler.c b/src/soc/intel/baytrail/smihandler.c index 6ca4e64..fa493e2 100644 --- a/src/soc/intel/baytrail/smihandler.c +++ b/src/soc/intel/baytrail/smihandler.c @@ -5,6 +5,7 @@ #include <arch/io.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <cpu/x86/cache.h> #include <cpu/x86/smm.h> #include <cpu/intel/em64t100_save_state.h> @@ -332,6 +333,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (global_nvs_t *)((uint32_t)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + break; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/soc/intel/braswell/smihandler.c b/src/soc/intel/braswell/smihandler.c index 188c14e..4655c2b 100644 --- a/src/soc/intel/braswell/smihandler.c +++ b/src/soc/intel/braswell/smihandler.c @@ -6,6 +6,7 @@ #include <device/mmio.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <cpu/x86/cache.h> #include <cpu/x86/smm.h> #include <cpu/intel/em64t100_save_state.h> @@ -309,6 +310,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (global_nvs_t *)((uint32_t)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + break; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/soc/intel/broadwell/smihandler.c b/src/soc/intel/broadwell/smihandler.c index 1d92c12..65aff47 100644 --- a/src/soc/intel/broadwell/smihandler.c +++ b/src/soc/intel/broadwell/smihandler.c @@ -7,6 +7,7 @@ #include <device/mmio.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <cpu/x86/cache.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> @@ -111,6 +112,10 @@ reg_base = (void *)((uintptr_t)pci_read_config32(SA_DEV_IGD, PCI_BASE_ADDRESS_0) & ~0xf);
+ /* Validate pointer before using it */ + if (smm_validate_pointer(reg_base)) + return; + /* Check if backlight is enabled */ pp_ctrl = read32(reg_base + PCH_PP_CONTROL); if (!(pp_ctrl & EDP_BLC_ENABLE)) @@ -352,6 +357,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (global_nvs_t *)((u32)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + break; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index d8127a8..0771fc3 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -5,6 +5,7 @@ #include <arch/io.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <cpu/x86/cache.h> #include <cpu/x86/smm.h> #include <cpu/intel/em64t100_save_state.h> @@ -382,6 +383,11 @@ /* EBX in the state save contains the GNVS pointer */ uint32_t reg_ebx = save_state_ops->get_reg(state, RBX); gnvs = (struct global_nvs_t *)(uintptr_t)reg_ebx; + struct region r = {(uintptr_t)gnvs, sizeof(struct global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + break; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/southbridge/intel/bd82x6x/smihandler.c b/src/southbridge/intel/bd82x6x/smihandler.c index b2b635f..e14e781 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -5,6 +5,7 @@ #include <arch/io.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> #include <cpu/intel/em64t101_save_state.h> @@ -111,15 +112,17 @@ xhci_bar = pci_read_config32(PCH_XHCI_DEV, PCI_BASE_ADDRESS_0) & ~0xFUL;
- if ((xhci_bar + 0x4C0) & 1) - pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); - if ((xhci_bar + 0x4D0) & 1) - pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); - if ((xhci_bar + 0x4E0) & 1) - pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); - if ((xhci_bar + 0x4F0) & 1) - pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); - + if (!smm_validate_pointer((void *)(uintptr_t)xhci_bar)) { + /* FIXME: This looks broken */ + if ((xhci_bar + 0x4C0) & 1) + pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); + if ((xhci_bar + 0x4D0) & 1) + pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); + if ((xhci_bar + 0x4E0) & 1) + pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); + if ((xhci_bar + 0x4F0) & 1) + pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); + } reg32 = pci_read_config32(PCH_XHCI_DEV, PCI_COMMAND); reg32 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY); pci_write_config32(PCH_XHCI_DEV, PCI_COMMAND, reg32); @@ -204,6 +207,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (global_nvs_t *)((u32)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } *smm_done = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/southbridge/intel/ibexpeak/smihandler.c b/src/southbridge/intel/ibexpeak/smihandler.c index 05cd20f..3f0c3d3 100644 --- a/src/southbridge/intel/ibexpeak/smihandler.c +++ b/src/southbridge/intel/ibexpeak/smihandler.c @@ -5,6 +5,7 @@ #include <arch/io.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> #include <cpu/intel/em64t101_save_state.h> @@ -162,6 +163,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (global_nvs_t *)((u32)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } *smm_done = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/southbridge/intel/lynxpoint/smihandler.c b/src/southbridge/intel/lynxpoint/smihandler.c index 427c4fb..accd552 100644 --- a/src/southbridge/intel/lynxpoint/smihandler.c +++ b/src/southbridge/intel/lynxpoint/smihandler.c @@ -5,6 +5,7 @@ #include <arch/io.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <cpu/x86/cache.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> @@ -327,6 +328,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (global_nvs_t *)((u32)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(global_nvs_t)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + break; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user provided pointers ......................................................................
Patch Set 4: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG@7 PS4, Line 7: user provided user-provided
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG@9 PS4, Line 9: introduced IMHO 'newly-added' sounds better
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG@9 PS4, Line 9: user pointer user-provided pointers
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user provided pointers ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG@7 PS4, Line 7: user provided
user-provided
Done
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG@9 PS4, Line 9: introduced
IMHO 'newly-added' sounds better
Done
https://review.coreboot.org/c/coreboot/+/41086/4//COMMIT_MSG@9 PS4, Line 9: user pointer
user-provided pointers
Done
Christian Walter has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Validate user-provided pointers using the newly-added functions.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 80 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/6
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 6: Code-Review+2
Hello Philipp Deppenwiese, build bot (Jenkins), Christian Walter, Angel Pons, Alexander Couzens, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41086
to look at the new patch set (#8).
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Validate user-provided pointers using the newly-added functions.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 83 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/8
Hello Philipp Deppenwiese, build bot (Jenkins), Christian Walter, Angel Pons, Alexander Couzens, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41086
to look at the new patch set (#9).
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Validate user-provided pointers using the newly-added functions.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 80 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/9
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 9: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41086/9/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/9/src/southbridge/intel/bd82x... PS9, Line 114: looks Always a NOOP? All the checks below only do things if the xhci_bar is even which it cannot be.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41086/9/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/9/src/southbridge/intel/bd82x... PS9, Line 114: looks
Always a NOOP? All the checks below only do things if the xhci_bar is even which it cannot be.
Yes, that's fixed in CB:41852 as unrelated to pointer validation
Hello Philipp Deppenwiese, build bot (Jenkins), Christian Walter, Angel Pons, Arthur Heymans, Alexander Couzens, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41086
to look at the new patch set (#11).
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "YuriyBulygin" and "OleksandrBazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/11
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 11: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41086/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/11//COMMIT_MSG@9 PS11, Line 9: Migitate Mitigate
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41086/11/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/11/src/soc/intel/braswell/smi... PS11, Line 306: return; Hmm.. we seem to call smm_setup_structures() late in ramstage, and reach here. If smi_apmc_find_state_save() does not fail smm_initialized will be set before we reach payload. So the pointer was on a non-reachable path anyways?
Hello Piotr Kleinschmidt, build bot (Jenkins), Frans Hendriks, Stefan Reinauer, Duncan Laurie, Angel Pons, Julius Werner, Arthur Heymans, Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Philipp Deppenwiese, Michał Żygowski, Tim Wawrzynczak, Christian Walter, Alexander Couzens, Furquan Shaikh, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41086
to look at the new patch set (#12).
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/12
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41086/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/12//COMMIT_MSG@9 PS12, Line 9: Migitate nit: Mitigate
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 7: #include <commonlib/region.h> Why? Same in couple other files.
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... PS12, Line 7: #include <commonlib/region.h> This one is fine.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 12:
(5 comments)
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break should this return instead?
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/braswell/smi... PS12, Line 314: break should this return instead?
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/broadwell/sm... File src/soc/intel/broadwell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/broadwell/sm... PS12, Line 361: break return?
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/common/block... PS12, Line 387: break return?
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/lynx... File src/southbridge/intel/lynxpoint/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/lynx... PS12, Line 332: break return?
Christian Walter has uploaded a new patch set (#13) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/13
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 13:
CB:42427
All those APM_CNT_GNVS_UPDATE cases will be gone.
Christian Walter has uploaded a new patch set (#14) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/14
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 13:
Patch Set 13:
CB:42427
All those APM_CNT_GNVS_UPDATE cases will be gone.
Alright - I will update this once it has been merged into the tree ;)
Christian Walter has uploaded a new patch set (#15) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/15
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41086/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/11//COMMIT_MSG@9 PS11, Line 9: Migitate
Mitigate
see latest patchset
https://review.coreboot.org/c/coreboot/+/41086/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/15//COMMIT_MSG@9 PS15, Line 9: Migitate Mitigate
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 7: #include <commonlib/region.h>
Why? Same in couple other files.
It's for checking whether the pointer overlaps the SMRAM region, AFAIUI
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break
should this return instead?
Definitely
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 7: #include <commonlib/region.h>
It's for checking whether the pointer overlaps the SMRAM region, AFAIUI
Did you read my comment in sb/bd82x6x. That one actually needs struct region.
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break
Definitely
Why definetely? The construction is generally that APMC_CNT is passed thru both sb/ an mb/ handler.
This APM_CNT_GNVS_UPDATE will be gone in couple weeks or less. Seems like Christian already agreed to wait for that to land first.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 7: #include <commonlib/region.h>
Did you read my comment in sb/bd82x6x. That one actually needs struct region.
Oh, I thought smm_points_to_smram was in this header. Sorry for the noise, it's actually in src/include/cpu/x86/smm.h
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break
Why definetely? The construction is generally that APMC_CNT is passed thru both sb/ an mb/ handler. […]
Not returning when the pointer is invalid is asking for security troubles, AFAIK. In any case the only baytail board does not handle this case anyway.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15:
I no longer have a schedule for CB:42427 so you may want to fix the few small issues here instead of waiting for that to be rebased and submitted. Could be weeks or months.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 15: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break
Not returning when the pointer is invalid is asking for security troubles, AFAIK. […]
It should definitely return. That was the point the authors were making in their presentation; we should detect if addresses (that are overwritable by another agent) are attempting to trick the SMI handler in to overwriting an arbitrary address.
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... PS12, Line 113: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar, I think it may be safer to do this check at the top of the case statement. If the xhci bar has been overwritten to point to smram, then something is very wrong and it should just return.
Christian Walter has uploaded a new patch set (#16) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/16
Christian Walter has uploaded a new patch set (#17) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 75 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/17
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 17:
(13 comments)
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... File src/mainboard/lenovo/t60/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... PS17, Line 20: ~0xf What about a #define for this (very common) mask? #define PCI_BASE_ADDRESS_MASK ~0xF something like that in pci.h ?
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 331: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 8: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 307: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... File src/soc/intel/broadwell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 9: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 351: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... PS17, Line 379: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 107: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar, : 0x4F0 + sizeof(uint32_t))) { : /* FIXME: This looks broken */ : if ((xhci_bar + 0x4C0) & 1) : pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4D0) & 1) : pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4E0) & 1) : pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4F0) & 1) : pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); : } Would prefer if this just return'ed instead of conditionally executing these. If the pointers overlap, something is broken and very wrong, or there's a rootkit.
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/ibex... File src/southbridge/intel/ibexpeak/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/ibex... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... File src/southbridge/intel/lynxpoint/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 323: break return
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... File src/mainboard/lenovo/t60/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... PS17, Line 27: *(bar+LVTMA_BL_MOD_LEVEL) This should really be using read8/write8 functions
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 107: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar, : 0x4F0 + sizeof(uint32_t))) { : /* FIXME: This looks broken */ : if ((xhci_bar + 0x4C0) & 1) : pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4D0) & 1) : pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4E0) & 1) : pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4F0) & 1) : pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); : }
Would prefer if this just return'ed instead of conditionally executing these. […]
I'd just drop this, it's broken
Angel Pons has uploaded a new patch set (#18) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Migitate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 58 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/18
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 18:
(23 comments)
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... File src/mainboard/lenovo/t60/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... PS17, Line 20: ~0xf
What about a #define for this (very common) mask? […]
I think we have better ways to get a PCI base address, but that's for another patch
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... PS17, Line 27: *(bar+LVTMA_BL_MOD_LEVEL)
This should really be using read8/write8 functions
Not trivial to do with the operations below, better to handle in a separate patch
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 7: #include <commonlib/region.h>
Oh, I thought smm_points_to_smram was in this header. […]
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break
It should definitely return. […]
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 7: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 331: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/11/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/11/src/soc/intel/braswell/smi... PS11, Line 306: return;
Hmm.. we seem to call smm_setup_structures() late in ramstage, and reach here. […]
Ack
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/braswell/smi... PS12, Line 314: break
should this return instead?
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 8: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 307: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/broadwell/sm... File src/soc/intel/broadwell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/broadwell/sm... PS12, Line 361: break
return?
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... File src/soc/intel/broadwell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 9: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 351: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/common/block... PS12, Line 387: break
return?
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... PS17, Line 7: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... PS17, Line 379: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... PS12, Line 113: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar,
I think it may be safer to do this check at the top of the case statement. […]
The conditions are always false. Added a FIXME comment.
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 7: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 107: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar, : 0x4F0 + sizeof(uint32_t))) { : /* FIXME: This looks broken */ : if ((xhci_bar + 0x4C0) & 1) : pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4D0) & 1) : pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4E0) & 1) : pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4F0) & 1) : pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); : }
I'd just drop this, it's broken
Added a comment for now, the conditions are always false
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/ibex... File src/southbridge/intel/ibexpeak/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/ibex... PS17, Line 7: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/lynx... File src/southbridge/intel/lynxpoint/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/lynx... PS12, Line 332: break
return?
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... File src/southbridge/intel/lynxpoint/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 7: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 323: break
return
Done
Angel Pons has uploaded a new patch set (#20) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Mitigate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 58 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/20
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41086/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41086/15//COMMIT_MSG@9 PS15, Line 9: Migitate
Mitigate
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 7: #include <commonlib/region.h>
Done
(stupid gerrit didn't save my message properly, retyping...)
We use the `struct region` type at the end of this file. That's why we're including it.
Angel Pons has uploaded a new patch set (#21) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Mitigate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 58 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41086/21
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 21: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
SMM: Validate more user-provided pointers
Mitigate issues presented in "Digging Into The Core of Boot" found by "Yuriy Bulygin" and "Oleksandr Bazhaniuk" at RECON-MTL-2017.
Validate user-provided pointers using the newly-added functions. This protects SMM from ring0 attacks.
Change-Id: I8a347ccdd20816924bf1bceb3b24bf7b22309312 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/+/41086 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/lenovo/t60/smihandler.c M src/soc/intel/baytrail/smihandler.c M src/soc/intel/braswell/smihandler.c M src/soc/intel/broadwell/smihandler.c M src/soc/intel/common/block/smm/smihandler.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 8 files changed, 58 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/lenovo/t60/smihandler.c b/src/mainboard/lenovo/t60/smihandler.c index fe732a3..69ffe33 100644 --- a/src/mainboard/lenovo/t60/smihandler.c +++ b/src/mainboard/lenovo/t60/smihandler.c @@ -2,6 +2,7 @@
#include <arch/io.h> #include <device/pci_ops.h> +#include <device/pci_def.h> #include <console/console.h> #include <cpu/x86/smm.h> #include <southbridge/intel/i82801gx/nvs.h> @@ -16,24 +17,32 @@
static void mainboard_smi_brightness_down(void) { - u8 *bar; - if ((bar = (u8 *)pci_read_config32(PCI_DEV(1, 0, 0), 0x18))) { - printk(BIOS_DEBUG, "bar: %08X, level %02X\n", (unsigned int)bar, *(bar+LVTMA_BL_MOD_LEVEL)); - *(bar+LVTMA_BL_MOD_LEVEL) &= 0xf0; - if (*(bar+LVTMA_BL_MOD_LEVEL) > 0x10) - *(bar+LVTMA_BL_MOD_LEVEL) -= 0x10; - } + uint32_t reg32 = pci_read_config32(PCI_DEV(1, 0, 0), PCI_BASE_ADDRESS_2) & ~0xf; + u8 *bar = (void *)(uintptr_t)reg32; + + /* Validate pointer before using it */ + if (!bar || smm_points_to_smram(bar, LVTMA_BL_MOD_LEVEL + sizeof(uint8_t))) + return; + + printk(BIOS_DEBUG, "bar: %p, level %02X\n", bar, *(bar+LVTMA_BL_MOD_LEVEL)); + *(bar+LVTMA_BL_MOD_LEVEL) &= 0xf0; + if (*(bar+LVTMA_BL_MOD_LEVEL) > 0x10) + *(bar+LVTMA_BL_MOD_LEVEL) -= 0x10; }
static void mainboard_smi_brightness_up(void) { - u8 *bar; - if ((bar = (u8 *)pci_read_config32(PCI_DEV(1, 0, 0), 0x18))) { - printk(BIOS_DEBUG, "bar: %08X, level %02X\n", (unsigned int)bar, *(bar+LVTMA_BL_MOD_LEVEL)); - *(bar+LVTMA_BL_MOD_LEVEL) |= 0x0f; - if (*(bar+LVTMA_BL_MOD_LEVEL) < 0xf0) - *(bar+LVTMA_BL_MOD_LEVEL) += 0x10; - } + uint32_t reg32 = pci_read_config32(PCI_DEV(1, 0, 0), PCI_BASE_ADDRESS_2) & ~0xf; + u8 *bar = (void *)(uintptr_t)reg32; + + /* Validate pointer before using it */ + if (!bar || smm_points_to_smram(bar, LVTMA_BL_MOD_LEVEL + sizeof(uint8_t))) + return; + + printk(BIOS_DEBUG, "bar: %p, level %02X\n", bar, *(bar+LVTMA_BL_MOD_LEVEL)); + *(bar+LVTMA_BL_MOD_LEVEL) |= 0x0f; + if (*(bar+LVTMA_BL_MOD_LEVEL) < 0xf0) + *(bar+LVTMA_BL_MOD_LEVEL) += 0x10; }
int mainboard_io_trap_handler(int smif) diff --git a/src/soc/intel/baytrail/smihandler.c b/src/soc/intel/baytrail/smihandler.c index 6f3f07e..1810821 100644 --- a/src/soc/intel/baytrail/smihandler.c +++ b/src/soc/intel/baytrail/smihandler.c @@ -321,6 +321,10 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (struct global_nvs *)((uint32_t)state->rbx); + if (smm_points_to_smram(gnvs, sizeof(*gnvs))) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/soc/intel/braswell/smihandler.c b/src/soc/intel/braswell/smihandler.c index a2c26c11..d2f73bf 100644 --- a/src/soc/intel/braswell/smihandler.c +++ b/src/soc/intel/braswell/smihandler.c @@ -301,6 +301,10 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (struct global_nvs *)((uint32_t)state->rbx); + if (smm_points_to_smram(gnvs, sizeof(*gnvs))) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/soc/intel/broadwell/smihandler.c b/src/soc/intel/broadwell/smihandler.c index 86be400..8dbb40f 100644 --- a/src/soc/intel/broadwell/smihandler.c +++ b/src/soc/intel/broadwell/smihandler.c @@ -100,6 +100,10 @@ reg_base = (void *)((uintptr_t)pci_read_config32(SA_DEV_IGD, PCI_BASE_ADDRESS_0) & ~0xf);
+ /* Validate pointer before using it */ + if (smm_points_to_smram(reg_base, PCH_PP_OFF_DELAYS + sizeof(uint32_t))) + return; + /* Check if backlight is enabled */ pp_ctrl = read32(reg_base + PCH_PP_CONTROL); if (!(pp_ctrl & EDP_BLC_ENABLE)) @@ -341,6 +345,10 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (struct global_nvs *)((u32)state->rbx); + if (smm_points_to_smram(gnvs, sizeof(*gnvs))) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index 7bd17c3..4998532 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -373,6 +373,10 @@ /* EBX in the state save contains the GNVS pointer */ uint32_t reg_ebx = save_state_ops->get_reg(state, RBX); gnvs = (struct global_nvs *)(uintptr_t)reg_ebx; + if (smm_points_to_smram(gnvs, sizeof(*gnvs))) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/southbridge/intel/bd82x6x/smihandler.c b/src/southbridge/intel/bd82x6x/smihandler.c index 8af1428..7211da3 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -4,6 +4,7 @@ #include <arch/io.h> #include <device/pci_ops.h> #include <console/console.h> +#include <commonlib/region.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> #include <cpu/intel/em64t101_save_state.h> @@ -103,6 +104,7 @@
xhci_bar = pci_read_config32(PCH_XHCI_DEV, PCI_BASE_ADDRESS_0) & ~0xFUL;
+ /* FIXME: This looks broken (conditions are always false) */ if ((xhci_bar + 0x4C0) & 1) pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); if ((xhci_bar + 0x4D0) & 1) @@ -191,6 +193,11 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (struct global_nvs *)((u32)state->rbx); + struct region r = {(uintptr_t)gnvs, sizeof(struct global_nvs)}; + if (smm_region_overlaps_handler(&r)) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } *smm_done = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/southbridge/intel/ibexpeak/smihandler.c b/src/southbridge/intel/ibexpeak/smihandler.c index 2bc31cf..6c3f349 100644 --- a/src/southbridge/intel/ibexpeak/smihandler.c +++ b/src/southbridge/intel/ibexpeak/smihandler.c @@ -152,6 +152,10 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (struct global_nvs *)((u32)state->rbx); + if (smm_points_to_smram(gnvs, sizeof(*gnvs))) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } *smm_done = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); } diff --git a/src/southbridge/intel/lynxpoint/smihandler.c b/src/southbridge/intel/lynxpoint/smihandler.c index bb05f99..5ccb229 100644 --- a/src/southbridge/intel/lynxpoint/smihandler.c +++ b/src/southbridge/intel/lynxpoint/smihandler.c @@ -314,6 +314,10 @@ if (state) { /* EBX in the state save contains the GNVS pointer */ gnvs = (struct global_nvs *)((u32)state->rbx); + if (smm_points_to_smram(gnvs, sizeof(*gnvs))) { + printk(BIOS_ERR, "SMI#: ERROR: GNVS overlaps SMM\n"); + return; + } smm_initialized = 1; printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Christian Walter (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Wim Vervoorn (Code Review)