[edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates
Marvin Häuser
mhaeuser at posteo.de
Mon Aug 9 06:15:42 UTC 2021
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 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 (#78926): https://edk2.groups.io/g/devel/message/78926
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