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>

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 */