-----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.wu@...>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Vitaly Cheptsov
<vit9696@...>
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.wu@...>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Vitaly Cheptsov
<vit9696@...>
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%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 <mhaeuser@...>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A
<hao.a.wu@...>; Bi, Dandan <dandan.bi@...>; Liming Gao
<gaoliming@...>; Vitaly Cheptsov
<vit9696@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <gaoliming@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
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