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); }