[edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates
Wu, Hao A
hao.a.wu at intel.com
Mon Aug 9 06:52:10 UTC 2021
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Marvin
> H?user
> Sent: Monday, August 9, 2021 2:16 PM
> To: Wu, Hao A <hao.a.wu at intel.com>; devel at edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Vitaly Cheptsov
> <vit9696 at 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 <mhaeuser at posteo.de>
> >> Sent: Monday, August 9, 2021 3:40 AM
> >> To: devel at edk2.groups.io
> >> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
> >> <hao.a.wu at intel.com>; Bi, Dandan <dandan.bi at intel.com>; Liming Gao
> >> <gaoliming at byosoft.com.cn>; Vitaly Cheptsov <vit9696 at 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 at intel.com>
> >> Cc: Hao A Wu <hao.a.wu at intel.com>
> >> Cc: Dandan Bi <dandan.bi at intel.com>
> >> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> >> Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
> >> Signed-off-by: Marvin Häuser <mhaeuser at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list