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

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

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

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

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

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;

+

+ 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

+ );

+

+ if (EFI_ERROR (Status)) {

+ goto OutputError;

+ }

+

+ break;

case TerminalTypeXtermR6:

- case TerminalTypeVt400:

case TerminalTypeSCO:

+ 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:

+ 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

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

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

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

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

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

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>
Sent: Saturday, July 17, 2021 5:18 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>;
Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>
Subject: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command
completion.

To follow the TCG CRB protocol specification, on every CRB TPM command
completion the TPM should return to Idle state, regardless of the
CRB Idle Bypass capability reported by the TPM device.

See: TCG PC Client Device Driver Design Principles for TPM 2.0,
Version 1.0, Rev 0.27

Signed-off-by: Rodrigo Gonzalez del Cueto
<rodrigo.gonzalez.del.cueto@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
---
SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
index f1f8091683..34e3874a5b 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
@@ -310,7 +310,7 @@ PtpCrbTpmCommand (
// Command completed, but buffer is not enough
//
Status = EFI_BUFFER_TOO_SMALL;
- goto GoReady_Exit;
+ goto GoIdle_Exit;
}
*SizeOut = TpmOutSize;
//
@@ -328,16 +328,6 @@ PtpCrbTpmCommand (
DEBUG ((EFI_D_VERBOSE, "\n"));
);

-GoReady_Exit:
- //
- // Goto Ready State if command is completed successfully and TPM support
IdleBypass
- // If not supported. flow down to GoIdle
- //
- if (GetCachedIdleByPass () == 1) {
- MmioWrite32((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
- return Status;
- }
-
//
// Do not wait for state transition for TIMEOUT_C
// This function will try to wait 2 TIMEOUT_C at the beginning in next call.
--
2.31.1.windows.1

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>
Sent: Friday, July 30, 2021 6:43 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>; Yao,
Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>
Subject: [PATCH] SecurityPkg: Debug code to audit BIOS TPM extend operations.

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

Add debug functionality to examine TPM extend operations
performed by BIOS and inspect the PCR 00 value prior to
any BIOS measurements.

Replaced usage of EFI_D_* for DEBUG_* definitions in debug
messages.

Signed-off-by: Rodrigo Gonzalez del Cueto
<rodrigo.gonzalez.del.cueto@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
---
SecurityPkg/Include/Library/Tpm2CommandLib.h | 28
++++++++++++++++++++++------
SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c | 226
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++-----------------------
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 34 ++++++++++++++++++++------
--------
3 files changed, 245 insertions(+), 43 deletions(-)

diff --git a/SecurityPkg/Include/Library/Tpm2CommandLib.h
b/SecurityPkg/Include/Library/Tpm2CommandLib.h
index ee8eb62295..5e5c340893 100644
--- a/SecurityPkg/Include/Library/Tpm2CommandLib.h
+++ b/SecurityPkg/Include/Library/Tpm2CommandLib.h
@@ -1,7 +1,7 @@
/** @file
This library is used by other modules to send TPM2 command.

-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -505,7 +505,7 @@ EFIAPI
Tpm2PcrEvent (
IN TPMI_DH_PCR PcrHandle,
IN TPM2B_EVENT *EventData,
- OUT TPML_DIGEST_VALUES *Digests
+ OUT TPML_DIGEST_VALUES *Digests
);

/**
@@ -522,10 +522,10 @@ Tpm2PcrEvent (
EFI_STATUS
EFIAPI
Tpm2PcrRead (
- IN TPML_PCR_SELECTION *PcrSelectionIn,
- OUT UINT32 *PcrUpdateCounter,
- OUT TPML_PCR_SELECTION *PcrSelectionOut,
- OUT TPML_DIGEST *PcrValues
+ IN TPML_PCR_SELECTION *PcrSelectionIn,
+ OUT UINT32 *PcrUpdateCounter,
+ OUT TPML_PCR_SELECTION *PcrSelectionOut,
+ OUT TPML_DIGEST *PcrValues
);

/**
@@ -1113,4 +1113,20 @@ GetDigestFromDigestList(
OUT VOID *Digest
);

+ /**
+ This function will query the TPM to determine which hashing algorithms and
+ get the digests of all active and supported PCR banks of a specific PCR
register.
+
+ @param[in] PcrHandle The index of the PCR register to be read.
+ @param[out] HashList List of digests from PCR register being read.
+
+ @retval EFI_SUCCESS The Pcr was read successfully.
+ @retval EFI_DEVICE_ERROR The command was unsuccessful.
+**/
+EFI_STATUS
+EFIAPI
+Tpm2PcrReadForActiveBank (
+ IN TPMI_DH_PCR PcrHandle,
+ OUT TPML_DIGEST *HashList
+ );
#endif
diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
index ddb15178fb..3b49192b93 100644
--- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
+++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
@@ -1,7 +1,7 @@
/** @file
Implement TPM2 Integrity related command.

-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -109,7 +109,6 @@ Tpm2PcrExtend (
Cmd.Header.commandCode = SwapBytes32(TPM_CC_PCR_Extend);
Cmd.PcrHandle = SwapBytes32(PcrHandle);

-
//
// Add in Auth session
//
@@ -130,14 +129,26 @@ Tpm2PcrExtend (
Buffer += sizeof(UINT16);
DigestSize = GetHashSizeFromAlgo (Digests->digests[Index].hashAlg);
if (DigestSize == 0) {
- DEBUG ((EFI_D_ERROR, "Unknown hash algorithm %d\r\n", Digests-

digests[Index].hashAlg));

+ DEBUG ((DEBUG_ERROR, "Unknown hash algorithm %d\r\n", Digests-

digests[Index].hashAlg));

return EFI_DEVICE_ERROR;
}
+
CopyMem(
Buffer,
&Digests->digests[Index].digest,
DigestSize
);
+
+ DEBUG_CODE_BEGIN ();
+ UINTN Index2;
+ DEBUG ((DEBUG_VERBOSE, "Tpm2PcrExtend - Hash = 0x%04x, Pcr[%02d],
digest = ", Digests->digests[Index].hashAlg, (UINT8) PcrHandle));
+
+ for (Index2 = 0; Index2 < DigestSize; Index2++) {
+ DEBUG ((DEBUG_VERBOSE, "%02x ", Buffer[Index2]));
+ }
+ DEBUG ((DEBUG_VERBOSE, "\n"));
+ DEBUG_CODE_END ();
+
Buffer += DigestSize;
}

@@ -151,7 +162,7 @@ Tpm2PcrExtend (
}

if (ResultBufSize > sizeof(Res)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrExtend: Failed ExecuteCommand: Buffer
Too Small\r\n"));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrExtend: Failed ExecuteCommand: Buffer
Too Small\r\n"));
return EFI_BUFFER_TOO_SMALL;
}

@@ -160,7 +171,7 @@ Tpm2PcrExtend (
//
RespSize = SwapBytes32(Res.Header.paramSize);
if (RespSize > sizeof(Res)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrExtend: Response size too large! %d\r\n",
RespSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrExtend: Response size too large! %d\r\n",
RespSize));
return EFI_BUFFER_TOO_SMALL;
}

@@ -168,10 +179,15 @@ Tpm2PcrExtend (
// Fail if command failed
//
if (SwapBytes32(Res.Header.responseCode) != TPM_RC_SUCCESS) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrExtend: Response Code error! 0x%08x\r\n",
SwapBytes32(Res.Header.responseCode)));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrExtend: Response Code error!
0x%08x\r\n", SwapBytes32(Res.Header.responseCode)));
return EFI_DEVICE_ERROR;
}

+ DEBUG_CODE_BEGIN ();
+ DEBUG ((DEBUG_VERBOSE, "Tpm2PcrExtend: PCR read after extend...\n"));
+ Tpm2PcrReadForActiveBank (PcrHandle, NULL);
+ DEBUG_CODE_END ();
+
//
// Unmarshal the response
//
@@ -246,7 +262,7 @@ Tpm2PcrEvent (
}

if (ResultBufSize > sizeof(Res)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrEvent: Failed ExecuteCommand: Buffer
Too Small\r\n"));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrEvent: Failed ExecuteCommand: Buffer
Too Small\r\n"));
return EFI_BUFFER_TOO_SMALL;
}

@@ -255,7 +271,7 @@ Tpm2PcrEvent (
//
RespSize = SwapBytes32(Res.Header.paramSize);
if (RespSize > sizeof(Res)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrEvent: Response size too large! %d\r\n",
RespSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrEvent: Response size too large! %d\r\n",
RespSize));
return EFI_BUFFER_TOO_SMALL;
}

@@ -263,7 +279,7 @@ Tpm2PcrEvent (
// Fail if command failed
//
if (SwapBytes32(Res.Header.responseCode) != TPM_RC_SUCCESS) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrEvent: Response Code error! 0x%08x\r\n",
SwapBytes32(Res.Header.responseCode)));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrEvent: Response Code error!
0x%08x\r\n", SwapBytes32(Res.Header.responseCode)));
return EFI_DEVICE_ERROR;
}

@@ -284,7 +300,7 @@ Tpm2PcrEvent (
Buffer += sizeof(UINT16);
DigestSize = GetHashSizeFromAlgo (Digests->digests[Index].hashAlg);
if (DigestSize == 0) {
- DEBUG ((EFI_D_ERROR, "Unknown hash algorithm %d\r\n", Digests-

digests[Index].hashAlg));

+ DEBUG ((DEBUG_ERROR, "Unknown hash algorithm %d\r\n", Digests-

digests[Index].hashAlg));

return EFI_DEVICE_ERROR;
}
CopyMem(
@@ -298,6 +314,7 @@ Tpm2PcrEvent (
return EFI_SUCCESS;
}

+
/**
This command returns the values of all PCR specified in pcrSelect.

@@ -353,11 +370,11 @@ Tpm2PcrRead (
}

if (RecvBufferSize < sizeof (TPM2_RESPONSE_HEADER)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
return EFI_DEVICE_ERROR;
}
if (SwapBytes32(RecvBuffer.Header.responseCode) != TPM_RC_SUCCESS) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrRead - responseCode - %x\n",
SwapBytes32(RecvBuffer.Header.responseCode)));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrRead - responseCode - %x\n",
SwapBytes32(RecvBuffer.Header.responseCode)));
return EFI_NOT_FOUND;
}

@@ -369,7 +386,7 @@ Tpm2PcrRead (
// PcrUpdateCounter
//
if (RecvBufferSize < sizeof (TPM2_RESPONSE_HEADER) +
sizeof(RecvBuffer.PcrUpdateCounter)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
return EFI_DEVICE_ERROR;
}
*PcrUpdateCounter = SwapBytes32(RecvBuffer.PcrUpdateCounter);
@@ -378,7 +395,7 @@ Tpm2PcrRead (
// PcrSelectionOut
//
if (RecvBufferSize < sizeof (TPM2_RESPONSE_HEADER) +
sizeof(RecvBuffer.PcrUpdateCounter) +
sizeof(RecvBuffer.PcrSelectionOut.count)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
return EFI_DEVICE_ERROR;
}
PcrSelectionOut->count = SwapBytes32(RecvBuffer.PcrSelectionOut.count);
@@ -388,7 +405,7 @@ Tpm2PcrRead (
}

if (RecvBufferSize < sizeof (TPM2_RESPONSE_HEADER) +
sizeof(RecvBuffer.PcrUpdateCounter) +
sizeof(RecvBuffer.PcrSelectionOut.count) +
sizeof(RecvBuffer.PcrSelectionOut.pcrSelections[0]) * PcrSelectionOut->count) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrRead - RecvBufferSize Error - %x\n",
RecvBufferSize));
return EFI_DEVICE_ERROR;
}
for (Index = 0; Index < PcrSelectionOut->count; Index++) {
@@ -513,7 +530,7 @@ Tpm2PcrAllocate (
}

if (ResultBufSize > sizeof(Res)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrAllocate: Failed ExecuteCommand: Buffer
Too Small\r\n"));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrAllocate: Failed ExecuteCommand:
Buffer Too Small\r\n"));
Status = EFI_BUFFER_TOO_SMALL;
goto Done;
}
@@ -523,7 +540,7 @@ Tpm2PcrAllocate (
//
RespSize = SwapBytes32(Res.Header.paramSize);
if (RespSize > sizeof(Res)) {
- DEBUG ((EFI_D_ERROR, "Tpm2PcrAllocate: Response size too large! %d\r\n",
RespSize));
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrAllocate: Response size too
large! %d\r\n", RespSize));
Status = EFI_BUFFER_TOO_SMALL;
goto Done;
}
@@ -532,7 +549,7 @@ Tpm2PcrAllocate (
// Fail if command failed
//
if (SwapBytes32(Res.Header.responseCode) != TPM_RC_SUCCESS) {
- DEBUG((EFI_D_ERROR,"Tpm2PcrAllocate: Response Code error! 0x%08x\r\n",
SwapBytes32(Res.Header.responseCode)));
+ DEBUG((DEBUG_ERROR,"Tpm2PcrAllocate: Response Code error!
0x%08x\r\n", SwapBytes32(Res.Header.responseCode)));
Status = EFI_DEVICE_ERROR;
goto Done;
}
@@ -673,17 +690,180 @@ Tpm2PcrAllocateBanks (
&SizeNeeded,
&SizeAvailable
);
- DEBUG ((EFI_D_INFO, "Tpm2PcrAllocateBanks call Tpm2PcrAllocate - %r\n",
Status));
+ DEBUG ((DEBUG_INFO, "Tpm2PcrAllocateBanks call Tpm2PcrAllocate - %r\n",
Status));
if (EFI_ERROR (Status)) {
goto Done;
}

- DEBUG ((EFI_D_INFO, "AllocationSuccess - %02x\n", AllocationSuccess));
- DEBUG ((EFI_D_INFO, "MaxPCR - %08x\n", MaxPCR));
- DEBUG ((EFI_D_INFO, "SizeNeeded - %08x\n", SizeNeeded));
- DEBUG ((EFI_D_INFO, "SizeAvailable - %08x\n", SizeAvailable));
+ DEBUG ((DEBUG_INFO, "AllocationSuccess - %02x\n", AllocationSuccess));
+ DEBUG ((DEBUG_INFO, "MaxPCR - %08x\n", MaxPCR));
+ DEBUG ((DEBUG_INFO, "SizeNeeded - %08x\n", SizeNeeded));
+ DEBUG ((DEBUG_INFO, "SizeAvailable - %08x\n", SizeAvailable));

Done:
ZeroMem(&LocalAuthSession.hmac, sizeof(LocalAuthSession.hmac));
return Status;
}
+
+/**
+ This function will query the TPM to determine which hashing algorithms and
+ get the digests of all active and supported PCR banks of a specific PCR
register.
+
+ @param[in] PcrHandle The index of the PCR register to be read.
+ @param[out] HashList List of digests from PCR register being read.
+
+ @retval EFI_SUCCESS The Pcr was read successfully.
+ @retval EFI_DEVICE_ERROR The command was unsuccessful.
+**/
+EFI_STATUS
+EFIAPI
+Tpm2PcrReadForActiveBank (
+ IN TPMI_DH_PCR PcrHandle,
+ OUT TPML_DIGEST *HashList
+)
+{
+ EFI_STATUS Status;
+ TPML_PCR_SELECTION Pcrs;
+ TPML_PCR_SELECTION PcrSelectionIn;
+ TPML_PCR_SELECTION PcrSelectionOut;
+ TPML_DIGEST PcrValues;
+ UINT32 PcrUpdateCounter;
+ UINT8 PcrIndex;
+ UINT32 TpmHashAlgorithmBitmap;
+ TPMI_ALG_HASH CurrentPcrBankHash;
+ UINT32 ActivePcrBanks;
+ UINT32 TcgRegistryHashAlg;
+ UINTN Index;
+ UINTN Index2;
+
+ PcrIndex = (UINT8) PcrHandle;
+
+ if ((PcrIndex < 0) ||
+ (PcrIndex >= IMPLEMENTATION_PCR)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ ZeroMem (&PcrSelectionIn, sizeof (PcrSelectionIn));
+ ZeroMem (&PcrUpdateCounter, sizeof (UINT32));
+ ZeroMem (&PcrSelectionOut, sizeof (PcrSelectionOut));
+ ZeroMem (&PcrValues, sizeof (PcrValues));
+ ZeroMem (&Pcrs, sizeof (TPML_PCR_SELECTION));
+
+ DEBUG ((DEBUG_INFO, "ReadPcr - %02d\n", PcrIndex));
+
+ //
+ // Read TPM capabilities
+ //
+ Status = Tpm2GetCapabilityPcrs (&Pcrs);
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "ReadPcr: Unable to read TPM capabilities\n"));
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Get Active Pcrs
+ //
+ Status = Tpm2GetCapabilitySupportedAndActivePcrs (
+ &TpmHashAlgorithmBitmap,
+ &ActivePcrBanks
+ );
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "ReadPcr: Unable to read TPM capabilities and
active PCRs\n"));
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Select from Active PCRs
+ //
+ for (Index = 0; Index < Pcrs.count; Index++) {
+ CurrentPcrBankHash = Pcrs.pcrSelections[Index].hash;
+
+ switch (CurrentPcrBankHash) {
+ case TPM_ALG_SHA1:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA1 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA1;
+ break;
+ case TPM_ALG_SHA256:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA256 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA256;
+ break;
+ case TPM_ALG_SHA384:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA384 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA384;
+ break;
+ case TPM_ALG_SHA512:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA512 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA512;
+ break;
+ case TPM_ALG_SM3_256:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SM3 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SM3_256;
+ break;
+ default:
+ //
+ // Unsupported algorithm
+ //
+ DEBUG ((DEBUG_VERBOSE, "Unknown algorithm present\n"));
+ TcgRegistryHashAlg = 0;
+ break;
+ }
+ //
+ // Skip unsupported and inactive PCR banks
+ //
+ if ((TcgRegistryHashAlg & ActivePcrBanks) == 0) {
+ DEBUG ((DEBUG_VERBOSE, "Skipping unsupported or inactive bank:
0x%04x\n", CurrentPcrBankHash));
+ continue;
+ }
+
+ //
+ // Select PCR from current active bank
+ //
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].hash =
Pcrs.pcrSelections[Index].hash;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].sizeofSelect =
PCR_SELECT_MAX;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].pcrSelect[0] = (PcrIndex <
8) ? 1 << PcrIndex : 0;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].pcrSelect[1] = (PcrIndex >
7) && (PcrIndex < 16) ? 1 << (PcrIndex - 8) : 0;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].pcrSelect[2] = (PcrIndex >
15) ? 1 << (PcrIndex - 16) : 0;
+ PcrSelectionIn.count++;
+ }
+
+ //
+ // Read PCRs
+ //
+ Status = Tpm2PcrRead (
+ &PcrSelectionIn,
+ &PcrUpdateCounter,
+ &PcrSelectionOut,
+ &PcrValues
+ );
+
+ if (EFI_ERROR (Status)) {
+ DEBUG((DEBUG_ERROR, "Tpm2PcrRead failed Status = %r \n", Status));
+ return EFI_DEVICE_ERROR;
+ }
+
+ for (Index = 0; Index < PcrValues.count; Index++) {
+ DEBUG ((
+ DEBUG_INFO,
+ "ReadPcr - HashAlg = 0x%04x, Pcr[%02d], digest = ",
+ PcrSelectionOut.pcrSelections[Index].hash,
+ PcrIndex
+ ));
+
+ for(Index2 = 0; Index2 < PcrValues.digests[Index].size; Index2++) {
+ DEBUG ((DEBUG_INFO, "%02x ", PcrValues.digests[Index].buffer[Index2]));
+ }
+ DEBUG ((DEBUG_INFO, "\n"));
+ }
+
+ if (HashList != NULL) {
+ CopyMem (
+ HashList,
+ &PcrValues,
+ sizeof (TPML_DIGEST)
+ );
+ }
+
+ return EFI_SUCCESS;
+}
diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index 93a8803ff6..ea79fa0af6 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
@@ -1,7 +1,7 @@
/** @file
Initialize TPM2 device and measure FVs before handing off control to DXE.

-Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, Microsoft Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -191,7 +191,6 @@ EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList[] = {
}
};

-
/**
Record all measured Firmware Volume Information into a Guid Hob
Guid Hob payload layout is
@@ -267,7 +266,7 @@ SyncPcrAllocationsAndPcrMask (
UINT32 Tpm2PcrMask;
UINT32 NewTpm2PcrMask;

- DEBUG ((EFI_D_ERROR, "SyncPcrAllocationsAndPcrMask!\n"));
+ DEBUG ((DEBUG_ERROR, "SyncPcrAllocationsAndPcrMask!\n"));

//
// Determine the current TPM support and the Platform PCR mask.
@@ -278,7 +277,7 @@ SyncPcrAllocationsAndPcrMask (
Tpm2PcrMask = PcdGet32 (PcdTpm2HashMask);
if (Tpm2PcrMask == 0) {
//
- // if PcdTPm2HashMask is zero, use ActivePcr setting
+ // if PcdTpm2HashMask is zero, use ActivePcr setting
//
PcdSet32S (PcdTpm2HashMask, TpmActivePcrBanks);
Tpm2PcrMask = TpmActivePcrBanks;
@@ -297,9 +296,9 @@ SyncPcrAllocationsAndPcrMask (
if ((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) {
NewTpmActivePcrBanks = TpmActivePcrBanks & Tpm2PcrMask;

- DEBUG ((EFI_D_INFO, "%a - Reallocating PCR banks from 0x%X to 0x%X.\n",
__FUNCTION__, TpmActivePcrBanks, NewTpmActivePcrBanks));
+ DEBUG ((DEBUG_INFO, "%a - Reallocating PCR banks from 0x%X to 0x%X.\n",
__FUNCTION__, TpmActivePcrBanks, NewTpmActivePcrBanks));
if (NewTpmActivePcrBanks == 0) {
- DEBUG ((EFI_D_ERROR, "%a - No viable PCRs active! Please set a less
restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
+ DEBUG ((DEBUG_ERROR, "%a - No viable PCRs active! Please set a less
restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
ASSERT (FALSE);
} else {
Status = Tpm2PcrAllocateBanks (NULL, (UINT32)TpmHashAlgorithmBitmap,
NewTpmActivePcrBanks);
@@ -307,7 +306,7 @@ SyncPcrAllocationsAndPcrMask (
//
// We can't do much here, but we hope that this doesn't happen.
//
- DEBUG ((EFI_D_ERROR, "%a - Failed to reallocate PCRs!\n",
__FUNCTION__));
+ DEBUG ((DEBUG_ERROR, "%a - Failed to reallocate PCRs!\n",
__FUNCTION__));
ASSERT_EFI_ERROR (Status);
}
//
@@ -324,9 +323,9 @@ SyncPcrAllocationsAndPcrMask (
if ((Tpm2PcrMask & TpmHashAlgorithmBitmap) != Tpm2PcrMask) {
NewTpm2PcrMask = Tpm2PcrMask & TpmHashAlgorithmBitmap;

- DEBUG ((EFI_D_INFO, "%a - Updating PcdTpm2HashMask from 0x%X to
0x%X.\n", __FUNCTION__, Tpm2PcrMask, NewTpm2PcrMask));
+ DEBUG ((DEBUG_INFO, "%a - Updating PcdTpm2HashMask from 0x%X to
0x%X.\n", __FUNCTION__, Tpm2PcrMask, NewTpm2PcrMask));
if (NewTpm2PcrMask == 0) {
- DEBUG ((EFI_D_ERROR, "%a - No viable PCRs supported! Please set a less
restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
+ DEBUG ((DEBUG_ERROR, "%a - No viable PCRs supported! Please set a less
restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
ASSERT (FALSE);
}

@@ -365,7 +364,7 @@ LogHashEvent (
RetStatus = EFI_SUCCESS;
for (Index = 0; Index < sizeof(mTcg2EventInfo)/sizeof(mTcg2EventInfo[0]);
Index++) {
if ((SupportedEventLogs & mTcg2EventInfo[Index].LogFormat) != 0) {
- DEBUG ((EFI_D_INFO, " LogFormat - 0x%08x\n",
mTcg2EventInfo[Index].LogFormat));
+ DEBUG ((DEBUG_INFO, " LogFormat - 0x%08x\n",
mTcg2EventInfo[Index].LogFormat));
switch (mTcg2EventInfo[Index].LogFormat) {
case EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2:
Status = GetDigestFromDigestList (TPM_ALG_SHA1, DigestList,
&NewEventHdr->Digest);
@@ -476,7 +475,7 @@ HashLogExtendEvent (
}

if (Status == EFI_DEVICE_ERROR) {
- DEBUG ((EFI_D_ERROR, "HashLogExtendEvent - %r. Disable TPM.\n", Status));
+ DEBUG ((DEBUG_ERROR, "HashLogExtendEvent - %r. Disable TPM.\n",
Status));
BuildGuidHob (&gTpmErrorHobGuid,0);
REPORT_STATUS_CODE (
EFI_ERROR_CODE | EFI_ERROR_MINOR,
@@ -1011,7 +1010,7 @@ PeimEntryMA (
}

if (GetFirstGuidHob (&gTpmErrorHobGuid) != NULL) {
- DEBUG ((EFI_D_ERROR, "TPM2 error!\n"));
+ DEBUG ((DEBUG_ERROR, "TPM2 error!\n"));
return EFI_DEVICE_ERROR;
}

@@ -1075,7 +1074,7 @@ PeimEntryMA (
for (PcrIndex = 0; PcrIndex < 8; PcrIndex++) {
Status = MeasureSeparatorEventWithError (PcrIndex);
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "Separator Event with Error not Measured.
Error!\n"));
+ DEBUG ((DEBUG_ERROR, "Separator Event with Error not Measured.
Error!\n"));
}
}
}
@@ -1092,6 +1091,13 @@ PeimEntryMA (
}
}

+ DEBUG_CODE_BEGIN ();
+ //
+ // Peek into TPM PCR 00 before any BIOS measurement.
+ //
+ Tpm2PcrReadForActiveBank (00, NULL);
+ DEBUG_CODE_END ();
+
//
// Only install TpmInitializedPpi on success
//
@@ -1106,7 +1112,7 @@ PeimEntryMA (

Done:
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "TPM2 error! Build Hob\n"));
+ DEBUG ((DEBUG_ERROR, "TPM2 error! Build Hob\n"));
BuildGuidHob (&gTpmErrorHobGuid,0);
REPORT_STATUS_CODE (
EFI_ERROR_CODE | EFI_ERROR_MINOR,
--
2.31.1.windows.1

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>
Sent: Thursday, August 5, 2021 7:28 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>;
Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>
Subject: [PATCH] Reallocate TPM Active PCRs based on platform support.

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

In V2: Add case to RegisterHashInterfaceLib logic

RegisterHashInterfaceLib needs to correctly handle registering the HashLib
instance supported algorithm bitmap when PcdTpm2HashMask is set to zero.

The current implementation of SyncPcrAllocationsAndPcrMask() triggers
PCR bank reallocation only based on the intersection between
TpmActivePcrBanks and PcdTpm2HashMask.

When the software HashLibBaseCryptoRouter solution is used, no PCR bank
reallocation is occurring based on the supported hashing algorithms
registered by the HashLib instances.

Need to have an additional check for the intersection between the
TpmActivePcrBanks and the PcdTcg2HashAlgorithmBitmap populated by the
HashLib instances present on the platform's BIOS.

Signed-off-by: Rodrigo Gonzalez del Cueto
<rodrigo.gonzalez.del.cueto@...>

Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
---
SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c
| 6 +++++-
SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c |
6 +++++-
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 18
+++++++++++++++++-
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 +
4 files changed, 28 insertions(+), 3 deletions(-)

diff --git
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
index 7a0f61efbb..0821159120 100644
---
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
+++
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
@@ -230,13 +230,17 @@ RegisterHashInterfaceLib (
{
UINTN Index;
UINT32 HashMask;
+ UINT32 Tpm2HashMask;
EFI_STATUS Status;

//
// Check allow
//
HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
- if ((HashMask & PcdGet32 (PcdTpm2HashMask)) == 0) {
+ Tpm2HashMask = PcdGet32 (PcdTpm2HashMask);
+
+ if ((Tpm2HashMask != 0) &&
+ ((HashMask & Tpm2HashMask) == 0)) {
return EFI_UNSUPPORTED;
}

diff --git
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
index 42cb562f67..6ae51dbce4 100644
---
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
+++
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
@@ -327,13 +327,17 @@ RegisterHashInterfaceLib (
UINTN Index;
HASH_INTERFACE_HOB *HashInterfaceHob;
UINT32 HashMask;
+ UINT32 Tpm2HashMask;
EFI_STATUS Status;

//
// Check allow
//
HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
- if ((HashMask & PcdGet32 (PcdTpm2HashMask)) == 0) {
+ Tpm2HashMask = PcdGet32 (PcdTpm2HashMask);
+
+ if ((Tpm2HashMask != 0) &&
+ ((HashMask & Tpm2HashMask) == 0)) {
return EFI_UNSUPPORTED;
}

diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index 93a8803ff6..5ad6a45cf3 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
@@ -262,6 +262,7 @@ SyncPcrAllocationsAndPcrMask (
{
EFI_STATUS Status;
EFI_TCG2_EVENT_ALGORITHM_BITMAP TpmHashAlgorithmBitmap;
+ EFI_TCG2_EVENT_ALGORITHM_BITMAP BiosHashAlgorithmBitmap;
UINT32 TpmActivePcrBanks;
UINT32 NewTpmActivePcrBanks;
UINT32 Tpm2PcrMask;
@@ -273,16 +274,27 @@ SyncPcrAllocationsAndPcrMask (
// Determine the current TPM support and the Platform PCR mask.
//
Status = Tpm2GetCapabilitySupportedAndActivePcrs
(&TpmHashAlgorithmBitmap, &TpmActivePcrBanks);
+
ASSERT_EFI_ERROR (Status);
+
+ DEBUG ((EFI_D_INFO, "Tpm2GetCapabilitySupportedAndActivePcrs -
TpmHashAlgorithmBitmap: 0x%08x\n", TpmHashAlgorithmBitmap));
+ DEBUG ((EFI_D_INFO, "Tpm2GetCapabilitySupportedAndActivePcrs -
TpmActivePcrBanks 0x%08x\n", TpmActivePcrBanks));

Tpm2PcrMask = PcdGet32 (PcdTpm2HashMask);
if (Tpm2PcrMask == 0) {
//
// if PcdTPm2HashMask is zero, use ActivePcr setting
//
+ DEBUG ((EFI_D_VERBOSE, "Initializing PcdTpm2HashMask to
TpmActivePcrBanks 0x%08x\n", TpmActivePcrBanks));
PcdSet32S (PcdTpm2HashMask, TpmActivePcrBanks);
+ DEBUG ((EFI_D_VERBOSE, "Initializing Tpm2PcrMask to TpmActivePcrBanks
0x%08x\n", Tpm2PcrMask));
Tpm2PcrMask = TpmActivePcrBanks;
}
+
+ BiosHashAlgorithmBitmap = PcdGet32 (PcdTcg2HashAlgorithmBitmap);
+ DEBUG ((EFI_D_INFO, "PcdTcg2HashAlgorithmBitmap 0x%08x\n",
BiosHashAlgorithmBitmap));
+ DEBUG ((EFI_D_INFO, "Tpm2PcrMask 0x%08x\n", Tpm2PcrMask)); // Active
PCR banks from TPM input
+ DEBUG ((EFI_D_INFO, "TpmActivePcrBanks & BiosHashAlgorithmBitmap =
0x%08x\n", NewTpmActivePcrBanks));

//
// Find the intersection of Pcd support and TPM support.
@@ -294,9 +306,12 @@ SyncPcrAllocationsAndPcrMask (
// If there are active PCR banks that are not supported by the Platform mask,
// update the TPM allocations and reboot the machine.
//
- if ((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) {
+ if (((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) ||
+ ((TpmActivePcrBanks & BiosHashAlgorithmBitmap) != TpmActivePcrBanks)) {
NewTpmActivePcrBanks = TpmActivePcrBanks & Tpm2PcrMask;
+ NewTpmActivePcrBanks &= BiosHashAlgorithmBitmap;

+ DEBUG ((EFI_D_INFO, "NewTpmActivePcrBanks 0x%08x\n",
NewTpmActivePcrBanks));
DEBUG ((EFI_D_INFO, "%a - Reallocating PCR banks from 0x%X to 0x%X.\n",
__FUNCTION__, TpmActivePcrBanks, NewTpmActivePcrBanks));
if (NewTpmActivePcrBanks == 0) {
DEBUG ((EFI_D_ERROR, "%a - No viable PCRs active! Please set a less
restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
@@ -331,6 +346,7 @@ SyncPcrAllocationsAndPcrMask (
}

Status = PcdSet32S (PcdTpm2HashMask, NewTpm2PcrMask);
+ DEBUG ((EFI_D_INFO, "Setting PcdTpm2Hash Mask to 0x%08x\n",
NewTpm2PcrMask));
ASSERT_EFI_ERROR (Status);
}
}
diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
index 06c26a2904..17ad116126 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
@@ -86,6 +86,7 @@
## SOMETIMES_CONSUMES
## SOMETIMES_PRODUCES
gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask
+ gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap ##
CONSUMES

[Depex]
gEfiPeiMasterBootModePpiGuid AND
--
2.31.1.windows.1