> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin > H?user > Sent: Monday, August 9, 2021 3:21 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 > > 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...@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. > > 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.
I am fine with this. My intention for asking V2 for all the patches was that doing so I can simply ignore all the V1 patch mails. Best Regards, Hao Wu > > 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%20Featu > > re%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 (#78931): https://edk2.groups.io/g/devel/message/78931 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] -=-=-=-=-=-=-=-=-=-=-=-
