Date   

Re: [Patch] BaseTools: use shutil.copyfile instead shutil.copy2

Philippe Mathieu-Daudé <philmd@...>
 

On 7/28/21 1:45 PM, Bob Feng wrote:
In Split tool, the copy file actions only need to
copy file content but not need to copy file metadata.

copy2() copies the file metadata that causes split
unit test failed under edk2-basetools CI environment.

So this patch changes the call of copy2() to copyfile().

Signed-off-by: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Cc: Yuwei Chen <yuwei.chen@...>
---
BaseTools/Source/Python/Split/Split.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@...>


Re: [PATCH] UefiPayloadPkg: Fix the non-ascii character in UniversalPayloadEntry.c

Philippe Mathieu-Daudé <philmd@...>
 

On 8/9/21 8:08 AM, duntan wrote:
Fix the non-ascii character in UniversalPayloadEntry.c

Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>

Signed-off-by: DunTan <dun.tan@...>
---
UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@...>


Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Wu, Hao A
 

-----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.

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@...>
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






Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Marvin Häuser <mhaeuser@...>
 

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%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@...>
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


Re: [PATCH] UefiPayloadPkg: Fix the non-ascii character in UniversalPayloadEntry.c

Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@...>

-----Original Message-----
From: Tan, Dun <dun.tan@...>
Sent: Monday, August 9, 2021 2:08 PM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Ni, Ray <ray.ni@...>; Ma, Maurice <maurice.ma@...>; You, Benjamin <benjamin.you@...>; Tan, Dun <dun.tan@...>
Subject: [PATCH] UefiPayloadPkg: Fix the non-ascii character in UniversalPayloadEntry.c

Fix the non-ascii character in UniversalPayloadEntry.c

Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>

Signed-off-by: DunTan <dun.tan@...>
---
UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c
index 09dd1e8378..03ad9c457b 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c
@@ -38,7 +38,7 @@ PrintHob (
/**
Some bootloader may pass a pcd database, and UPL also contain a PCD database.
Dxe PCD driver has the assumption that the two PCD database can be catenated and
- the local token number should be successive。
+ the local token number should be successive.
This function will fix up the UPL PCD database to meet that assumption.

@param[in] DxeFv The FV where to find the Universal PCD database.
--
2.31.1.windows.1


Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Wu, Hao A
 

-----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@...
Cc: Wang, Jian J <jian.j.wang@...>; Vitaly Cheptsov
<vit9696@...>
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@...>; 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.

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@...>
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







Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Wu, Hao A
 

-----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.

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@...>
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




Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Marvin Häuser <mhaeuser@...>
 

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@...>
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


Re: [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Wu, Hao A
 

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


[PATCH v2 1/1] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands

Marvin Häuser <mhaeuser@...>
 

The legacy codebase allowed SMM images to be registered for profiling
from DXE. Support for this has been dropped entirely, so remove the
remaining handlers.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 89 ++------------------
MdeModulePkg/Include/Guid/MemoryProfile.h | 6 +-
2 files changed, 12 insertions(+), 83 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index 1b302c810cc9..9d6e3bf27aca 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -2232,64 +2232,6 @@ Done:
mSmramProfileGettingStatus = SmramProfileGettingStatus;
}

-/**
- SMRAM profile handler to register SMM image.
-
- @param SmramProfileParameterRegisterImage The parameter of SMM profile register image.
-
-**/
-VOID
-SmramProfileHandlerRegisterImage (
- IN SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *SmramProfileParameterRegisterImage
- )
-{
- EFI_STATUS Status;
- EFI_SMM_DRIVER_ENTRY DriverEntry;
- VOID *EntryPointInImage;
-
- ZeroMem (&DriverEntry, sizeof (DriverEntry));
- CopyMem (&DriverEntry.FileName, &SmramProfileParameterRegisterImage->FileName, sizeof(EFI_GUID));
- DriverEntry.ImageBuffer = SmramProfileParameterRegisterImage->ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterRegisterImage->NumberOfPage;
- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN) DriverEntry.ImageBuffer, &EntryPointInImage);
- ASSERT_EFI_ERROR (Status);
- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN) EntryPointInImage;
-
- Status = RegisterSmramProfileImage (&DriverEntry, FALSE);
- if (!EFI_ERROR (Status)) {
- SmramProfileParameterRegisterImage->Header.ReturnStatus = 0;
- }
-}
-
-/**
- SMRAM profile handler to unregister SMM image.
-
- @param SmramProfileParameterUnregisterImage The parameter of SMM profile unregister image.
-
-**/
-VOID
-SmramProfileHandlerUnregisterImage (
- IN SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *SmramProfileParameterUnregisterImage
- )
-{
- EFI_STATUS Status;
- EFI_SMM_DRIVER_ENTRY DriverEntry;
- VOID *EntryPointInImage;
-
- ZeroMem (&DriverEntry, sizeof (DriverEntry));
- CopyMem (&DriverEntry.FileName, &SmramProfileParameterUnregisterImage->FileName, sizeof (EFI_GUID));
- DriverEntry.ImageBuffer = SmramProfileParameterUnregisterImage->ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterUnregisterImage->NumberOfPage;
- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN) DriverEntry.ImageBuffer, &EntryPointInImage);
- ASSERT_EFI_ERROR (Status);
- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN) EntryPointInImage;
-
- Status = UnregisterSmramProfileImage (&DriverEntry, FALSE);
- if (!EFI_ERROR (Status)) {
- SmramProfileParameterUnregisterImage->Header.ReturnStatus = 0;
- }
-}
-
/**
Dispatch function for a Software SMI handler.

@@ -2374,28 +2316,6 @@ SmramProfileHandler (
}
SmramProfileHandlerGetDataByOffset ((SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET *) (UINTN) CommBuffer);
break;
- case SMRAM_PROFILE_COMMAND_REGISTER_IMAGE:
- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerRegisterImage\n"));
- if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE)) {
- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer size invalid!\n"));
- return EFI_SUCCESS;
- }
- if (mSmramReadyToLock) {
- return EFI_SUCCESS;
- }
- SmramProfileHandlerRegisterImage ((SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *) (UINTN) CommBuffer);
- break;
- case SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE:
- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerUnregisterImage\n"));
- if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE)) {
- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer size invalid!\n"));
- return EFI_SUCCESS;
- }
- if (mSmramReadyToLock) {
- return EFI_SUCCESS;
- }
- SmramProfileHandlerUnregisterImage ((SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *) (UINTN) CommBuffer);
- break;
case SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE:
DEBUG ((EFI_D_ERROR, "SmramProfileHandlerGetRecordingState\n"));
if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)) {
@@ -2417,6 +2337,15 @@ SmramProfileHandler (
ParameterRecordingState->Header.ReturnStatus = 0;
break;

+ //
+ // Below 2 commands have been deprecated. They may not be (re-)used.
+ //
+ case SMRAM_PROFILE_COMMAND_DEPRECATED1:
+ case SMRAM_PROFILE_COMMAND_DEPRECATED2:
+ ASSERT (FALSE);
+ //
+ // Fall-through to the default (unrecognized command) case.
+ //
default:
break;
}
diff --git a/MdeModulePkg/Include/Guid/MemoryProfile.h b/MdeModulePkg/Include/Guid/MemoryProfile.h
index eee3b9125240..7565e68b5c33 100644
--- a/MdeModulePkg/Include/Guid/MemoryProfile.h
+++ b/MdeModulePkg/Include/Guid/MemoryProfile.h
@@ -389,10 +389,10 @@ struct _EDKII_MEMORY_PROFILE_PROTOCOL {
#define SMRAM_PROFILE_COMMAND_GET_PROFILE_INFO 0x1
#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA 0x2
//
-// Below 2 commands are now used by ECP only and only valid before SmmReadyToLock
+// Below 2 commands have been deprecated. They may not be re-used.
//
-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3
-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4
+#define SMRAM_PROFILE_COMMAND_DEPRECATED1 0x3
+#define SMRAM_PROFILE_COMMAND_DEPRECATED2 0x4

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA_BY_OFFSET 0x5
#define SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE 0x6
--
2.31.1


[PATCH] UefiPayloadPkg: Fix the non-ascii character in UniversalPayloadEntry.c

duntan
 

Fix the non-ascii character in UniversalPayloadEntry.c

Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>

Signed-off-by: DunTan <dun.tan@...>
---
UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c
index 09dd1e8378..03ad9c457b 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c
@@ -38,7 +38,7 @@ PrintHob (
/**
Some bootloader may pass a pcd database, and UPL also contain a PCD database.
Dxe PCD driver has the assumption that the two PCD database can be catenated and
- the local token number should be successive。
+ the local token number should be successive.
This function will fix up the UPL PCD database to meet that assumption.

@param[in] DxeFv The FV where to find the Universal PCD database.
--
2.31.1.windows.1


Re: [PATCH 1/1] MdeModulePkg/Console: Improve encoding of box drawing characters

Wu, Hao A
 

Sorry Zhichao and Ray, could you please help to check if you have comments for this patch. Thanks in advance.

Hello Caden Kline, could you help to provide the information on what kind of unit test was done for this patch?

Some inline comments for ONLY coding style issues:

-----Original Message-----
From: Caden Kline <cadenkline9@...>
Sent: Friday, July 30, 2021 10:45 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Gao, Zhichao <zhichao.gao@...>; Ni, Ray <ray.ni@...>
Subject: [PATCH 1/1] MdeModulePkg/Console: Improve encoding of box
drawing characters

Improved encoding of box drawing characters for different terminal types.
This includes Dec special graphics mode and more utf8.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Zhichao Gao <zhichao.gao@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Caden Kline <cadenkline9@...>
---
.../Universal/Console/TerminalDxe/Terminal.h | 24 +-
.../Universal/Console/TerminalDxe/Ansi.c | 2 +-
.../Console/TerminalDxe/TerminalConOut.c | 322 ++++++++++++++----
3 files changed, 269 insertions(+), 79 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
index 360e58e84743..83c3ea94a042 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
@@ -122,7 +122,10 @@ typedef struct {
EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL SimpleInputEx;

LIST_ENTRY NotifyList;

EFI_EVENT KeyNotifyProcessEvent;

+ BOOLEAN DecSpecialGraphicsMode;

} TERMINAL_DEV;

+// This the lenth the escape squences for entering and exiting Dec Special

Typo: lenth -> length, squences -> sequences

Also, please help to update the comments to style:
//
// This the length the escape sequences for entering and exiting Dec Special
//



Graphics Mode

+#define LENGTH_DEC_ESCAPE 0x03



#define INPUT_STATE_DEFAULT 0x00

#define INPUT_STATE_ESC 0x01

@@ -169,6 +172,7 @@ typedef struct {
UINT16 Unicode;

CHAR8 PcAnsi;

CHAR8 Ascii;

+ CHAR8 DecSpecialGraphics;

} UNICODE_TO_CHAR;



//

@@ -1367,20 +1371,22 @@ Utf8ToUnicode (
/**

Detects if a Unicode char is for Box Drawing text graphics.



- @param Graphic Unicode char to test.

- @param PcAnsi Optional pointer to return PCANSI equivalent of

- Graphic.

- @param Ascii Optional pointer to return ASCII equivalent of

- Graphic.

-

- @retval TRUE If Graphic is a supported Unicode Box Drawing character.

+ @param Graphic Unicode char to test.

+ @param PcAnsi Optional pointer to return PCANSI equivalent of

+ Graphic.

+ @param Ascii Optional pointer to return ASCII equivalent of

+ Graphic.

+ @param DecSpecialGraphics Optional pointer to return Dec Special
Graphics equivalent of

+ Graphic.

+ @retval TRUE If Graphic is a supported Unicode Box Drawing
character.



**/

BOOLEAN

TerminalIsValidTextGraphics (

IN CHAR16 Graphic,

- OUT CHAR8 *PcAnsi, OPTIONAL

- OUT CHAR8 *Ascii OPTIONAL

+ OUT CHAR8 *PcAnsi, OPTIONAL

+ OUT CHAR8 *Ascii, OPTIONAL

+ OUT CHAR8 *DecSpecialGraphics OPTIONAL

);



/**

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Ansi.c
b/MdeModulePkg/Universal/Console/TerminalDxe/Ansi.c
index f117d90b9de3..5ae5a4f0212e 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Ansi.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Ansi.c
@@ -63,7 +63,7 @@ AnsiTestString (


if ( !(TerminalIsValidAscii (*WString) ||

TerminalIsValidEfiCntlChar (*WString) ||

- TerminalIsValidTextGraphics (*WString, &GraphicChar, NULL) )) {

+ TerminalIsValidTextGraphics (*WString, &GraphicChar, NULL, NULL) )) {



return EFI_UNSUPPORTED;

}

diff --git
a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c
b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c
index aae470e9562c..1c22ed426715 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c
@@ -16,61 +16,59 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
//

//

UNICODE_TO_CHAR UnicodeToPcAnsiOrAscii[] = {

- { BOXDRAW_HORIZONTAL, 0xc4, L'-' },

- { BOXDRAW_VERTICAL, 0xb3, L'|' },

- { BOXDRAW_DOWN_RIGHT, 0xda, L'/' },

- { BOXDRAW_DOWN_LEFT, 0xbf, L'\\' },

- { BOXDRAW_UP_RIGHT, 0xc0, L'\\' },

- { BOXDRAW_UP_LEFT, 0xd9, L'/' },

- { BOXDRAW_VERTICAL_RIGHT, 0xc3, L'|' },

- { BOXDRAW_VERTICAL_LEFT, 0xb4, L'|' },

- { BOXDRAW_DOWN_HORIZONTAL, 0xc2, L'+' },

- { BOXDRAW_UP_HORIZONTAL, 0xc1, L'+' },

- { BOXDRAW_VERTICAL_HORIZONTAL, 0xc5, L'+' },

- { BOXDRAW_DOUBLE_HORIZONTAL, 0xcd, L'-' },

- { BOXDRAW_DOUBLE_VERTICAL, 0xba, L'|' },

- { BOXDRAW_DOWN_RIGHT_DOUBLE, 0xd5, L'/' },

- { BOXDRAW_DOWN_DOUBLE_RIGHT, 0xd6, L'/' },

- { BOXDRAW_DOUBLE_DOWN_RIGHT, 0xc9, L'/' },

- { BOXDRAW_DOWN_LEFT_DOUBLE, 0xb8, L'\\' },

- { BOXDRAW_DOWN_DOUBLE_LEFT, 0xb7, L'\\' },

- { BOXDRAW_DOUBLE_DOWN_LEFT, 0xbb, L'\\' },

- { BOXDRAW_UP_RIGHT_DOUBLE, 0xd4, L'\\' },

- { BOXDRAW_UP_DOUBLE_RIGHT, 0xd3, L'\\' },

- { BOXDRAW_DOUBLE_UP_RIGHT, 0xc8, L'\\' },

- { BOXDRAW_UP_LEFT_DOUBLE, 0xbe, L'/' },

- { BOXDRAW_UP_DOUBLE_LEFT, 0xbd, L'/' },

- { BOXDRAW_DOUBLE_UP_LEFT, 0xbc, L'/' },

- { BOXDRAW_VERTICAL_RIGHT_DOUBLE, 0xc6, L'|' },

- { BOXDRAW_VERTICAL_DOUBLE_RIGHT, 0xc7, L'|' },

- { BOXDRAW_DOUBLE_VERTICAL_RIGHT, 0xcc, L'|' },

- { BOXDRAW_VERTICAL_LEFT_DOUBLE, 0xb5, L'|' },

- { BOXDRAW_VERTICAL_DOUBLE_LEFT, 0xb6, L'|' },

- { BOXDRAW_DOUBLE_VERTICAL_LEFT, 0xb9, L'|' },

- { BOXDRAW_DOWN_HORIZONTAL_DOUBLE, 0xd1, L'+' },

- { BOXDRAW_DOWN_DOUBLE_HORIZONTAL, 0xd2, L'+' },

- { BOXDRAW_DOUBLE_DOWN_HORIZONTAL, 0xcb, L'+' },

- { BOXDRAW_UP_HORIZONTAL_DOUBLE, 0xcf, L'+' },

- { BOXDRAW_UP_DOUBLE_HORIZONTAL, 0xd0, L'+' },

- { BOXDRAW_DOUBLE_UP_HORIZONTAL, 0xca, L'+' },

- { BOXDRAW_VERTICAL_HORIZONTAL_DOUBLE, 0xd8, L'+' },

- { BOXDRAW_VERTICAL_DOUBLE_HORIZONTAL, 0xd7, L'+' },

- { BOXDRAW_DOUBLE_VERTICAL_HORIZONTAL, 0xce, L'+' },

+ { BOXDRAW_HORIZONTAL, 0xc4, L'-', 0x71 },

+ { BOXDRAW_VERTICAL, 0xb3, L'|', 0x78 },

+ { BOXDRAW_DOWN_RIGHT, 0xda, L'/', 0x6c },

+ { BOXDRAW_DOWN_LEFT, 0xbf, L'\\', 0x6b },

+ { BOXDRAW_UP_RIGHT, 0xc0, L'\\', 0x6d },

+ { BOXDRAW_UP_LEFT, 0xd9, L'/', 0x6a },

+ { BOXDRAW_VERTICAL_RIGHT, 0xc3, L'|', 0x74 },

+ { BOXDRAW_VERTICAL_LEFT, 0xb4, L'|', 0x75 },

+ { BOXDRAW_DOWN_HORIZONTAL, 0xc2, L'+', 0x77 },

+ { BOXDRAW_UP_HORIZONTAL, 0xc1, L'+', 0x76 },

+ { BOXDRAW_VERTICAL_HORIZONTAL, 0xc5, L'+', 0x6e },

+ { BOXDRAW_DOUBLE_HORIZONTAL, 0xcd, L'-', 0x71 },

+ { BOXDRAW_DOUBLE_VERTICAL, 0xba, L'|', 0x78 },

+ { BOXDRAW_DOWN_RIGHT_DOUBLE, 0xd5, L'/', 0x6c },

+ { BOXDRAW_DOWN_DOUBLE_RIGHT, 0xd6, L'/', 0x6c },

+ { BOXDRAW_DOUBLE_DOWN_RIGHT, 0xc9, L'/', 0x6c },

+ { BOXDRAW_DOWN_LEFT_DOUBLE, 0xb8, L'\\', 0x6b },

+ { BOXDRAW_DOWN_DOUBLE_LEFT, 0xb7, L'\\', 0x6b },

+ { BOXDRAW_DOUBLE_DOWN_LEFT, 0xbb, L'\\', 0x6b },

+ { BOXDRAW_UP_RIGHT_DOUBLE, 0xd4, L'\\', 0x6d },

+ { BOXDRAW_UP_DOUBLE_RIGHT, 0xd3, L'\\', 0x6d },

+ { BOXDRAW_DOUBLE_UP_RIGHT, 0xc8, L'\\', 0x6d },

+ { BOXDRAW_UP_LEFT_DOUBLE, 0xbe, L'/', 0x6a },

+ { BOXDRAW_UP_DOUBLE_LEFT, 0xbd, L'/', 0x6a },

+ { BOXDRAW_DOUBLE_UP_LEFT, 0xbc, L'/', 0x6a },

+ { BOXDRAW_VERTICAL_RIGHT_DOUBLE, 0xc6, L'|', 0x74 },

+ { BOXDRAW_VERTICAL_DOUBLE_RIGHT, 0xc7, L'|', 0x74 },

+ { BOXDRAW_DOUBLE_VERTICAL_RIGHT, 0xcc, L'|', 0x74 },

+ { BOXDRAW_VERTICAL_LEFT_DOUBLE, 0xb5, L'|', 0x75 },

+ { BOXDRAW_VERTICAL_DOUBLE_LEFT, 0xb6, L'|', 0x75 },

+ { BOXDRAW_DOUBLE_VERTICAL_LEFT, 0xb9, L'|', 0x75 },

+ { BOXDRAW_DOWN_HORIZONTAL_DOUBLE, 0xd1, L'+', 0x77 },

+ { BOXDRAW_DOWN_DOUBLE_HORIZONTAL, 0xd2, L'+', 0x77 },

+ { BOXDRAW_DOUBLE_DOWN_HORIZONTAL, 0xcb, L'+', 0x77 },

+ { BOXDRAW_UP_HORIZONTAL_DOUBLE, 0xcf, L'+', 0x76 },

+ { BOXDRAW_UP_DOUBLE_HORIZONTAL, 0xd0, L'+', 0x76 },

+ { BOXDRAW_DOUBLE_UP_HORIZONTAL, 0xca, L'+', 0x76 },

+ { BOXDRAW_VERTICAL_HORIZONTAL_DOUBLE, 0xd8, L'+', 0x6e },

+ { BOXDRAW_VERTICAL_DOUBLE_HORIZONTAL, 0xd7, L'+', 0x6e },

+ { BOXDRAW_DOUBLE_VERTICAL_HORIZONTAL, 0xce, L'+', 0x6e },



- { BLOCKELEMENT_FULL_BLOCK, 0xdb, L'*' },

- { BLOCKELEMENT_LIGHT_SHADE, 0xb0, L'+' },

+ { BLOCKELEMENT_FULL_BLOCK, 0xdb, L'*', 0x61 },

+ { BLOCKELEMENT_LIGHT_SHADE, 0xb0, L'+', 0x61 },



- { GEOMETRICSHAPE_UP_TRIANGLE, '^', L'^' },

- { GEOMETRICSHAPE_RIGHT_TRIANGLE, '>', L'>' },

- { GEOMETRICSHAPE_DOWN_TRIANGLE, 'v', L'v' },

- { GEOMETRICSHAPE_LEFT_TRIANGLE, '<', L'<' },

+ { GEOMETRICSHAPE_UP_TRIANGLE, '^', L'^', L'^' },

+ { GEOMETRICSHAPE_RIGHT_TRIANGLE, '>', L'>', L'>' },

+ { GEOMETRICSHAPE_DOWN_TRIANGLE, 'v', L'v', L'v' },

+ { GEOMETRICSHAPE_LEFT_TRIANGLE, '<', L'<', L'<' },



- { ARROW_LEFT, '<', L'<' },

- { ARROW_UP, '^', L'^' },

- { ARROW_RIGHT, '>', L'>' },

- { ARROW_DOWN, 'v', L'v' },

-

- { 0x0000, 0x00, L'\0' }

+ { ARROW_LEFT, '<', L'<', L'<' },

+ { ARROW_UP, '^', L'^', L'^' },

+ { ARROW_RIGHT, '>', L'>', L'>' },

+ { ARROW_DOWN, 'v', L'v', L'v' },

};



CHAR16 mSetModeString[] = { ESC, '[', '=', '3', 'h', 0 };

@@ -80,6 +78,8 @@ CHAR16 mSetCursorPositionString[] = { ESC, '[', '0', '0', ';',
'0', '0', 'H', 0
CHAR16 mCursorForwardString[] = { ESC, '[', '0', '0', 'C', 0 };

CHAR16 mCursorBackwardString[] = { ESC, '[', '0', '0', 'D', 0 };



+CHAR8 SetDecModeString[] = {ESC, 0x28, 0x30};

+CHAR8 ExitDecModeString[] = {ESC, 0x28, 0x42};

//

// Body of the ConOut functions

//

@@ -183,16 +183,19 @@ TerminalConOutOutputString (
EFI_STATUS Status;

UINT8 ValidBytes;

CHAR8 CrLfStr[2];

+ CHAR8 DecChar;

+ UINTN ModeSwitchLength;

//

// flag used to indicate whether condition happens which will cause

// return EFI_WARN_UNKNOWN_GLYPH

//

BOOLEAN Warning;



- ValidBytes = 0;

- Warning = FALSE;

- AsciiChar = 0;

-

+ ValidBytes = 0;

+ Warning = FALSE;

+ AsciiChar = 0;

+ DecChar = 0;

+ ModeSwitchLength = LENGTH_DEC_ESCAPE;

//

// get Terminal device data structure pointer.

//

@@ -217,17 +220,136 @@ TerminalConOutOutputString (
for (; *WString != CHAR_NULL; WString++) {



switch (TerminalDevice->TerminalType) {

-

case TerminalTypePcAnsi:

- case TerminalTypeVt100:

- case TerminalTypeVt100Plus:

- case TerminalTypeTtyTerm:

- case TerminalTypeLinux:

+ if (!TerminalIsValidTextGraphics (*WString, &GraphicChar, &AsciiChar,
NULL)) {

+ //

+ // If it's not a graphic character convert Unicode to ASCII.

+ //

+ GraphicChar = (CHAR8)*WString;

Please help to fix the space indent (extra spaces) here to keep it aligned with the contexts.



+

+ if (!(TerminalIsValidAscii (GraphicChar) || TerminalIsValidEfiCntlChar
(GraphicChar))) {

+ //

+ // when this driver use the OutputString to output control string,

+ // TerminalDevice->OutputEscChar is set to let the Esc char

+ // to be output to the terminal emulation software.

+ //

+ if ((GraphicChar == 27) && TerminalDevice->OutputEscChar) {

+ GraphicChar = 27;

+ } else {

+ GraphicChar = '?';

+ Warning = TRUE;

+ }

+ }

+

+ AsciiChar = GraphicChar;

+ }

+ Length = 1;

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &Length,

+ &GraphicChar

+ );

Please help to add an extra space for the above 3 lines for space indent coding style.



+

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ break;

case TerminalTypeXtermR6:

- case TerminalTypeVt400:

case TerminalTypeSCO:

For the below added codes (for case TerminalTypeXtermR6 & TerminalTypeSCO), please help to fix below coding style issues:
a) Codes within a 'case' or 'if-else' statement is recommended to add 2 spaces for indent. (The space indent of the below chunk of codes are messy, which impacts the readability.)
b) For multi-line function calls, the input parameters (including the closing bracket) are recommended to add 2 spaces from the function name in the 1st line.
c) Please update multi-line comment to align with style:
//
// Comments...
//


+ if (TerminalIsValidTextGraphics (*WString, NULL, NULL, &DecChar)
&& !TerminalIsValidAscii (*WString)) {

+ //Box graphics are split into 2 types simple and advanced

+ //simple are drawn with dec special graphics

+ //advanced are drawn with utf8

+ //This checks for simple because they have a lower value than the
advanced

+ if(*WString < BOXDRAW_DOUBLE_HORIZONTAL) {

+ if (!TerminalDevice->DecSpecialGraphicsMode) {

+ ValidBytes = 0;

+ ModeSwitchLength = LENGTH_DEC_ESCAPE;

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &ModeSwitchLength,

+ (UINT8 *)SetDecModeString

+ );

+ TerminalDevice->DecSpecialGraphicsMode = TRUE;

+ }



- if (!TerminalIsValidTextGraphics (*WString, &GraphicChar, &AsciiChar)) {

+ GraphicChar = DecChar;

+ Length = 1;

+ } else {

+ if (TerminalDevice->DecSpecialGraphicsMode) {

+ ModeSwitchLength = LENGTH_DEC_ESCAPE;

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &ModeSwitchLength,

+ (UINT8 *)ExitDecModeString

+ );

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ TerminalDevice->DecSpecialGraphicsMode = FALSE;

+ }

+

+ UnicodeToUtf8 (*WString, &Utf8Char, &ValidBytes);

+ Length = ValidBytes;

+ }

+ } else {

+ if (TerminalDevice->DecSpecialGraphicsMode) {

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &ModeSwitchLength,

+ (UINT8 *)ExitDecModeString

+ );

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ TerminalDevice->DecSpecialGraphicsMode = FALSE;

+ }

+

+ GraphicChar = (CHAR8)*WString;

+

+ if (!(TerminalIsValidAscii (GraphicChar) || TerminalIsValidEfiCntlChar
(GraphicChar))) {

+ //

+ // when this driver use the OutputString to output control string,

+ // TerminalDevice->OutputEscChar is set to let the Esc char

+ // to be output to the terminal emulation software.

+ //

+ if ((GraphicChar == 27) && TerminalDevice->OutputEscChar) {

+ GraphicChar = 27;

+ Length = 1;

+ } else {

+ UnicodeToUtf8 (*WString, &Utf8Char, &ValidBytes);

+ Length = ValidBytes;

+ }

+ } else {

+ Length = 1;

+ }

+ }

+

+ if (ValidBytes) {

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &Length,

+ (UINT8 *)&Utf8Char

+ );

+ ValidBytes = 0;

+ } else {

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &Length,

+ &GraphicChar

+ );

+ }

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ break;

+ case TerminalTypeVt100:

+ case TerminalTypeTtyTerm:

+ if (!TerminalIsValidTextGraphics (*WString, &GraphicChar, &AsciiChar,
NULL)) {

//

// If it's not a graphic character convert Unicode to ASCII.

//

@@ -248,12 +370,9 @@ TerminalConOutOutputString (
}



AsciiChar = GraphicChar;

-

}



- if (TerminalDevice->TerminalType != TerminalTypePcAnsi) {

- GraphicChar = AsciiChar;

- }

+ GraphicChar = AsciiChar;



Length = 1;



@@ -267,8 +386,67 @@ TerminalConOutOutputString (
goto OutputError;

}



- break;

+ break;

+ case TerminalTypeVt100Plus:

+ case TerminalTypeVt400:

For the below added codes (for case TerminalTypeVt100Plus & TerminalTypeVt400), please help to fix below coding style issues:
a) Codes within a 'case' or 'if-else' statement is recommended to add 2 spaces for indent. (The space indent of the below chunk of codes are messy, which impacts the readability.)
b) For multi-line function calls, the input parameters (including the closing bracket) are recommended to add 2 spaces from the function name in the 1st line.

Best Regards,
Hao Wu



+ Length = 1;

+ if (TerminalIsValidTextGraphics (*WString, NULL, NULL, &DecChar)) {

+ if (!TerminalDevice->DecSpecialGraphicsMode) {

+ ModeSwitchLength = LENGTH_DEC_ESCAPE;

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &ModeSwitchLength,

+ (UINT8 *)SetDecModeString

+ );

+ TerminalDevice->DecSpecialGraphicsMode = TRUE;

+ }



+ GraphicChar = DecChar;

+ } else {

+ if (TerminalDevice->DecSpecialGraphicsMode) {

+ ModeSwitchLength = LENGTH_DEC_ESCAPE;

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &ModeSwitchLength,

+ (UINT8 *)ExitDecModeString

+ );

+

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ TerminalDevice->DecSpecialGraphicsMode = FALSE;

+ }

+

+ GraphicChar = (CHAR8)*WString;

+

+ if (!(TerminalIsValidAscii (GraphicChar) || TerminalIsValidEfiCntlChar
(GraphicChar))) {

+ //

+ // when this driver use the OutputString to output control string,

+ // TerminalDevice->OutputEscChar is set to let the Esc char

+ // to be output to the terminal emulation software.

+ //

+ if ((GraphicChar == 27) && TerminalDevice->OutputEscChar) {

+ GraphicChar = 27;

+ } else {

+ GraphicChar = '?';

+ Warning = TRUE;

+ }

+ }

+ }

+

+ Status = TerminalDevice->SerialIo->Write (

+ TerminalDevice->SerialIo,

+ &Length,

+ &GraphicChar

+ );

+

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ break;

+ case TerminalTypeLinux:

case TerminalTypeVtUtf8:

UnicodeToUtf8 (*WString, &Utf8Char, &ValidBytes);

Length = ValidBytes;

@@ -280,8 +458,10 @@ TerminalConOutOutputString (
if (EFI_ERROR (Status)) {

goto OutputError;

}

+

break;

}

+

//

// Update cursor position.

//

@@ -875,7 +1055,8 @@ BOOLEAN
TerminalIsValidTextGraphics (

IN CHAR16 Graphic,

OUT CHAR8 *PcAnsi, OPTIONAL

- OUT CHAR8 *Ascii OPTIONAL

+ OUT CHAR8 *Ascii, OPTIONAL

+ OUT CHAR8 *DecSpecialGraphics OPTIONAL

)

{

UNICODE_TO_CHAR *Table;

@@ -897,6 +1078,9 @@ TerminalIsValidTextGraphics (
if (Ascii != NULL) {

*Ascii = Table->Ascii;

}

+ if (DecSpecialGraphics != NULL){

+ *DecSpecialGraphics = Table->DecSpecialGraphics;

+ }



return TRUE;

}

--
2.32.0


Re: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Fix memory corruption

Marvin Häuser <mhaeuser@...>
 

On 09/08/2021 06:20, Ni, Ray wrote:
It's so lucky that no code calls AllocatePool so the bug didn't cause real issues. (I tried to remove AllocatePool() and build still passed.)

Thanks for catching the bug. Reviewed-by: Ray Ni <ray.ni@...>

Can you kindly share how you found this issue?
Hey Ray,

clang-tidy gave me a hand. :)
"Suspicious usage of 'sizeof(K)'; did you mean 'K'?
clang-tidy(bugprone-sizeof-expression)"

I set it up as follows (this is *not* sophisticated, just added things to quickly move on):
https://github.com/tianocore/edk2-staging/blob/2021-gsoc-secure-loader/compile_flags.txt

Best regards,
Marvin


Thanks,
Ray

-----Original Message-----
From: Marvin Häuser <mhaeuser@...>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Ni, Ray <ray.ni@...>; Ma, Maurice <maurice.ma@...>; You, Benjamin <benjamin.you@...>; Vitaly Cheptsov <vit9696@...>
Subject: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Fix memory corruption

UefiPayloadEntry's AllocatePool() applies the "sizeof" operator to
HOB index rather than the HOB header structure. This yields 4 Bytes
compared to the 8 Bytes the structure header requires. Fix the call
to allocate the required space instead.

Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c b/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
index 1204573b3e09..f3494969e5ac 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
@@ -163,7 +163,7 @@ AllocatePool (
return NULL;

}


- Hob = (EFI_HOB_MEMORY_POOL *)CreateHob (EFI_HOB_TYPE_MEMORY_POOL, (UINT16)(sizeof (EFI_HOB_TYPE_MEMORY_POOL) + AllocationSize));

+ Hob = (EFI_HOB_MEMORY_POOL *)CreateHob (EFI_HOB_TYPE_MEMORY_POOL, (UINT16)(sizeof (EFI_HOB_MEMORY_POOL) + AllocationSize));

return (VOID *)(Hob + 1);

}


Re: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands

Marvin Häuser <mhaeuser@...>
 

Hey Jiewen,

Good point, sure. Will probably add ASSERTs as well, if that is fine with you.

Best regards,
Marvin

On 09/08/2021 07:33, Yao, Jiewen wrote:
Can we define 3 and 4 to be "reserved and do not use", instead of removed ?
-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3
-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4

For example:
Command 0x3 and 0x4 are deprecated and reserved. They should not be used in the future.

With that changed, reviewed-by: Jiewen Yao <Jiewen.yao@...>


-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Monday, August 9, 2021 12:24 PM
To: mhaeuser@...; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Dong, Eric <eric.dong@...>; Vitaly Cheptsov <vit9696@...>;
Zeng, Star <star.zeng@...>; Yao, Jiewen <jiewen.yao@...>
Subject: RE: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image
profiling commands

Reviewed-by: Ray Ni <ray.ni@...>

+Star and Jiewen for confirmation.

-----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@...>;
Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Vitaly Cheptsov
<vit9696@...>
Subject: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling
commands

The legacy codebase allowed SMM images to be registered for profiling
from DXE. Support for this has been dropped entirely, so remove the
remaining handlers.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 80 --------------------
MdeModulePkg/Include/Guid/MemoryProfile.h | 5 --
2 files changed, 85 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index 1b302c810cc9..7316df7531fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -2232,64 +2232,6 @@ Done:
mSmramProfileGettingStatus = SmramProfileGettingStatus;

}



-/**

- SMRAM profile handler to register SMM image.

-

- @param SmramProfileParameterRegisterImage The parameter of SMM
profile register image.

-

-**/

-VOID

-SmramProfileHandlerRegisterImage (

- IN SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE
*SmramProfileParameterRegisterImage

- )

-{

- EFI_STATUS Status;

- EFI_SMM_DRIVER_ENTRY DriverEntry;

- VOID *EntryPointInImage;

-

- ZeroMem (&DriverEntry, sizeof (DriverEntry));

- CopyMem (&DriverEntry.FileName, &SmramProfileParameterRegisterImage-
FileName, sizeof(EFI_GUID));
- DriverEntry.ImageBuffer = SmramProfileParameterRegisterImage-
ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterRegisterImage-
NumberOfPage;
- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN)
DriverEntry.ImageBuffer, &EntryPointInImage);

- ASSERT_EFI_ERROR (Status);

- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN)
EntryPointInImage;

-

- Status = RegisterSmramProfileImage (&DriverEntry, FALSE);

- if (!EFI_ERROR (Status)) {

- SmramProfileParameterRegisterImage->Header.ReturnStatus = 0;

- }

-}

-

-/**

- SMRAM profile handler to unregister SMM image.

-

- @param SmramProfileParameterUnregisterImage The parameter of SMM
profile unregister image.

-

-**/

-VOID

-SmramProfileHandlerUnregisterImage (

- IN SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE
*SmramProfileParameterUnregisterImage

- )

-{

- EFI_STATUS Status;

- EFI_SMM_DRIVER_ENTRY DriverEntry;

- VOID *EntryPointInImage;

-

- ZeroMem (&DriverEntry, sizeof (DriverEntry));

- CopyMem (&DriverEntry.FileName,
&SmramProfileParameterUnregisterImage->FileName, sizeof (EFI_GUID));

- DriverEntry.ImageBuffer = SmramProfileParameterUnregisterImage-
ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN)
SmramProfileParameterUnregisterImage->NumberOfPage;

- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN)
DriverEntry.ImageBuffer, &EntryPointInImage);

- ASSERT_EFI_ERROR (Status);

- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN)
EntryPointInImage;

-

- Status = UnregisterSmramProfileImage (&DriverEntry, FALSE);

- if (!EFI_ERROR (Status)) {

- SmramProfileParameterUnregisterImage->Header.ReturnStatus = 0;

- }

-}

-

/**

Dispatch function for a Software SMI handler.



@@ -2374,28 +2316,6 @@ SmramProfileHandler (
}

SmramProfileHandlerGetDataByOffset
((SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET *) (UINTN)
CommBuffer);

break;

- case SMRAM_PROFILE_COMMAND_REGISTER_IMAGE:

- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerRegisterImage\n"));

- if (TempCommBufferSize != sizeof
(SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE)) {

- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer
size invalid!\n"));

- return EFI_SUCCESS;

- }

- if (mSmramReadyToLock) {

- return EFI_SUCCESS;

- }

- SmramProfileHandlerRegisterImage
((SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *) (UINTN) CommBuffer);

- break;

- case SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE:

- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerUnregisterImage\n"));

- if (TempCommBufferSize != sizeof
(SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE)) {

- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer
size invalid!\n"));

- return EFI_SUCCESS;

- }

- if (mSmramReadyToLock) {

- return EFI_SUCCESS;

- }

- SmramProfileHandlerUnregisterImage
((SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *) (UINTN) CommBuffer);

- break;

case SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE:

DEBUG ((EFI_D_ERROR, "SmramProfileHandlerGetRecordingState\n"));

if (TempCommBufferSize != sizeof
(SMRAM_PROFILE_PARAMETER_RECORDING_STATE)) {

diff --git a/MdeModulePkg/Include/Guid/MemoryProfile.h
b/MdeModulePkg/Include/Guid/MemoryProfile.h
index eee3b9125240..92cd1e7cf493 100644
--- a/MdeModulePkg/Include/Guid/MemoryProfile.h
+++ b/MdeModulePkg/Include/Guid/MemoryProfile.h
@@ -388,11 +388,6 @@ struct _EDKII_MEMORY_PROFILE_PROTOCOL {
//

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_INFO 0x1

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA 0x2

-//

-// Below 2 commands are now used by ECP only and only valid before
SmmReadyToLock

-//

-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3

-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4



#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA_BY_OFFSET 0x5

#define SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE 0x6

--
2.31.1



Re: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256 hash in dbx

Marvin Häuser <mhaeuser@...>
 

Hey Jiewen,

Right, I meant to ask about this and forgot (sorry, I sent out a bit less than 30 patches yesterday :) ).
Why do we record and potentially defer the loading of images that are distrusted by dbx?
I would expect any image explicitly distrusted (not just untrusted) to be rejected and unloaded immediately.

Sorry if I got wrong what is happening!

Best regards,
Marvin

On 09/08/2021 04:48, Yao, Jiewen wrote:
Hi Marvin
With this patch, the path "Action == EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND" no longer exists.

Do you think we should remove EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND as well?



Thank you
Yao Jiewen


-----Original Message-----
From: Marvin Häuser <mhaeuser@...>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Xu, Min M <min.m.xu@...>; Vitaly Cheptsov <vit9696@...>
Subject: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256
hash in dbx

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3461

The UEFI specification prohibits loading any UEFI image of which a
matching SHA-256 hash is contained in "dbx" (UEFI 2.9, 32.5.3.3
"Authorization Process", 3.A). Currently, this is only explicitly
checked when the image is unsigned and otherwise the hash algorithms
of the certificates are used.

Align with the UEFI specification by specifically looking up the
SHA-256 hash of the image in "dbx".

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 60
++++++++------------
1 file changed, 24 insertions(+), 36 deletions(-)

diff --git
a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index c48861cd6496..1f9bb33e86c3 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1803,34 +1803,36 @@ DxeImageVerificationHandler (
}

}



+ //

+ // The SHA256 hash value of the image must not be reflected in the security
data base "dbx".

+ //

+ if (!HashPeImage (HASHALG_SHA256)) {

+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image
using %s.\n", mHashTypeStr));

+ goto Failed;

+ }

+

+ DbStatus = IsSignatureFoundInDatabase (

+ EFI_IMAGE_SECURITY_DATABASE1,

+ mImageDigest,

+ &mCertType,

+ mImageDigestSize,

+ &IsFound

+ );

+ if (EFI_ERROR (DbStatus) || IsFound) {

+ //

+ // Image Hash is in forbidden database (DBX).

+ //

+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed
and %s hash of image is forbidden by DBX.\n", mHashTypeStr));

+ goto Failed;

+ }

+

//

// Start Image Validation.

//

if (SecDataDir == NULL || SecDataDir->Size == 0) {

//

- // This image is not signed. The SHA256 hash value of the image must match
a record in the security database "db",

- // and not be reflected in the security data base "dbx".

+ // This image is not signed. The SHA256 hash value of the image must match
a record in the security database "db".

//

- if (!HashPeImage (HASHALG_SHA256)) {

- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image
using %s.\n", mHashTypeStr));

- goto Failed;

- }

-

- DbStatus = IsSignatureFoundInDatabase (

- EFI_IMAGE_SECURITY_DATABASE1,

- mImageDigest,

- &mCertType,

- mImageDigestSize,

- &IsFound

- );

- if (EFI_ERROR (DbStatus) || IsFound) {

- //

- // Image Hash is in forbidden database (DBX).

- //

- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed
and %s hash of image is forbidden by DBX.\n", mHashTypeStr));

- goto Failed;

- }

-

DbStatus = IsSignatureFoundInDatabase (

EFI_IMAGE_SECURITY_DATABASE,

mImageDigest,

@@ -1932,20 +1934,6 @@ DxeImageVerificationHandler (
//

// Check the image's hash value.

//

- DbStatus = IsSignatureFoundInDatabase (

- EFI_IMAGE_SECURITY_DATABASE1,

- mImageDigest,

- &mCertType,

- mImageDigestSize,

- &IsFound

- );

- if (EFI_ERROR (DbStatus) || IsFound) {

- Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;

- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s
hash of image is found in DBX.\n", mHashTypeStr));

- IsVerified = FALSE;

- break;

- }

-

if (!IsVerified) {

DbStatus = IsSignatureFoundInDatabase (

EFI_IMAGE_SECURITY_DATABASE,

--
2.31.1



Re: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands

Yao, Jiewen
 

Can we define 3 and 4 to be "reserved and do not use", instead of removed ?
-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3
-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4

For example:
Command 0x3 and 0x4 are deprecated and reserved. They should not be used in the future.

With that changed, reviewed-by: Jiewen Yao <Jiewen.yao@...>

-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Monday, August 9, 2021 12:24 PM
To: mhaeuser@...; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Dong, Eric <eric.dong@...>; Vitaly Cheptsov <vit9696@...>;
Zeng, Star <star.zeng@...>; Yao, Jiewen <jiewen.yao@...>
Subject: RE: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image
profiling commands

Reviewed-by: Ray Ni <ray.ni@...>

+Star and Jiewen for confirmation.

-----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@...>;
Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Vitaly Cheptsov
<vit9696@...>
Subject: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling
commands

The legacy codebase allowed SMM images to be registered for profiling
from DXE. Support for this has been dropped entirely, so remove the
remaining handlers.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 80 --------------------
MdeModulePkg/Include/Guid/MemoryProfile.h | 5 --
2 files changed, 85 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index 1b302c810cc9..7316df7531fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -2232,64 +2232,6 @@ Done:
mSmramProfileGettingStatus = SmramProfileGettingStatus;

}



-/**

- SMRAM profile handler to register SMM image.

-

- @param SmramProfileParameterRegisterImage The parameter of SMM
profile register image.

-

-**/

-VOID

-SmramProfileHandlerRegisterImage (

- IN SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE
*SmramProfileParameterRegisterImage

- )

-{

- EFI_STATUS Status;

- EFI_SMM_DRIVER_ENTRY DriverEntry;

- VOID *EntryPointInImage;

-

- ZeroMem (&DriverEntry, sizeof (DriverEntry));

- CopyMem (&DriverEntry.FileName, &SmramProfileParameterRegisterImage-
FileName, sizeof(EFI_GUID));
- DriverEntry.ImageBuffer = SmramProfileParameterRegisterImage-
ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterRegisterImage-
NumberOfPage;
- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN)
DriverEntry.ImageBuffer, &EntryPointInImage);

- ASSERT_EFI_ERROR (Status);

- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN)
EntryPointInImage;

-

- Status = RegisterSmramProfileImage (&DriverEntry, FALSE);

- if (!EFI_ERROR (Status)) {

- SmramProfileParameterRegisterImage->Header.ReturnStatus = 0;

- }

-}

-

-/**

- SMRAM profile handler to unregister SMM image.

-

- @param SmramProfileParameterUnregisterImage The parameter of SMM
profile unregister image.

-

-**/

-VOID

-SmramProfileHandlerUnregisterImage (

- IN SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE
*SmramProfileParameterUnregisterImage

- )

-{

- EFI_STATUS Status;

- EFI_SMM_DRIVER_ENTRY DriverEntry;

- VOID *EntryPointInImage;

-

- ZeroMem (&DriverEntry, sizeof (DriverEntry));

- CopyMem (&DriverEntry.FileName,
&SmramProfileParameterUnregisterImage->FileName, sizeof (EFI_GUID));

- DriverEntry.ImageBuffer = SmramProfileParameterUnregisterImage-
ImageBuffer;
- DriverEntry.NumberOfPage = (UINTN)
SmramProfileParameterUnregisterImage->NumberOfPage;

- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN)
DriverEntry.ImageBuffer, &EntryPointInImage);

- ASSERT_EFI_ERROR (Status);

- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN)
EntryPointInImage;

-

- Status = UnregisterSmramProfileImage (&DriverEntry, FALSE);

- if (!EFI_ERROR (Status)) {

- SmramProfileParameterUnregisterImage->Header.ReturnStatus = 0;

- }

-}

-

/**

Dispatch function for a Software SMI handler.



@@ -2374,28 +2316,6 @@ SmramProfileHandler (
}

SmramProfileHandlerGetDataByOffset
((SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET *) (UINTN)
CommBuffer);

break;

- case SMRAM_PROFILE_COMMAND_REGISTER_IMAGE:

- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerRegisterImage\n"));

- if (TempCommBufferSize != sizeof
(SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE)) {

- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer
size invalid!\n"));

- return EFI_SUCCESS;

- }

- if (mSmramReadyToLock) {

- return EFI_SUCCESS;

- }

- SmramProfileHandlerRegisterImage
((SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *) (UINTN) CommBuffer);

- break;

- case SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE:

- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerUnregisterImage\n"));

- if (TempCommBufferSize != sizeof
(SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE)) {

- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer
size invalid!\n"));

- return EFI_SUCCESS;

- }

- if (mSmramReadyToLock) {

- return EFI_SUCCESS;

- }

- SmramProfileHandlerUnregisterImage
((SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *) (UINTN) CommBuffer);

- break;

case SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE:

DEBUG ((EFI_D_ERROR, "SmramProfileHandlerGetRecordingState\n"));

if (TempCommBufferSize != sizeof
(SMRAM_PROFILE_PARAMETER_RECORDING_STATE)) {

diff --git a/MdeModulePkg/Include/Guid/MemoryProfile.h
b/MdeModulePkg/Include/Guid/MemoryProfile.h
index eee3b9125240..92cd1e7cf493 100644
--- a/MdeModulePkg/Include/Guid/MemoryProfile.h
+++ b/MdeModulePkg/Include/Guid/MemoryProfile.h
@@ -388,11 +388,6 @@ struct _EDKII_MEMORY_PROFILE_PROTOCOL {
//

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_INFO 0x1

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA 0x2

-//

-// Below 2 commands are now used by ECP only and only valid before
SmmReadyToLock

-//

-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3

-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4



#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA_BY_OFFSET 0x5

#define SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE 0x6

--
2.31.1


Re: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256 hash in dbx

Marvin Häuser <mhaeuser@...>
 

Good day,

I just woke up to this mess, yes. I actually did follow that guide, just around 3 years ago I believe, so let me check where things went wrong since then...
The patch you quoted was a standalone patch. However, 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 02:02, Min Xu wrote:
On August 9, 2021 3:40 AM, Marvin Häuser wrote:
Subject: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256
hash in dbx

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3461

The UEFI specification prohibits loading any UEFI image of which a matching
SHA-256 hash is contained in "dbx" (UEFI 2.9, 32.5.3.3 "Authorization Process",
3.A). Currently, this is only explicitly checked when the image is unsigned and
otherwise the hash algorithms of the certificates are used.

Align with the UEFI specification by specifically looking up the
SHA-256 hash of the image in "dbx".

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
It seems there are 3 patches sent from Marvin Häuser and I suppose they're in one patch-set, right? Please follow the link below to send out patch-set for review.
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
For example, if there are 3 commits in one patch-set, then the subject of the commits looks like:
[PATCH 0/4] This is the cover letter
[PATCH 1/4] This is patch 1

Otherwise the reviewers are confused by the patches.

Thanks!
Xu, Min




Re: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands

Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@...>

+Star and Jiewen for confirmation.

-----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@...>; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Vitaly Cheptsov <vit9696@...>
Subject: [PATCH] MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands

The legacy codebase allowed SMM images to be registered for profiling
from DXE. Support for this has been dropped entirely, so remove the
remaining handlers.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 80 --------------------
MdeModulePkg/Include/Guid/MemoryProfile.h | 5 --
2 files changed, 85 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index 1b302c810cc9..7316df7531fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -2232,64 +2232,6 @@ Done:
mSmramProfileGettingStatus = SmramProfileGettingStatus;

}



-/**

- SMRAM profile handler to register SMM image.

-

- @param SmramProfileParameterRegisterImage The parameter of SMM profile register image.

-

-**/

-VOID

-SmramProfileHandlerRegisterImage (

- IN SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *SmramProfileParameterRegisterImage

- )

-{

- EFI_STATUS Status;

- EFI_SMM_DRIVER_ENTRY DriverEntry;

- VOID *EntryPointInImage;

-

- ZeroMem (&DriverEntry, sizeof (DriverEntry));

- CopyMem (&DriverEntry.FileName, &SmramProfileParameterRegisterImage->FileName, sizeof(EFI_GUID));

- DriverEntry.ImageBuffer = SmramProfileParameterRegisterImage->ImageBuffer;

- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterRegisterImage->NumberOfPage;

- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN) DriverEntry.ImageBuffer, &EntryPointInImage);

- ASSERT_EFI_ERROR (Status);

- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN) EntryPointInImage;

-

- Status = RegisterSmramProfileImage (&DriverEntry, FALSE);

- if (!EFI_ERROR (Status)) {

- SmramProfileParameterRegisterImage->Header.ReturnStatus = 0;

- }

-}

-

-/**

- SMRAM profile handler to unregister SMM image.

-

- @param SmramProfileParameterUnregisterImage The parameter of SMM profile unregister image.

-

-**/

-VOID

-SmramProfileHandlerUnregisterImage (

- IN SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *SmramProfileParameterUnregisterImage

- )

-{

- EFI_STATUS Status;

- EFI_SMM_DRIVER_ENTRY DriverEntry;

- VOID *EntryPointInImage;

-

- ZeroMem (&DriverEntry, sizeof (DriverEntry));

- CopyMem (&DriverEntry.FileName, &SmramProfileParameterUnregisterImage->FileName, sizeof (EFI_GUID));

- DriverEntry.ImageBuffer = SmramProfileParameterUnregisterImage->ImageBuffer;

- DriverEntry.NumberOfPage = (UINTN) SmramProfileParameterUnregisterImage->NumberOfPage;

- Status = InternalPeCoffGetEntryPoint ((VOID *) (UINTN) DriverEntry.ImageBuffer, &EntryPointInImage);

- ASSERT_EFI_ERROR (Status);

- DriverEntry.ImageEntryPoint = (PHYSICAL_ADDRESS) (UINTN) EntryPointInImage;

-

- Status = UnregisterSmramProfileImage (&DriverEntry, FALSE);

- if (!EFI_ERROR (Status)) {

- SmramProfileParameterUnregisterImage->Header.ReturnStatus = 0;

- }

-}

-

/**

Dispatch function for a Software SMI handler.



@@ -2374,28 +2316,6 @@ SmramProfileHandler (
}

SmramProfileHandlerGetDataByOffset ((SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET *) (UINTN) CommBuffer);

break;

- case SMRAM_PROFILE_COMMAND_REGISTER_IMAGE:

- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerRegisterImage\n"));

- if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE)) {

- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer size invalid!\n"));

- return EFI_SUCCESS;

- }

- if (mSmramReadyToLock) {

- return EFI_SUCCESS;

- }

- SmramProfileHandlerRegisterImage ((SMRAM_PROFILE_PARAMETER_REGISTER_IMAGE *) (UINTN) CommBuffer);

- break;

- case SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE:

- DEBUG ((EFI_D_ERROR, "SmramProfileHandlerUnregisterImage\n"));

- if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE)) {

- DEBUG ((EFI_D_ERROR, "SmramProfileHandler: SMM communication buffer size invalid!\n"));

- return EFI_SUCCESS;

- }

- if (mSmramReadyToLock) {

- return EFI_SUCCESS;

- }

- SmramProfileHandlerUnregisterImage ((SMRAM_PROFILE_PARAMETER_UNREGISTER_IMAGE *) (UINTN) CommBuffer);

- break;

case SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE:

DEBUG ((EFI_D_ERROR, "SmramProfileHandlerGetRecordingState\n"));

if (TempCommBufferSize != sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)) {

diff --git a/MdeModulePkg/Include/Guid/MemoryProfile.h b/MdeModulePkg/Include/Guid/MemoryProfile.h
index eee3b9125240..92cd1e7cf493 100644
--- a/MdeModulePkg/Include/Guid/MemoryProfile.h
+++ b/MdeModulePkg/Include/Guid/MemoryProfile.h
@@ -388,11 +388,6 @@ struct _EDKII_MEMORY_PROFILE_PROTOCOL {
//

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_INFO 0x1

#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA 0x2

-//

-// Below 2 commands are now used by ECP only and only valid before SmmReadyToLock

-//

-#define SMRAM_PROFILE_COMMAND_REGISTER_IMAGE 0x3

-#define SMRAM_PROFILE_COMMAND_UNREGISTER_IMAGE 0x4



#define SMRAM_PROFILE_COMMAND_GET_PROFILE_DATA_BY_OFFSET 0x5

#define SMRAM_PROFILE_COMMAND_GET_RECORDING_STATE 0x6

--
2.31.1


Re: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Fix memory corruption

Ni, Ray
 

It's so lucky that no code calls AllocatePool so the bug didn't cause real issues. (I tried to remove AllocatePool() and build still passed.)

Thanks for catching the bug. Reviewed-by: Ray Ni <ray.ni@...>

Can you kindly share how you found this issue?

Thanks,
Ray

-----Original Message-----
From: Marvin Häuser <mhaeuser@...>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Ni, Ray <ray.ni@...>; Ma, Maurice <maurice.ma@...>; You, Benjamin <benjamin.you@...>; Vitaly Cheptsov <vit9696@...>
Subject: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Fix memory corruption

UefiPayloadEntry's AllocatePool() applies the "sizeof" operator to
HOB index rather than the HOB header structure. This yields 4 Bytes
compared to the 8 Bytes the structure header requires. Fix the call
to allocate the required space instead.

Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c b/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
index 1204573b3e09..f3494969e5ac 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/MemoryAllocation.c
@@ -163,7 +163,7 @@ AllocatePool (
return NULL;

}



- Hob = (EFI_HOB_MEMORY_POOL *)CreateHob (EFI_HOB_TYPE_MEMORY_POOL, (UINT16)(sizeof (EFI_HOB_TYPE_MEMORY_POOL) + AllocationSize));

+ Hob = (EFI_HOB_MEMORY_POOL *)CreateHob (EFI_HOB_TYPE_MEMORY_POOL, (UINT16)(sizeof (EFI_HOB_MEMORY_POOL) + AllocationSize));

return (VOID *)(Hob + 1);

}



--
2.31.1


Re: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256 hash in dbx

Yao, Jiewen
 

Hi Marvin
With this patch, the path "Action == EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND" no longer exists.

Do you think we should remove EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND as well?



Thank you
Yao Jiewen

-----Original Message-----
From: Marvin Häuser <mhaeuser@...>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>;
Xu, Min M <min.m.xu@...>; Vitaly Cheptsov <vit9696@...>
Subject: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256
hash in dbx

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3461

The UEFI specification prohibits loading any UEFI image of which a
matching SHA-256 hash is contained in "dbx" (UEFI 2.9, 32.5.3.3
"Authorization Process", 3.A). Currently, this is only explicitly
checked when the image is unsigned and otherwise the hash algorithms
of the certificates are used.

Align with the UEFI specification by specifically looking up the
SHA-256 hash of the image in "dbx".

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Min Xu <min.m.xu@...>
Cc: Vitaly Cheptsov <vit9696@...>
Signed-off-by: Marvin Häuser <mhaeuser@...>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 60
++++++++------------
1 file changed, 24 insertions(+), 36 deletions(-)

diff --git
a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index c48861cd6496..1f9bb33e86c3 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1803,34 +1803,36 @@ DxeImageVerificationHandler (
}

}



+ //

+ // The SHA256 hash value of the image must not be reflected in the security
data base "dbx".

+ //

+ if (!HashPeImage (HASHALG_SHA256)) {

+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image
using %s.\n", mHashTypeStr));

+ goto Failed;

+ }

+

+ DbStatus = IsSignatureFoundInDatabase (

+ EFI_IMAGE_SECURITY_DATABASE1,

+ mImageDigest,

+ &mCertType,

+ mImageDigestSize,

+ &IsFound

+ );

+ if (EFI_ERROR (DbStatus) || IsFound) {

+ //

+ // Image Hash is in forbidden database (DBX).

+ //

+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed
and %s hash of image is forbidden by DBX.\n", mHashTypeStr));

+ goto Failed;

+ }

+

//

// Start Image Validation.

//

if (SecDataDir == NULL || SecDataDir->Size == 0) {

//

- // This image is not signed. The SHA256 hash value of the image must match
a record in the security database "db",

- // and not be reflected in the security data base "dbx".

+ // This image is not signed. The SHA256 hash value of the image must match
a record in the security database "db".

//

- if (!HashPeImage (HASHALG_SHA256)) {

- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image
using %s.\n", mHashTypeStr));

- goto Failed;

- }

-

- DbStatus = IsSignatureFoundInDatabase (

- EFI_IMAGE_SECURITY_DATABASE1,

- mImageDigest,

- &mCertType,

- mImageDigestSize,

- &IsFound

- );

- if (EFI_ERROR (DbStatus) || IsFound) {

- //

- // Image Hash is in forbidden database (DBX).

- //

- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed
and %s hash of image is forbidden by DBX.\n", mHashTypeStr));

- goto Failed;

- }

-

DbStatus = IsSignatureFoundInDatabase (

EFI_IMAGE_SECURITY_DATABASE,

mImageDigest,

@@ -1932,20 +1934,6 @@ DxeImageVerificationHandler (
//

// Check the image's hash value.

//

- DbStatus = IsSignatureFoundInDatabase (

- EFI_IMAGE_SECURITY_DATABASE1,

- mImageDigest,

- &mCertType,

- mImageDigestSize,

- &IsFound

- );

- if (EFI_ERROR (DbStatus) || IsFound) {

- Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;

- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s
hash of image is found in DBX.\n", mHashTypeStr));

- IsVerified = FALSE;

- break;

- }

-

if (!IsVerified) {

DbStatus = IsSignatureFoundInDatabase (

EFI_IMAGE_SECURITY_DATABASE,

--
2.31.1

13361 - 13380 of 92218