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

["Marvin Häuser" <mhaeuser@posteo.de>]

On 09/08/2021 08:52, Wu, Hao A wrote:
>> -----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.

Right, I can do that, just many of the patches were actually meant to be 
single and independent. I believe there were two series that somehow did 
not get indexed by the command. I just forced numbering now and it seems 
to work.

May it be easier if I re-send only the two series? A few of the 
individual patches actually started review.

Thanks for your suggestions, and sorry again for the disruption!

Best regards,
Marvin

>
> 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
>>
>>
>> 
>>