Patchwork fix potential smm security hole

login
register
about
Submitter Stefan Reinauer
Date 2010-07-29 15:47:12
Message ID <4C51A280.4040104@coresystems.de>
Download mbox | patch
Permalink /patch/1697/
State Accepted
Commit r5676
Headers show

Comments

Stefan Reinauer - 2010-07-29 15:47:12
This patch resulted from a security review of coreboot's SMM handler.
Feedback appreciated.

Regards,
Stefan
- fix SMM code relocation race
- make SMM relocation debugging Kconfig accessible

Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Joseph Smith - 2010-07-29 15:55:03
On Thu, 29 Jul 2010 17:47:12 +0200, Stefan Reinauer
<stefan.reinauer@coresystems.de> wrote:
>  This patch resulted from a security review of coreboot's SMM handler.
> Feedback appreciated.
> 
> Regards,
> Stefan

Hello Stefan,
I thought the SMM Handler already lived at 0xa0000 instead of TSEG or HSEG.
I don't really understand where the security hole is?
Can you explain a little more in depth?
Patrick Georgi - 2010-07-29 16:01:37
Am 29.07.2010 17:47, schrieb Stefan Reinauer:
>  This patch resulted from a security review of coreboot's SMM handler.
> Feedback appreciated.
Nice patch!

Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>

Patch

Index: src/southbridge/intel/i82801dx/i82801dx_smi.c
===================================================================
--- src/southbridge/intel/i82801dx/i82801dx_smi.c	(revision 5672)
+++ src/southbridge/intel/i82801dx/i82801dx_smi.c	(working copy)
@@ -335,11 +335,13 @@ 
 
 void smm_init(void)
 {
-	// FIXME is this a race condition?
-	smm_relocate();
+	/* Put SMM code to 0xa0000 */
 	smm_install();
 
-	// We're done. Make sure SMIs can happen!
+	/* Put relocation code to 0x38000 and relocate SMBASE */
+	smm_relocate();
+
+	/* We're done. Make sure SMIs can happen! */
 	smi_set_eos();
 }
 
Index: src/southbridge/intel/i82801dx/i82801dx.h
===================================================================
--- src/southbridge/intel/i82801dx/i82801dx.h	(revision 5672)
+++ src/southbridge/intel/i82801dx/i82801dx.h	(working copy)
@@ -86,6 +86,7 @@ 
 #define PCICMD          0x04
 #define PMBASE          0x40
 #define   PMBASE_ADDR	0x0400
+#define   DEFAULT_PMBASE PMBASE_ADDR
 #define ACPI_CNTL       0x44
 #define BIOS_CNTL       0x4E
 #define GPIO_BASE       0x58
Index: src/southbridge/intel/i82801gx/i82801gx_smi.c
===================================================================
--- src/southbridge/intel/i82801gx/i82801gx_smi.c	(revision 5672)
+++ src/southbridge/intel/i82801gx/i82801gx_smi.c	(working copy)
@@ -335,11 +335,13 @@ 
 
 void smm_init(void)
 {
-	// FIXME is this a race condition?
-	smm_relocate();
+	/* Put SMM code to 0xa0000 */
 	smm_install();
 
-	// We're done. Make sure SMIs can happen!
+	/* Put relocation code to 0x38000 and relocate SMBASE */
+	smm_relocate();
+
+	/* We're done. Make sure SMIs can happen! */
 	smi_set_eos();
 }
 
Index: src/Kconfig
===================================================================
--- src/Kconfig	(revision 5672)
+++ src/Kconfig	(working copy)
@@ -560,6 +560,18 @@ 
 
 	  If unsure, say N.
 
+config DEBUG_SMM_RELOCATION
+	bool "Debug SMM relocation code"
+	default n
+	depends on HAVE_SMI_HANDLER
+	help
+	  This option enables additional SMM handler relocation related
+	  debug messages.
+
+	  Note: This option will increase the size of the coreboot image.
+
+	  If unsure, say N.
+
 config X86EMU_DEBUG
 	bool "Output verbose x86emu debug messages"
 	default n
Index: src/cpu/x86/smm/smmrelocate.S
===================================================================
--- src/cpu/x86/smm/smmrelocate.S	(revision 5672)
+++ src/cpu/x86/smm/smmrelocate.S	(working copy)
@@ -1,7 +1,7 @@ 
 /*
  * This file is part of the coreboot project.
  *
- * Copyright (C) 2008 coresystems GmbH
+ * Copyright (C) 2008-2010 coresystems GmbH
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -24,13 +24,16 @@ 
 
 // FIXME: Is this piece of code southbridge specific, or
 // can it be cleaned up so this include is not required?
-// It's needed right now because we get our PM_BASE from
+// It's needed right now because we get our DEFAULT_PMBASE from
 // here.
+#if defined(CONFIG_SOUTHBRIDGE_INTEL_I82801GX)
 #include "../../../southbridge/intel/i82801gx/i82801gx.h"
+#elif defined(CONFIG_SOUTHBRIDGE_INTEL_I82801DX)
+#include "../../../southbridge/intel/i82801dx/i82801dx.h"
+#else
+#error "Southbridge needs SMM handler support."
+#endif
 
-#undef DEBUG_SMM_RELOCATION
-//#define DEBUG_SMM_RELOCATION
-
 #define LAPIC_ID 0xfee00020
 
 .global smm_relocation_start
@@ -125,7 +128,7 @@ 
 	addr32 movl %eax, (%ebx)
 
 
-	/* The next section of code is hardware specific */
+	/* The next section of code is potentially southbridge specific */
 
 	/* Clear SMI status */
 	movw $(DEFAULT_PMBASE + 0x34), %dx
@@ -143,8 +146,9 @@ 
 	orl $(1 << 1), %eax
 	outl %eax, %dx
 
-	/* End of hardware specific section. */
-#ifdef DEBUG_SMM_RELOCATION
+	/* End of southbridge specific section. */
+
+#if defined(CONFIG_DEBUG_SMM_RELOCATION) && CONFIG_DEBUG_SMM_RELOCATION
 	/* print [SMM-x] so we can determine if CPUx went to SMM */
 	movw $CONFIG_TTYS0_BASE, %dx
 	mov $'[', %al