From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4EF0021A07ABE for ; Mon, 19 Jun 2017 20:04:25 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jun 2017 20:05:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,363,1493708400"; d="scan'208";a="276264629" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 19 Jun 2017 20:05:46 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 19 Jun 2017 20:05:45 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 19 Jun 2017 20:05:44 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.116]) with mapi id 14.03.0319.002; Tue, 20 Jun 2017 11:05:42 +0800 From: "Gao, Liming" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable() Thread-Index: AQHS6OZB9alakSFVPUWMVnJGmKBSsqItElIg Date: Tue, 20 Jun 2017 03:05:41 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D74C19D@shsmsx102.ccr.corp.intel.com> References: <1497867862-117352-1-git-send-email-star.zeng@intel.com> In-Reply-To: <1497867862-117352-1-git-send-email-star.zeng@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Jun 2017 03:04:25 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Liming Gao >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sta= r >Zeng >Sent: Monday, June 19, 2017 6:24 PM >To: edk2-devel@lists.01.org >Cc: Yao, Jiewen ; Zeng, Star ; >Gao, Liming >Subject: [edk2] [PATCH] MdeModulePkg: Fix use-after-free error in >InstallConfigurationTable() > >From: "Shi, Steven" > >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 >Cc: Liming Gao >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Steven Shi >Signed-off-by: Star Zeng >--- > .../Core/Dxe/Misc/InstallConfigurationTable.c | 34 ++++++++++++++++-= --- >-- > .../Core/PiSmmCore/InstallConfigurationTable.c | 34 ++++++++++++++++-= - >---- > 2 files changed, 50 insertions(+), 18 deletions(-) > mode change 100644 =3D> 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.
>+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> 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 !=3D 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 =3D Table; > >@@ -134,15 +135,30 @@ CoreInstallConfigurationTable ( > ); > > // >- // Free Old Table >+ // Record the old table pointer. > // >- CoreFreePool (gDxeCoreST->ConfigurationTable); >- } >+ OldTable =3D gDxeCoreST->ConfigurationTable; > >- // >- // Update System Table >- // >- gDxeCoreST->ConfigurationTable =3D EfiConfigurationTable; >+ // >+ // As the CoreInstallConfigurationTable() may be re-entered by >CoreFreePool() >+ // in its calling stack, updating System table to the new table p= ointer >must >+ // be done before calling CoreFreePool() to free the old table. >+ // It can make sure the gDxeCoreST->ConfigurationTable point to t= he >new table >+ // and avoid the errors of use-after-free to the old table by the= reenter >of >+ // CoreInstallConfigurationTable() in CoreFreePool()'s calling st= ack. >+ // >+ gDxeCoreST->ConfigurationTable =3D EfiConfigurationTable; >+ >+ // >+ // Free the old table after updating System Table to the new tabl= e >pointer. >+ // >+ CoreFreePool (OldTable); >+ } else { >+ // >+ // Update System Table >+ // >+ gDxeCoreST->ConfigurationTable =3D 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.
>+ Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials are licensed and made >available > under the terms and conditions of the BSD License which accompanies thi= s > 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 !=3D 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 =3D Table; > return EFI_SUCCESS; >@@ -130,15 +131,30 @@ SmmInstallConfigurationTable ( > ); > > // >- // Free Old Table >+ // Record the old table pointer. > // >- FreePool (gSmmCoreSmst.SmmConfigurationTable); >- } >+ OldTable =3D gSmmCoreSmst.SmmConfigurationTable; > >- // >- // Update System Table >- // >- gSmmCoreSmst.SmmConfigurationTable =3D ConfigurationTable; >+ // >+ // As the SmmInstallConfigurationTable() may be re-entered by >FreePool() in >+ // its calling stack, updating System table to the new table poin= ter 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 call= ing stack. >+ // >+ gSmmCoreSmst.SmmConfigurationTable =3D ConfigurationTable; >+ >+ // >+ // Free the old table after updating System Table to the new tabl= e >pointer. >+ // >+ FreePool (OldTable); >+ } else { >+ // >+ // Update System Table >+ // >+ gSmmCoreSmst.SmmConfigurationTable =3D ConfigurationTable; >+ } > } > > // >-- >2.7.0.windows.1 > >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel