> -----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...@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com>; Vitaly Cheptsov > <vit9...@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. 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 <mhaeu...@posteo.de> > >> Sent: Monday, August 9, 2021 3:40 AM > >> To: devel@edk2.groups.io > >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A > >> <hao.a...@intel.com>; Bi, Dandan <dandan...@intel.com>; Liming Gao > >> <gaolim...@byosoft.com.cn>; Vitaly Cheptsov <vit9...@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.w...@intel.com> > >> Cc: Hao A Wu <hao.a...@intel.com> > >> Cc: Dandan Bi <dandan...@intel.com> > >> Cc: Liming Gao <gaolim...@byosoft.com.cn> > >> Cc: Vitaly Cheptsov <vit9...@protonmail.com> > >> Signed-off-by: Marvin Häuser <mhaeu...@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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78927): https://edk2.groups.io/g/devel/message/78927 Mute This Topic: https://groups.io/mt/84754054/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
