summaryrefslogtreecommitdiff
path: root/src/cpu/x86
diff options
context:
space:
mode:
authorRaul E Rangel <rrangel@chromium.org>2018-06-25 14:22:27 -0600
committerPatrick Georgi <pgeorgi@google.com>2018-06-28 09:01:02 +0000
commiteb5d76a510d7a4b46e6b33d8a697a30510a0a6d7 (patch)
tree50cde77d90a3f5668fb57aff13a687fb3581c112 /src/cpu/x86
parentf78f97e156f3ec71c9ec62cac4cf8954728cddf8 (diff)
downloadcoreboot-eb5d76a510d7a4b46e6b33d8a697a30510a0a6d7.tar.xz
smm: Add canary to end of stack and die() if a stack overflow occurs
If CPU 0's stack grows to large, it will overflow into CPU 1's stack. If CPU 0 is handling the interrupt then CPU 1 should be in an idle loop. When the stack overflow occurs it will override the return pointer for CPU 1, so when CPU 0 unlocks the SMI lock, CPU 1 will attempt to return to a random address. This method is not foolproof. If code allocates some stack variables that overlap with the canary, and if the variables are never set, then the canary will not be overwritten, but it will have been skipped. We could mitigate this by adding a larger canary value if we wanted. I chose to use the stack bottom pointer value as the canary value because: * It will change per CPU stack. * Doesn't require hard coding a value that must be shared between the .S and .c. * Passing the expected canary value as a parameter felt like overkill. We can explore adding other methods of signaling that a stack overflow had occurred in a follow up. I limited die() to debug only because otherwise it would be very hard to track down. TEST=built on grunt with a small and large stack size. Then verified that one causes a stack overflow and the other does not. Stack overflow message: canary 0x0 != 0xcdeafc00 SMM Handler caused a stack overflow Change-Id: I0184de7e3bfb84e0f74e1fa6a307633541f55612 Signed-off-by: Raul E Rangel <rrangel@chromium.org> Reviewed-on: https://review.coreboot.org/27229 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Diffstat (limited to 'src/cpu/x86')
-rw-r--r--src/cpu/x86/smm/smm_module_handler.c14
-rw-r--r--src/cpu/x86/smm/smm_stub.S13
2 files changed, 25 insertions, 2 deletions
diff --git a/src/cpu/x86/smm/smm_module_handler.c b/src/cpu/x86/smm/smm_module_handler.c
index c2001ec9e2..6dff9411de 100644
--- a/src/cpu/x86/smm/smm_module_handler.c
+++ b/src/cpu/x86/smm/smm_module_handler.c
@@ -122,10 +122,13 @@ asmlinkage void smm_handler_start(void *arg)
const struct smm_module_params *p;
const struct smm_runtime *runtime;
int cpu;
+ uintptr_t actual_canary;
+ uintptr_t expected_canary;
p = arg;
runtime = p->runtime;
cpu = p->cpu;
+ expected_canary = (uintptr_t)p->canary;
/* Make sure to set the global runtime. It's OK to race as the value
* will be the same across CPUs as well as multiple SMIs. */
@@ -171,6 +174,17 @@ asmlinkage void smm_handler_start(void *arg)
smi_restore_pci_address();
+ actual_canary = *p->canary;
+
+ if (actual_canary != expected_canary) {
+ printk(BIOS_DEBUG, "canary 0x%lx != 0x%lx\n", actual_canary,
+ expected_canary);
+
+ // Don't die if we can't indicate an error.
+ if (IS_ENABLED(CONFIG_DEBUG_SMI))
+ die("SMM Handler caused a stack overflow\n");
+ }
+
smi_release_lock();
/* De-assert SMI# signal to allow another SMI */
diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S
index ba66db9179..eb890df150 100644
--- a/src/cpu/x86/smm/smm_stub.S
+++ b/src/cpu/x86/smm/smm_stub.S
@@ -136,6 +136,11 @@ smm_trampoline32:
subl %eax, %ebx /* global_stack_top - offset = stack_top */
mov %ebx, %esp
+ /* Write canary to the bottom of the stack */
+ movl stack_size, %eax
+ subl %eax, %ebx /* %ebx(stack_top) - size = %ebx(stack_bottom) */
+ movl %ebx, (%ebx)
+
/* Create stack frame by pushing a NULL stack base pointer */
pushl $0x0
mov %esp, %ebp
@@ -166,14 +171,18 @@ smm_trampoline32:
fxsave (%edi)
1:
- /* Align stack to 16 bytes. Another 16 bytes are pushed below. */
+ /* Align stack to 16 bytes. Another 32 bytes are pushed below. */
andl $0xfffffff0, %esp
/* Call into the c-based SMM relocation function with the platform
* parameters. Equivalent to:
- * struct arg = { c_handler_params, cpu_num, smm_runtime };
+ * struct arg = { c_handler_params, cpu_num, smm_runtime, canary };
* c_handler(&arg)
*/
+ push $0x0 /* Padding */
+ push $0x0 /* Padding */
+ push $0x0 /* Padding */
+ push %ebx /* uintptr_t *canary */
push $(smm_runtime)
push %ecx /* int cpu */
push c_handler_arg /* void *arg */