MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 60 +++++++++++--------- 1 file changed, 34 insertions(+), 26 deletions(-)
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78882): https://edk2.groups.io/g/devel/message/78882
Mute This Topic: https://groups.io/mt/84754054/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78925): https://edk2.groups.io/g/devel/message/78925
Mute This Topic: https://groups.io/mt/84754054/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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?
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78926): https://edk2.groups.io/g/devel/message/78926
Mute This Topic: https://groups.io/mt/84754054/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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.
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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Monday, August 9, 2021 2:52 PM
> To: devel@edk2.groups.io; mhaeuser@posteo.de
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Vitaly Cheptsov
> <vit9696@protonmail.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent
> DebugImageInfoTable updates
>
> > -----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.
Maybe more than 1 patch series.
I cannot tell at this moment since there are many patches sent from you.
Best Regards,
Hao Wu
>
> 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
> >
> >
> >
> >
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78928): https://edk2.groups.io/g/devel/message/78928
Mute This Topic: https://groups.io/mt/84754054/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>>
>>
>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78930): https://edk2.groups.io/g/devel/message/78930
Mute This Topic: https://groups.io/mt/84754054/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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@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
>
> 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.
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 <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
> >>
> >>
> >>
> >>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Separate tracking the used entries from the table's self-reported
size. Removing an entry from the table does not necessarily reduce
the size of the table as defragmentation is not performed.
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 | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index 7bd970115111..cc22e23eb0b3 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -18,6 +18,8 @@ EFI_DEBUG_IMAGE_INFO_TABLE_HEADER mDebugInfoTableHeader = {
UINTN mMaxTableEntries = 0;
+UINTN mUsedTableEntries = 0;
+
EFI_SYSTEM_TABLE_POINTER *mDebugTable = NULL;
#define EFI_DEBUG_TABLE_ENTRY_SIZE (sizeof (VOID *))
@@ -178,7 +180,7 @@ CoreNewDebugImageInfoEntry (
Table = mDebugInfoTableHeader.EfiDebugImageInfoTable;
- if (mDebugInfoTableHeader.TableSize < mMaxTableEntries) {
+ if (mUsedTableEntries < mMaxTableEntries) {
//
// We still have empty entires in the Table, find the first empty entry.
//
@@ -237,8 +239,17 @@ CoreNewDebugImageInfoEntry (
// increase the number of EFI_DEBUG_IMAGE_INFO elements.
//
mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
+ mUsedTableEntries++;
Table[Index].NormalImage = NormalImage;
- mDebugInfoTableHeader.TableSize++;
+ //
+ // Only increase the amount of elements in the table if the new entry did
+ // not take the place of a previously removed entry.
+ //
+ if (Index == mDebugInfoTableHeader.TableSize) {
+ mDebugInfoTableHeader.TableSize++;
+ }
+
+ ASSERT (Index < mDebugInfoTableHeader.TableSize);
}
mDebugInfoTableHeader.UpdateStatus &= ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
}
@@ -274,9 +285,10 @@ CoreRemoveDebugImageInfoEntry (
mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
Table[Index].NormalImage = NULL;
//
- // Decrease the number of EFI_DEBUG_IMAGE_INFO elements.
+ // Do not reduce the amount of elements reported to be in the table as
+ // this would only work for the last element without defragmentation.
//
- mDebugInfoTableHeader.TableSize--;
+ mUsedTableEntries--;
//
// Free up the record.
//
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78897): https://edk2.groups.io/g/devel/message/78897
Mute This Topic: https://groups.io/mt/84754072/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
C does not allow casting to or dereferencing incompatible pointer
types. Use the ImageInfoType member of the union first to determine
the data type before dereferencing NormalImage.
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
EmbeddedPkg/GdbStub/GdbStub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/EmbeddedPkg/GdbStub/GdbStub.c b/EmbeddedPkg/GdbStub/GdbStub.c
index 7f2a5ed20011..09167fdafb4d 100644
--- a/EmbeddedPkg/GdbStub/GdbStub.c
+++ b/EmbeddedPkg/GdbStub/GdbStub.c
@@ -1043,8 +1043,8 @@ QxferLibrary (
if (gDebugTable != NULL) {
for (; gEfiDebugImageTableEntry < gDebugImageTableHeader->TableSize; gEfiDebugImageTableEntry++, gDebugTable++) {
- if (gDebugTable->NormalImage != NULL) {
- if ((gDebugTable->NormalImage->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
+ if (gDebugTable->ImageInfoType != NULL) {
+ if ((*gDebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
(gDebugTable->NormalImage->LoadedImageProtocolInstance != NULL)) {
Pdb = PeCoffLoaderGetDebuggerInfo (
gDebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase,
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78900): https://edk2.groups.io/g/devel/message/78900
Mute This Topic: https://groups.io/mt/84754075/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
C does not allow casting to or dereferencing incompatible pointer
types. Use the ImageInfoType member of the union first to determine
the data type before dereferencing NormalImage.
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c
index e9fea4038252..9befb6d4db9b 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c
@@ -51,8 +51,8 @@ GetImageName (
Address = (CHAR8 *)(UINTN)FaultAddress;
for (Entry = 0; Entry < DebugTableHeader->TableSize; Entry++, DebugTable++) {
- if (DebugTable->NormalImage != NULL) {
- if ((DebugTable->NormalImage->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
+ if (DebugTable->ImageInfoType != NULL) {
+ if ((*DebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
(DebugTable->NormalImage->LoadedImageProtocolInstance != NULL)) {
if ((Address >= (CHAR8 *)DebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase) &&
(Address <= ((CHAR8 *)DebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase + DebugTable->NormalImage->LoadedImageProtocolInstance->ImageSize))) {
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78901): https://edk2.groups.io/g/devel/message/78901
Mute This Topic: https://groups.io/mt/84754076/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
To make parsing DebugImageInfoTable easier and safer, require the
LoadedImage protocol instance to be valid for every NormalImage
entry.
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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index cc22e23eb0b3..afc54965bc33 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -173,6 +173,8 @@ CoreNewDebugImageInfoEntry (
UINTN TableSize;
EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;
+ ASSERT (LoadedImage != NULL);
+
//
// Set the flag indicating that we're in the process of updating the table.
//
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78902): https://edk2.groups.io/g/devel/message/78902
Mute This Topic: https://groups.io/mt/84754077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
To make parsing DebugImageInfoTable easier and safer, require the
LoadedImage protocol instance to be valid for every NormalImage
entry.
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
EmbeddedPkg/GdbStub/GdbStub.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/EmbeddedPkg/GdbStub/GdbStub.c b/EmbeddedPkg/GdbStub/GdbStub.c
index 09167fdafb4d..29aa63237304 100644
--- a/EmbeddedPkg/GdbStub/GdbStub.c
+++ b/EmbeddedPkg/GdbStub/GdbStub.c
@@ -1044,8 +1044,9 @@ QxferLibrary (
if (gDebugTable != NULL) {
for (; gEfiDebugImageTableEntry < gDebugImageTableHeader->TableSize; gEfiDebugImageTableEntry++, gDebugTable++) {
if (gDebugTable->ImageInfoType != NULL) {
- if ((*gDebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
- (gDebugTable->NormalImage->LoadedImageProtocolInstance != NULL)) {
+ if (*gDebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) {
+ ASSERT (gDebugTable->NormalImage->LoadedImageProtocolInstance != NULL);
+
Pdb = PeCoffLoaderGetDebuggerInfo (
gDebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase,
&LoadAddress
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78904): https://edk2.groups.io/g/devel/message/78904
Mute This Topic: https://groups.io/mt/84754079/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
To make parsing DebugImageInfoTable easier and safer, require the
LoadedImage protocol instance to be valid for every NormalImage
entry.
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c
index 9befb6d4db9b..d442b5d358b2 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c
@@ -52,8 +52,9 @@ GetImageName (
Address = (CHAR8 *)(UINTN)FaultAddress;
for (Entry = 0; Entry < DebugTableHeader->TableSize; Entry++, DebugTable++) {
if (DebugTable->ImageInfoType != NULL) {
- if ((*DebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) &&
- (DebugTable->NormalImage->LoadedImageProtocolInstance != NULL)) {
+ if (*DebugTable->ImageInfoType == EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL) {
+ ASSERT (gDebugTable->NormalImage->LoadedImageProtocolInstance != NULL);
+
if ((Address >= (CHAR8 *)DebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase) &&
(Address <= ((CHAR8 *)DebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase + DebugTable->NormalImage->LoadedImageProtocolInstance->ImageSize))) {
*ImageBase = (UINTN)DebugTable->NormalImage->LoadedImageProtocolInstance->ImageBase;
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78903): https://edk2.groups.io/g/devel/message/78903
Mute This Topic: https://groups.io/mt/84754078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.