[PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable()

* [PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable()
@ 2017-06-19 10:24 Star Zeng
       [not found] ` <0C09AFA07DD0434D9E2A0C6AEB0483103B8E9423@shsmsx102.ccr.corp.intel.com>
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Star Zeng @ 2017-06-19 10:24 UTC (permalink / raw)
  To: edk2-devel; +Cc: Shi, Steven, Jiewen Yao, Liming Gao, Star Zeng

From: "Shi, Steven" <steven.shi@intel.com>

When installing configuration table and the original
gDxeCoreST->ConfigurationTable[] buffer happen to be not big enough to
add a new table, the CoreInstallConfigurationTable() enter the branch
of line 113 in InstallConfigurationTable.c to free the old
gDxeCoreST->ConfigurationTable[] buffer and allocate a new bigger one.
The problem happens at line 139 CoreFreePool(), which is to free the
old gDxeCoreST->ConfigurationTable[] buffer. The CoreFreePool()'s
behavior is to free the buffer firstly, then call the
InstallMemoryAttributesTableOnMemoryAllocation (PoolType) to update
the EfiRuntimeServices type memory info, the
CoreInstallConfigurationTable() will be re-entered by CoreFreePool()
in its calling stack, then use-after-free read error will happen at
line 59 of InstallConfigurationTable.c and use-after-free write error
will happen at line 151 and 152 of InstallConfigurationTable.c.

The patch is to update System table to the new table pointer before
calling CoreFreePool() to free the old table.

The case above is in DxeCore, but not in PiSmmCore.
The change in PiSmmCore is to be consistent with DxeCore.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Steven Shi <steven.shi@intel.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 .../Core/Dxe/Misc/InstallConfigurationTable.c      | 34 ++++++++++++++++------
 .../Core/PiSmmCore/InstallConfigurationTable.c     | 34 ++++++++++++++++------
 2 files changed, 50 insertions(+), 18 deletions(-)
 mode change 100644 => 100755 MdeModulePkg/Core/Dxe/Misc/InstallConfigurationTable.c

diff --git a/MdeModulePkg/Core/Dxe/Misc/InstallConfigurationTable.c b/MdeModulePkg/Core/Dxe/Misc/InstallConfigurationTable.c
old mode 100644
new mode 100755
index e4735db7ba45..dcdeb7f45803
--- a/MdeModulePkg/Core/Dxe/Misc/InstallConfigurationTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/InstallConfigurationTable.c
@@ -1,7 +1,7 @@
 /** @file
   UEFI Miscellaneous boot Services InstallConfigurationTable service
 
-Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -42,6 +42,7 @@ CoreInstallConfigurationTable (
 {
   UINTN                   Index;
   EFI_CONFIGURATION_TABLE *EfiConfigurationTable;
+  EFI_CONFIGURATION_TABLE *OldTable;
 
   //
   // If Guid is NULL, then this operation cannot be performed
@@ -68,7 +69,7 @@ CoreInstallConfigurationTable (
     if (Table != NULL) {
       //
       // If Table is not NULL, then this is a modify operation.
-      // Modify the table enty and return.
+      // Modify the table entry and return.
       //
       gDxeCoreST->ConfigurationTable[Index].VendorTable = Table;
 
@@ -134,15 +135,30 @@ CoreInstallConfigurationTable (
           );
 
         //
-        // Free Old Table
+        // Record the old table pointer.
         //
-        CoreFreePool (gDxeCoreST->ConfigurationTable);
-      }
+        OldTable = gDxeCoreST->ConfigurationTable;
 
-      //
-      // Update System Table
-      //
-      gDxeCoreST->ConfigurationTable = EfiConfigurationTable;
+        //
+        // As the CoreInstallConfigurationTable() may be re-entered by CoreFreePool()
+        // in its calling stack, updating System table to the new table pointer must
+        // be done before calling CoreFreePool() to free the old table.
+        // It can make sure the gDxeCoreST->ConfigurationTable point to the new table
+        // and avoid the errors of use-after-free to the old table by the reenter of
+        // CoreInstallConfigurationTable() in CoreFreePool()'s calling stack.
+        //
+        gDxeCoreST->ConfigurationTable = EfiConfigurationTable;
+
+        //
+        // Free the old table after updating System Table to the new table pointer.
+        //
+        CoreFreePool (OldTable);
+      } else {
+        //
+        // Update System Table
+        //
+        gDxeCoreST->ConfigurationTable = EfiConfigurationTable;
+      }
     }
 
     //
diff --git a/MdeModulePkg/Core/PiSmmCore/InstallConfigurationTable.c b/MdeModulePkg/Core/PiSmmCore/InstallConfigurationTable.c
index b2f6769c109f..2b6eef9a0e3e 100644
--- a/MdeModulePkg/Core/PiSmmCore/InstallConfigurationTable.c
+++ b/MdeModulePkg/Core/PiSmmCore/InstallConfigurationTable.c
@@ -1,7 +1,7 @@
 /** @file
   System Management System Table Services SmmInstallConfigurationTable service
 
-  Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
   This program and the accompanying materials are licensed and made available 
   under the terms and conditions of the BSD License which accompanies this 
   distribution.  The full text of the license may be found at        
@@ -46,6 +46,7 @@ SmmInstallConfigurationTable (
 {
   UINTN                    Index;
   EFI_CONFIGURATION_TABLE  *ConfigurationTable;
+  EFI_CONFIGURATION_TABLE  *OldTable;
 
   //
   // If Guid is NULL, then this operation cannot be performed
@@ -72,7 +73,7 @@ SmmInstallConfigurationTable (
     if (Table != NULL) {
       //
       // If Table is not NULL, then this is a modify operation.
-      // Modify the table enty and return.
+      // Modify the table entry and return.
       //
       ConfigurationTable[Index].VendorTable = Table;
       return EFI_SUCCESS;
@@ -130,15 +131,30 @@ SmmInstallConfigurationTable (
           );
 
         //
-        // Free Old Table
+        // Record the old table pointer.
         //
-        FreePool (gSmmCoreSmst.SmmConfigurationTable);
-      }
+        OldTable = gSmmCoreSmst.SmmConfigurationTable;
 
-      //
-      // Update System Table
-      //
-      gSmmCoreSmst.SmmConfigurationTable = ConfigurationTable;
+        //
+        // As the SmmInstallConfigurationTable() may be re-entered by FreePool() in
+        // its calling stack, updating System table to the new table pointer must
+        // be done before calling FreePool() to free the old table.
+        // It can make sure the gSmmCoreSmst->SmmConfigurationTable point to the new
+        // table and avoid the errors of use-after-free to the old table by the
+        // reenter of SmmInstallConfigurationTable() in FreePool()'s calling stack.
+        //
+        gSmmCoreSmst.SmmConfigurationTable = ConfigurationTable;
+
+        //
+        // Free the old table after updating System Table to the new table pointer.
+        //
+        FreePool (OldTable);
+      } else {
+        //
+        // Update System Table
+        //
+        gSmmCoreSmst.SmmConfigurationTable = ConfigurationTable;
+      }
     }
 
     //
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread