<p>Raul Rangel has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/27229">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">smm: Add canary to end of stack and die() if a stack overflow occurs<br><br>If CPU 0's stack grows to large, it will overflow into CPU 1's stack.<br>If CPU 0 is handling the interrupt then CPU 1 should be in an idle loop.<br>When the stack overflow occurs it will override the return pointer for<br>CPU 1, so when CPU 0 unlocks the SMI lock, CPU 1 will attempt to return<br>to a random address.<br><br>This method is not full proof. If code allocates some stack variables<br>that overlap with the canary, and if the variables are never set, then<br>the canary will not be overwritten, but it will have been skipped. We<br>could mitigate this by adding a larger canary value if we wanted.<br><br>We can explore adding other methods of signaling that a stack overflow<br>had occurred in a follow up. I limited die() to debug only because<br>otherwise it would be very hard to track down.<br><br>TEST=built on grunt with a small and large stack size. Then verified<br>that one causes a stack overflow and the other does not.<br><br>Stack overflow message:<br>canary 0x0 != 0xcdeafc00<br>SMM Handler caused a stack overflow<br><br>Change-Id: I0184de7e3bfb84e0f74e1fa6a307633541f55612<br>Signed-off-by: Raul E Rangel <rrangel@chromium.org><br>---<br>M src/cpu/x86/smm/smm_module_handler.c<br>M src/cpu/x86/smm/smm_stub.S<br>M src/include/cpu/x86/smm.h<br>3 files changed, 28 insertions(+), 2 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/27229/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/cpu/x86/smm/smm_module_handler.c b/src/cpu/x86/smm/smm_module_handler.c</span><br><span>index c2001ec..6dff941 100644</span><br><span>--- a/src/cpu/x86/smm/smm_module_handler.c</span><br><span>+++ b/src/cpu/x86/smm/smm_module_handler.c</span><br><span>@@ -122,10 +122,13 @@</span><br><span> const struct smm_module_params *p;</span><br><span> const struct smm_runtime *runtime;</span><br><span> int cpu;</span><br><span style="color: hsl(120, 100%, 40%);">+ uintptr_t actual_canary;</span><br><span style="color: hsl(120, 100%, 40%);">+ uintptr_t expected_canary;</span><br><span> </span><br><span> p = arg;</span><br><span> runtime = p->runtime;</span><br><span> cpu = p->cpu;</span><br><span style="color: hsl(120, 100%, 40%);">+ expected_canary = (uintptr_t)p->canary;</span><br><span> </span><br><span> /* Make sure to set the global runtime. It's OK to race as the value</span><br><span> * will be the same across CPUs as well as multiple SMIs. */</span><br><span>@@ -171,6 +174,17 @@</span><br><span> </span><br><span> smi_restore_pci_address();</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ actual_canary = *p->canary;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (actual_canary != expected_canary) {</span><br><span style="color: hsl(120, 100%, 40%);">+ printk(BIOS_DEBUG, "canary 0x%lx != 0x%lx\n", actual_canary,</span><br><span style="color: hsl(120, 100%, 40%);">+ expected_canary);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ // Don't die if we can't indicate an error.</span><br><span style="color: hsl(120, 100%, 40%);">+ if (IS_ENABLED(CONFIG_DEBUG_SMI))</span><br><span style="color: hsl(120, 100%, 40%);">+ die("SMM Handler caused a stack overflow\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> smi_release_lock();</span><br><span> </span><br><span> /* De-assert SMI# signal to allow another SMI */</span><br><span>diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S</span><br><span>index 060361b..6bc06fc 100644</span><br><span>--- a/src/cpu/x86/smm/smm_stub.S</span><br><span>+++ b/src/cpu/x86/smm/smm_stub.S</span><br><span>@@ -136,6 +136,10 @@</span><br><span> subl %eax, %ebx # %ebx(stack_top) - %eax(offset) = %ebx(stack_top)</span><br><span> mov %ebx, %esp</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ movl stack_size, %eax</span><br><span style="color: hsl(120, 100%, 40%);">+ subl %eax, %ebx # %ebx(stack_top) - %eax(size) = %ebx(stack_bottom)</span><br><span style="color: hsl(120, 100%, 40%);">+ movl %ebx, (%ebx) # Write canary to the bottom of the stack.</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> pushl $0x0 # push a NULL stack base pointer</span><br><span> mov %esp, %ebp</span><br><span> </span><br><span>@@ -164,14 +168,18 @@</span><br><span> fxsave (%edi)</span><br><span> </span><br><span> 1:</span><br><span style="color: hsl(0, 100%, 40%);">- /* Align stack to 16 bytes. Another 16 bytes are pushed below. */</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Align stack to 16 bytes. Another 32 bytes are pushed below. */</span><br><span> andl $0xfffffff0, %esp</span><br><span> </span><br><span> /* Call into the c-based SMM relocation function with the platform</span><br><span> * parameters. Equivalent to:</span><br><span style="color: hsl(0, 100%, 40%);">- * struct arg = { c_handler_params, cpu_num, smm_runtime };</span><br><span style="color: hsl(120, 100%, 40%);">+ * struct arg = { c_handler_params, cpu_num, smm_runtime, canary };</span><br><span> * c_handler(&arg)</span><br><span> */</span><br><span style="color: hsl(120, 100%, 40%);">+ push $0x0 # Padding</span><br><span style="color: hsl(120, 100%, 40%);">+ push $0x0 # Padding</span><br><span style="color: hsl(120, 100%, 40%);">+ push $0x0 # Padding</span><br><span style="color: hsl(120, 100%, 40%);">+ push %ebx # uintptr_t *canary</span><br><span> push $(smm_runtime)</span><br><span> push %ecx # cpu</span><br><span> push c_handler_arg</span><br><span>diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h</span><br><span>index 7dcf4d7..9942772 100644</span><br><span>--- a/src/include/cpu/x86/smm.h</span><br><span>+++ b/src/include/cpu/x86/smm.h</span><br><span>@@ -527,6 +527,10 @@</span><br><span> void *arg;</span><br><span> int cpu;</span><br><span> const struct smm_runtime *runtime;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* A canary value that has been placed at the end of the stack.</span><br><span style="color: hsl(120, 100%, 40%);">+ * If (uintptr_t)canary != *canary then a stack overflow has occurred.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ const uintptr_t *canary;</span><br><span> };</span><br><span> </span><br><span> /* smm_handler_t is called with arg of smm_module_params pointer. */</span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/27229">change 27229</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/27229"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I0184de7e3bfb84e0f74e1fa6a307633541f55612 </div>
<div style="display:none"> Gerrit-Change-Number: 27229 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Raul Rangel <rrangel@chromium.org> </div>