From: "Gao, Liming" <liming.gao@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>, "Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable()
Date: Tue, 20 Jun 2017 03:05:41 +0000 [thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D74C19D@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <1497867862-117352-1-git-send-email-star.zeng@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star
>Zeng
>Sent: Monday, June 19, 2017 6:24 PM
>To: edk2-devel@lists.01.org
>Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>;
>Gao, Liming <liming.gao@intel.com>
>Subject: [edk2] [PATCH] MdeModulePkg: Fix use-after-free error in
>InstallConfigurationTable()
>
>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
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2017-06-20 3:04 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-19 10:24 [PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable() Star Zeng
[not found] ` <0C09AFA07DD0434D9E2A0C6AEB0483103B8E9423@shsmsx102.ccr.corp.intel.com>
2017-06-20 2:53 ` Yao, Jiewen
2017-06-20 3:05 ` Gao, Liming [this message]
2017-06-20 4:30 ` Shi, Steven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A89E2EF3DFEDB4C8BFDE51014F606A14D74C19D@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox