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

["Shi, Steven" <steven.shi@intel.com>]

Reviewed-by: Steven Shi <steven.shi@intel.com>


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: edk2-devel@lists.01.org
> Cc: Shi, Steven <steven.shi@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [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