Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

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

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Monday, August 9, 2021 2:52 PM
> To: devel@edk2.groups.io; mhaeuser@posteo.de
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent
> DebugImageInfoTable updates
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Marvin
> > H?user
> > Sent: Monday, August 9, 2021 2:16 PM
> > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Vitaly Cheptsov
> > <vit9696@protonmail.com>
> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent
> > DebugImageInfoTable updates
> >
> > Good day Hao,
> >
> > Sorry for the confusion, and you are (rightfully!) not alone. :( I'll
> > quote myself from a different patch:
> >
> > [...] for some reason, none of the other patch series had indices appended.
> > I'm sure I can get that fixed shortly, but what to do then, re-send
> > the entire bulk? I don't want to spam the list, maybe it is smarter to
> > group them by some overview mail this one time?
> 
> 
> I would suggest to send a V2 series for all the patches (not only limited to
> MdeModulePkg) you sent.


Maybe more than 1 patch series.
I cannot tell at this moment since there are many patches sent from you.

Best Regards,
Hao Wu


> 
> Please ensure that patches belong to one series are generated by a single 'git
> format-patch' command.
> I think doing so will add information like '1/n', '2/n', ..., 'n/n' for the patches in
> one series.
> And you may need to create a cover-letter for one patch series to give a brief
> summary on the purpose of the series as a whole.
> 
> Also, if you are implementing a new feature or a fix that touches many
> modules, I suggest to file a Bugzilla tracker for it:
> Feature request:
> https://bugzilla.tianocore.org/enter_bug.cgi?product=Tianocore%20Feature
> %20Requests
> Bugfix: https://bugzilla.tianocore.org/enter_bug.cgi?product=EDK2
> 
> Lastly, you may keep the 'Reviewed-by' tags already received by other
> reviewers.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Sorry for the disruption!
> >
> > Best regards,
> > Marvin
> >
> > On 09/08/2021 08:10, Wu, Hao A wrote:
> > > 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
> >
> >
> >
> >
> >
> 
> 
> 
> 
>