Reviewed-by: Steven Shi <[email protected]>
Steven Shi Intel\SSG\STO\UEFI Firmware Tel: +86 021-61166522 iNet: 821-6522 > -----Original Message----- > From: Zeng, Star > Sent: Monday, June 19, 2017 6:24 PM > To: [email protected] > Cc: Shi, Steven <[email protected]>; Yao, Jiewen <[email protected]>; > Gao, Liming <[email protected]>; Zeng, Star <[email protected]> > Subject: [PATCH] MdeModulePkg: Fix use-after-free error in > InstallConfigurationTable() > > From: "Shi, Steven" <[email protected]> > > 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 <[email protected]> > Cc: Liming Gao <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Steven Shi <[email protected]> > Signed-off-by: Star Zeng <[email protected]> > --- > .../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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
