From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 26B4D21962331 for ; Mon, 19 Jun 2017 19:52:35 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jun 2017 19:53:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,363,1493708400"; d="scan'208";a="276262058" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 19 Jun 2017 19:53:57 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 19 Jun 2017 19:53:56 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.56]) with mapi id 14.03.0319.002; Tue, 20 Jun 2017 10:53:44 +0800 From: "Yao, Jiewen" To: "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Zeng, Star" , "Gao, Liming" Thread-Topic: [edk2] [PATCH] MdeModulePkg: Fix use-after-free error in InstallConfigurationTable() Thread-Index: AQHS6OY/PdKR0ogMg0yr7iYEMmBFa6Is9m1QgAAYdQA= Date: Tue, 20 Jun 2017 02:53:43 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A96B19E@shsmsx102.ccr.corp.intel.com> References: <1497867862-117352-1-git-send-email-star.zeng@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B8E9423@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B8E9423@shsmsx102.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action 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 02:52:35 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jiewen.yao@intel.com > -----Original Message----- > From: Zeng, Star > Sent: Tuesday, June 20, 2017 9:28 AM > To: Yao, Jiewen ; Gao, Liming > Cc: Zeng, Star ; Shi, Steven > Subject: RE: [edk2] [PATCH] MdeModulePkg: Fix use-after-free error in > InstallConfigurationTable() >=20 > Help review this patch. >=20 > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of St= ar > 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() >=20 > From: "Shi, Steven" >=20 > 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 i= ts 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. >=20 > The patch is to update System table to the new table pointer before calli= ng > CoreFreePool() to free the old table. >=20 > The case above is in DxeCore, but not in PiSmmCore. > The change in PiSmmCore is to be consistent with DxeCore. >=20 > 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 >=20 > 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 >=20 > -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 avail= able > 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; >=20 > // > // 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; >=20 > @@ -134,15 +135,30 @@ CoreInstallConfigurationTable ( > ); >=20 > // > - // Free Old Table > + // Record the old table pointer. > // > - CoreFreePool (gDxeCoreST->ConfigurationTable); > - } > + OldTable =3D gDxeCoreST->ConfigurationTable; >=20 > - // > - // 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 = 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 th= e > reenter of > + // CoreInstallConfigurationTable() in CoreFreePool()'s calling s= tack. > + // > + gDxeCoreST->ConfigurationTable =3D EfiConfigurationTable; > + > + // > + // Free the old table after updating System Table to the new tab= le > pointer. > + // > + CoreFreePool (OldTable); > + } else { > + // > + // Update System Table > + // > + gDxeCoreST->ConfigurationTable =3D EfiConfigurationTable; > + } > } >=20 > // > 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 >=20 > - 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 avai= lable > under the terms and conditions of the BSD License which accompanies th= is > 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; >=20 > // > // 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 ( > ); >=20 > // > - // Free Old Table > + // Record the old table pointer. > // > - FreePool (gSmmCoreSmst.SmmConfigurationTable); > - } > + OldTable =3D gSmmCoreSmst.SmmConfigurationTable; >=20 > - // > - // 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 poi= nter > 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 cal= ling > stack. > + // > + gSmmCoreSmst.SmmConfigurationTable =3D ConfigurationTable; > + > + // > + // Free the old table after updating System Table to the new tab= le > pointer. > + // > + FreePool (OldTable); > + } else { > + // > + // Update System Table > + // > + gSmmCoreSmst.SmmConfigurationTable =3D ConfigurationTable; > + } > } >=20 > // > -- > 2.7.0.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel