Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37289 )
Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ......................................................................
cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub
This adds a check for LAPIC base twice. One very early check when the CPU is still executing in real mode checks if the LAPIC base is inside the region [smmbase,smmbase + SMM_DEFAULT_SIZE). This cannot use anything but a hardcoded size since even accessing the relocatable parameters is impossible in the state of the CPU.
The actual SMI handler is located above smmbase + SMM_DEFAULT_SIZE and before jumping to it the LAPIC base is checked against the whole SMM region. Given that we have a working stack at this point, this is done in C code.
UNTESTED.
Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/sinkhole.c M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 4 files changed, 87 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/1
diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index 11a4e67..b94f11e 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -45,6 +45,7 @@ postcar-y += tseg_region.c
smmstub-y += smm_stub.S +smmstub-y += sinkhole.c
smm-y += smm_module_handler.c
diff --git a/src/cpu/x86/smm/sinkhole.c b/src/cpu/x86/smm/sinkhole.c new file mode 100644 index 0000000..773549c --- /dev/null +++ b/src/cpu/x86/smm/sinkhole.c @@ -0,0 +1,34 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <cpu/x86/lapic_def.h> +#include <cpu/x86/msr.h> +#include <cpu/x86/smm.h> + +const extern struct smm_runtime smm_runtime; + +/* Don't try to recover because, if somebody tried to do shenanigans like + these, we have to expect more. */ +void mitigate_sinkhole(void) +{ + msr_t lapic_base_msr = rdmsr(LAPIC_BASE_MSR); + uintptr_t lapic_base = lapic_base_msr.lo & LAPIC_BASE_MSR_ADDR_MASK; + + const uintptr_t smm_end = smm_runtime.smbase + smm_runtime.smm_size; + + if (lapic_base > smm_runtime.smbase && lapic_base < smm_end) { + printk(BIOS_EMERG, "Wrong LAPIC base detected! dying...\n"); + asm("ud2"); + } +} diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 304ea4b..1d39ae1 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -22,6 +22,7 @@ */
#include <cpu/x86/cr.h> +#include <cpu/x86/lapic_def.h>
.code32 .section ".module_parameters", "aw", @progbits @@ -39,6 +40,7 @@ fxsave_area_size: .long 0 /* struct smm_runtime begins here. */ +.global smm_runtime smm_runtime: smbase: .long 0 @@ -63,10 +65,51 @@ (CR0_CD | CR0_NW | CR0_PG | CR0_AM | CR0_WP | \ CR0_NE | CR0_TS | CR0_EM | CR0_MP)
+#define SMM_DEFAULT_SIZE 0x10000 + .text .code16 .global _start _start: +#if CONFIG(SMM_LAPIC_REMAP_MITIGATION) + /* Check if the LAPIC register block overlaps with SMM. + * This block needs to work without data accesses because they + * may be routed into the LAPIC register block. + * Code accesses, on the other hand, are never routed to LAPIC, + * which is what makes this work in the first place. + * This is a mitigation against the sinkhole vulnerability + * possible on pre sandy bridge Intel hardware. + */ + mov $LAPIC_BASE_MSR, %ecx + rdmsr + and $(~0xfff), %eax + sub $_start, %eax + /* This might cover a bit more than needed but in the case that + this is a stub to the relocated, permanent handler it should + cover some parts of the permanent handler too, so no harm is + done. In the case that this is a stub to a relocation handler + coreboot did something horribly wrong if LAPIC is here. + This has to be 'hardcoded' as the CPU is running in real mode + at this point so it cannot access any relocatable variable. */ + cmp $SMM_DEFAULT_SIZE, %eax + ja untampered_lapic +1: + /* emit "Crash" on serial */ + mov $(CONFIG_TTYS0_BASE), %dx + mov $'C', %al + out %al, (%dx) + mov $'r', %al + out %al, (%dx) + mov $'a', %al + out %al, (%dx) + mov $'s', %al + out %al, (%dx) + mov $'h', %al + out %al, (%dx) + /* now crash for real */ + ud2 +untampered_lapic: +#endif movl $(smm_relocate_gdt), %ebx lgdtl (%ebx)
@@ -174,6 +217,12 @@ /* Align stack to 16 bytes. Another 32 bytes are pushed below. */ andl $0xfffffff0, %esp
+#if CONFIG(SMM_LAPIC_REMAP_MITIGATION) + /* Now that we have a stack, check for against lapic base in + C code before jumping to the permanent handler */ + call mitigate_sinkhole +#endif + /* Call into the c-based SMM relocation function with the platform * parameters. Equivalent to: * struct arg = { c_handler_params, cpu_num, smm_runtime, canary }; diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index 2e3c639..884d5cb 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -89,6 +89,9 @@
/* SMM Runtime helpers. */
+/* SMM Runtime sinkhole mitigation check */ +void mitigate_sinkhole(void); + /* Entry point for SMM modules. */ asmlinkage void smm_handler_start(void *params);