From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"mhaeuser@posteo.de" <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
Date: Mon, 9 Aug 2021 07:26:55 +0000 [thread overview]
Message-ID: <BN8PR11MB36662B1E581A23F71349B38BCAF69@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <d332444f-a2e0-9500-9ecb-4f0124a41d2c@posteo.de>
> -----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
> >>
> >>
> >>
> >>
>
>
>
>
>
next prev parent reply other threads:[~2021-08-09 7:27 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-08 19:39 [PATCH] ArmPkg/DefaultExceptionHandlerLib: Fix DebugImageInfoTable lookup Marvin Häuser
2021-08-08 19:39 ` [PATCH] BaseTools: Define the read-only data section name per toolchain Marvin Häuser
2021-08-08 19:39 ` [PATCH] UefiCpuPkg/BaseUefiCpuLib: Use toolchain-specific rodata section name Marvin Häuser
2021-08-08 19:39 ` [PATCH] BaseTools/tools_def: Fix CLANGPDB X64 RCPATH Marvin Häuser
2021-08-08 19:39 ` [PATCH] EmulatorPkg/Host/Unix: Drop dlopen() usage Marvin Häuser
2021-08-08 19:39 ` [PATCH] EmulatorPkg/Host/Unix: Remove unused declarations Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdeModulePkg/CoreDxe: Drop caller-allocated image buffers Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdeModulePkg/DxeCore: Fix DebugImageInfoTable size report Marvin Häuser
2021-08-08 19:39 ` [PATCH] EmbeddedPkg/GdbStub: Check DebugImageInfoTable type safely Marvin Häuser
2021-08-08 19:39 ` [PATCH] ArmPkg/DefaultExceptionHandlerLib: " Marvin Häuser
2021-08-08 19:40 ` [PATCH] MdeModulePkg/CoreDxe: Mandatory LoadedImage for DebugImageInfoTable Marvin Häuser
2021-08-08 19:40 ` [PATCH] EmbeddedPkg/GdbStub: " Marvin Häuser
2021-08-08 19:40 ` [PATCH] ArmPkg/DefaultExceptionHandlerLib: " Marvin Häuser
2021-08-09 6:10 ` [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates Wu, Hao A
2021-08-09 6:15 ` Marvin Häuser
2021-08-09 6:52 ` [edk2-devel] " Wu, Hao A
2021-08-09 6:55 ` Wu, Hao A
2021-08-09 7:21 ` Marvin Häuser
2021-08-09 7:26 ` Wu, Hao A [this message]
2021-08-08 19:39 ` [PATCH] MdeModulePkg/DxeCore: Drop unnecessary pointer indirection Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdeModulePkg/DxeCore: Use the correct source for fixed load address Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands Marvin Häuser
2021-08-09 4:23 ` Ni, Ray
2021-08-09 5:33 ` Yao, Jiewen
2021-08-09 5:43 ` [edk2-devel] " Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdeModulePkg/PiSmmIpl: Correct fixed load address bounds check Marvin Häuser
2021-08-08 19:39 ` [PATCH] MdePkg/Base.h: Introduce various alignment-related macros Marvin Häuser
2021-08-13 7:27 ` Wu, Hao A
2021-08-13 8:41 ` [edk2-devel] " Marvin Häuser
2021-08-13 8:45 ` Wu, Hao A
2021-08-08 19:39 ` [PATCH] MdePkg/BaseLib: Fix unaligned API prototypes Marvin Häuser
2021-08-08 19:39 ` [PATCH] BaseTools/CommonLib: " Marvin Häuser
2021-08-08 19:39 ` [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256 hash in dbx Marvin Häuser
2021-08-09 0:02 ` Min Xu
2021-08-09 5:25 ` [edk2-devel] " Marvin Häuser
2021-08-09 2:48 ` Yao, Jiewen
2021-08-09 5:42 ` [edk2-devel] " Marvin Häuser
2021-08-08 19:39 ` [PATCH] SecurityPkg/DxeImageVerificationLib: Fix certificate lookup algorithm Marvin Häuser
2021-08-08 19:39 ` [PATCH] SecurityPkg/SecureBootConfigDxe: " Marvin Häuser
2021-08-08 19:39 ` [PATCH] StandaloneMmPkg/FvLib: Correct FV section data size Marvin Häuser
2021-08-08 19:39 ` [PATCH] StandaloneMmPkg/StandaloneMmCore: Drop code for traditional drivers Marvin Häuser
2021-08-08 19:39 ` [PATCH] StandaloneMmPkg/StandaloneMmCore: Drop unused fixed address feature Marvin Häuser
2021-08-08 19:39 ` [PATCH] StandaloneMmPkg: Support CLANGPDB X64 builds Marvin Häuser
2021-10-11 1:04 ` [edk2-devel] " Steven Shi
2021-08-08 19:39 ` [PATCH] UefiPayloadPkg/UefiPayloadEntry: Fix memory corruption Marvin Häuser
2021-08-09 4:20 ` Ni, Ray
2021-08-09 5:47 ` Marvin Häuser
2021-08-10 19:13 ` Guo Dong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB36662B1E581A23F71349B38BCAF69@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox