[coreboot-gerrit] Change in coreboot[master]: smm: Add canary to end of stack and die() if a stack overflow occurs

[Raul Rangel (Code Review)]

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/27229


Change subject: smm: Add canary to end of stack and die() if a stack overflow occurs
......................................................................

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 full proof. 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.

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 at chromium.org>
---
M src/cpu/x86/smm/smm_module_handler.c
M src/cpu/x86/smm/smm_stub.S
M src/include/cpu/x86/smm.h
3 files changed, 28 insertions(+), 2 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/27229/1

diff --git a/src/cpu/x86/smm/smm_module_handler.c b/src/cpu/x86/smm/smm_module_handler.c
index c2001ec..6dff941 100644
--- a/src/cpu/x86/smm/smm_module_handler.c
+++ b/src/cpu/x86/smm/smm_module_handler.c
@@ -122,10 +122,13 @@
 	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 @@
 
 	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 060361b..6bc06fc 100644
--- a/src/cpu/x86/smm/smm_stub.S
+++ b/src/cpu/x86/smm/smm_stub.S
@@ -136,6 +136,10 @@
 	subl	%eax, %ebx # %ebx(stack_top) - %eax(offset) = %ebx(stack_top)
 	mov	%ebx, %esp
 
+	movl	stack_size, %eax
+	subl	%eax, %ebx # %ebx(stack_top) - %eax(size) = %ebx(stack_bottom)
+	movl	%ebx, (%ebx) # Write canary to the bottom of the stack.
+
 	pushl	$0x0 # push a NULL stack base pointer
 	mov	%esp, %ebp
 
@@ -164,14 +168,18 @@
 	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 # cpu
 	push	c_handler_arg
diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h
index 7dcf4d7..9942772 100644
--- a/src/include/cpu/x86/smm.h
+++ b/src/include/cpu/x86/smm.h
@@ -527,6 +527,10 @@
 	void *arg;
 	int cpu;
 	const struct smm_runtime *runtime;
+	/* A canary value that has been placed at the end of the stack.
+	 * If (uintptr_t)canary != *canary then a stack overflow has occurred.
+	 */
+	const uintptr_t *canary;
 };
 
 /* smm_handler_t is called with arg of smm_module_params pointer. */

-- 
To view, visit https://review.coreboot.org/27229
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0184de7e3bfb84e0f74e1fa6a307633541f55612
Gerrit-Change-Number: 27229
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel at chromium.org>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180625/acddce53/attachment-0001.html>