Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

["Wu, Hao A" <hao.a.wu@intel.com>]

Sorry Marvin Häuser,

Could you help to confirm that below 9 MdeModulePkg related patches are either:
 * All independent patches
 * Belong to a patch series that includes all these 9 MdeModulePkg related commits
 * Belong to several independent patch series

MdePkg/Base.h: Introduce various alignment-related macros
MdeModulePkg/CoreDxe: Mandatory LoadedImage for DebugImageInfoTable
MdeModulePkg/DxeCore: Fix DebugImageInfoTable size report
MdeModulePkg/DxeCore: Use the correct source for fixed load address
MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands
MdeModulePkg/CoreDxe: Drop caller-allocated image buffers
MdeModulePkg/DxeCore: Drop unnecessary pointer indirection
MdeModulePkg/PiSmmIpl: Correct fixed load address bounds check
MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Best Regards,
Hao Wu

> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Monday, August 9, 2021 3:40 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: [PATCH] MdeModulePkg/DxeCore: Consistent
> DebugImageInfoTable updates
> 
> In theory, modifications to the DebugImageInfoTable may cause exceptions.
> If the exception handler parses the table, this can lead to subsequent
> exceptions if the table state is inconsistent.
> 
> Ensure the DebugImageInfoTable remains consistent during modifications.
> This includes:
> 1) Free the old table only only after the new table has been published.
> Mitigates use-after-free of the old table.
> 2) Do not insert an image entry till it is fully initialised. Entries may be inserted
> in the live range if an entry was deleted previously.
> Mitigaes the usage of inconsistent entries.
> 3) Free the old image entry only after the table has been updated with the
> NULL value. Mitigates use-after-free of the old entry.
> 4) Set the MODIFIED state before performing any modifications.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> ---
>  MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 60 +++++++++++------
> ---
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
> b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
> index a75d4158280b..7bd970115111 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
> @@ -165,10 +165,11 @@ CoreNewDebugImageInfoEntry (
>    IN  EFI_HANDLE                  ImageHandle   ) {-  EFI_DEBUG_IMAGE_INFO
> *Table;-  EFI_DEBUG_IMAGE_INFO      *NewTable;-  UINTN                     Index;-
> UINTN                     TableSize;+  EFI_DEBUG_IMAGE_INFO        *Table;+
> EFI_DEBUG_IMAGE_INFO        *NewTable;+  UINTN                       Index;+
> UINTN                       TableSize;+  EFI_DEBUG_IMAGE_INFO_NORMAL
> *NormalImage;    //   // Set the flag indicating that we're in the process of
> updating the table.@@ -203,14 +204,6 @@ CoreNewDebugImageInfoEntry (
>      // Copy the old table into the new one     //     CopyMem (NewTable, Table,
> TableSize);-    //-    // Free the old table-    //-    CoreFreePool (Table);-    //-
> // Update the table header-    //-    Table = NewTable;
> mDebugInfoTableHeader.EfiDebugImageInfoTable = NewTable;     //     //
> Enlarge the max table entries and set the first empty entry index to@@ -
> 218,24 +211,34 @@ CoreNewDebugImageInfoEntry (
>      //     Index             = mMaxTableEntries;     mMaxTableEntries +=
> EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;+    //+    // Free the old
> table+    //+    CoreFreePool (Table);+    //+    // Update the table header+
> //+    Table = NewTable;   }    //   // Allocate data for new entry   //-
> Table[Index].NormalImage = AllocateZeroPool (sizeof
> (EFI_DEBUG_IMAGE_INFO_NORMAL));-  if (Table[Index].NormalImage !=
> NULL) {+  NormalImage = AllocateZeroPool (sizeof
> (EFI_DEBUG_IMAGE_INFO_NORMAL));+  if (NormalImage != NULL) {     //
> // Update the entry     //-    Table[Index].NormalImage->ImageInfoType
> = (UINT32) ImageInfoType;-    Table[Index].NormalImage-
> >LoadedImageProtocolInstance = LoadedImage;-
> Table[Index].NormalImage->ImageHandle                 = ImageHandle;+
> NormalImage->ImageInfoType               = (UINT32) ImageInfoType;+
> NormalImage->LoadedImageProtocolInstance = LoadedImage;+
> NormalImage->ImageHandle                 = ImageHandle;     //-    // Increase the
> number of EFI_DEBUG_IMAGE_INFO elements and set the
> mDebugInfoTable in modified status.+    // Set the mDebugInfoTable in
> modified status, insert the entry, and+    // increase the number of
> EFI_DEBUG_IMAGE_INFO elements.     //-
> mDebugInfoTableHeader.TableSize++;
> mDebugInfoTableHeader.UpdateStatus |=
> EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;+    Table[Index].NormalImage
> = NormalImage;+    mDebugInfoTableHeader.TableSize++;   }
> mDebugInfoTableHeader.UpdateStatus &=
> ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS; }@@ -253,8 +256,9
> @@ CoreRemoveDebugImageInfoEntry (
>    EFI_HANDLE ImageHandle   ) {-  EFI_DEBUG_IMAGE_INFO  *Table;-  UINTN
> Index;+  EFI_DEBUG_IMAGE_INFO        *Table;+  UINTN                       Index;+
> EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;
> mDebugInfoTableHeader.UpdateStatus |=
> EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS; @@ -263,16 +267,20
> @@ CoreRemoveDebugImageInfoEntry (
>    for (Index = 0; Index < mMaxTableEntries; Index++) {     if
> (Table[Index].NormalImage != NULL && Table[Index].NormalImage-
> >ImageHandle == ImageHandle) {       //-      // Found a match. Free up the
> record, then NULL the pointer to indicate the slot-      // is free.+      // Found a
> match. Set the mDebugInfoTable in modified status and NULL the+      //
> pointer to indicate the slot is free and.       //-      CoreFreePool
> (Table[Index].NormalImage);+      NormalImage =
> Table[Index].NormalImage;+      mDebugInfoTableHeader.UpdateStatus |=
> EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;       Table[Index].NormalImage
> = NULL;       //-      // Decrease the number of EFI_DEBUG_IMAGE_INFO
> elements and set the mDebugInfoTable in modified status.+      // Decrease
> the number of EFI_DEBUG_IMAGE_INFO elements.       //
> mDebugInfoTableHeader.TableSize--;-
> mDebugInfoTableHeader.UpdateStatus |=
> EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;+      //+      // Free up the
> record.+      //+      CoreFreePool (NormalImage);       break;     }   }--
> 2.31.1