From 9f8f11513a5db45b224f764525eae9c64fcfe360 Mon Sep 17 00:00:00 2001 From: Patrick Rudolph Date: Wed, 6 May 2020 11:58:45 +0200 Subject: 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 Signed-off-by: Christian Walter Reviewed-on: https://review.coreboot.org/c/coreboot/+/41086 Tested-by: build bot (Jenkins) Reviewed-by: Tim Wawrzynczak --- src/mainboard/lenovo/t60/smihandler.c | 37 +++++++++++++++++----------- src/soc/intel/baytrail/smihandler.c | 4 +++ src/soc/intel/braswell/smihandler.c | 4 +++ src/soc/intel/broadwell/smihandler.c | 8 ++++++ src/soc/intel/common/block/smm/smihandler.c | 4 +++ src/southbridge/intel/bd82x6x/smihandler.c | 7 ++++++ src/southbridge/intel/ibexpeak/smihandler.c | 4 +++ src/southbridge/intel/lynxpoint/smihandler.c | 4 +++ 8 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/mainboard/lenovo/t60/smihandler.c b/src/mainboard/lenovo/t60/smihandler.c index fe732a32b5..69ffe33209 100644 --- a/src/mainboard/lenovo/t60/smihandler.c +++ b/src/mainboard/lenovo/t60/smihandler.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -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 6f3f07e73d..1810821ec8 100644 --- a/src/soc/intel/baytrail/smihandler.c +++ b/src/soc/intel/baytrail/smihandler.c @@ -321,6 +321,10 @@ static void southbridge_smi_apmc(void) 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 a2c26c119f..d2f73bf654 100644 --- a/src/soc/intel/braswell/smihandler.c +++ b/src/soc/intel/braswell/smihandler.c @@ -301,6 +301,10 @@ static void southbridge_smi_apmc(void) 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 86be400e71..8dbb40f2b7 100644 --- a/src/soc/intel/broadwell/smihandler.c +++ b/src/soc/intel/broadwell/smihandler.c @@ -100,6 +100,10 @@ static void backlight_off(void) 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 @@ static void southbridge_smi_apmc(void) 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 7bd17c3e78..4998532837 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -373,6 +373,10 @@ void smihandler_southbridge_apmc( /* 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 8af14283c3..7211da37a9 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -103,6 +104,7 @@ static void xhci_sleep(u8 slp_typ) 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 @@ void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) 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 2bc31cf0cf..6c3f349ce3 100644 --- a/src/southbridge/intel/ibexpeak/smihandler.c +++ b/src/southbridge/intel/ibexpeak/smihandler.c @@ -152,6 +152,10 @@ void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) 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 bb05f99439..5ccb229805 100644 --- a/src/southbridge/intel/lynxpoint/smihandler.c +++ b/src/southbridge/intel/lynxpoint/smihandler.c @@ -314,6 +314,10 @@ static void southbridge_smi_apmc(void) 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); } -- cgit v1.2.3