Date   

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

Marvin Häuser
 

Good day Hao,

Sorry for the confusion, and you are (rightfully!) not alone. :( I'll quote myself from a different patch:

[...] for some reason, none of the other patch series had indices appended.
I'm sure I can get that fixed shortly, but what to do then, re-send the entire bulk? I don't want to spam the list, maybe it is smarter to group them by some overview mail this one time?

Sorry for the disruption!

Best regards,
Marvin

On 09/08/2021 08:10, Wu, Hao A wrote:
Sorry Marvin Häuser,

Could you help to confirm that below 9 MdeModulePkg related patches are either:
* All independent patches
* Belong to a patch series that includes all these 9 MdeModulePkg related commits
* Belong to several independent patch series

MdePkg/Base.h: Introduce various alignment-related macros
MdeModulePkg/CoreDxe: Mandatory LoadedImage for DebugImageInfoTable
MdeModulePkg/DxeCore: Fix DebugImageInfoTable size report
MdeModulePkg/DxeCore: Use the correct source for fixed load address
MdeModulePkg/PiSmmCore: Drop deprecated image profiling commands
MdeModulePkg/CoreDxe: Drop caller-allocated image buffers
MdeModulePkg/DxeCore: Drop unnecessary pointer indirection
MdeModulePkg/PiSmmIpl: Correct fixed load address bounds check
MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

Best Regards,
Hao Wu

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Bi, Dandan <dandan.bi@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: [PATCH] MdeModulePkg/DxeCore: Consistent
DebugImageInfoTable updates

In theory, modifications to the DebugImageInfoTable may cause exceptions.
If the exception handler parses the table, this can lead to subsequent
exceptions if the table state is inconsistent.

Ensure the DebugImageInfoTable remains consistent during modifications.
This includes:
1) Free the old table only only after the new table has been published.
Mitigates use-after-free of the old table.
2) Do not insert an image entry till it is fully initialised. Entries may be inserted
in the live range if an entry was deleted previously.
Mitigaes the usage of inconsistent entries.
3) Free the old image entry only after the table has been updated with the
NULL value. Mitigates use-after-free of the old entry.
4) Set the MODIFIED state before performing any modifications.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 60 +++++++++++------
---
1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index a75d4158280b..7bd970115111 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -165,10 +165,11 @@ CoreNewDebugImageInfoEntry (
IN EFI_HANDLE ImageHandle ) {- EFI_DEBUG_IMAGE_INFO
*Table;- EFI_DEBUG_IMAGE_INFO *NewTable;- UINTN Index;-
UINTN TableSize;+ EFI_DEBUG_IMAGE_INFO *Table;+
EFI_DEBUG_IMAGE_INFO *NewTable;+ UINTN Index;+
UINTN TableSize;+ EFI_DEBUG_IMAGE_INFO_NORMAL
*NormalImage; // // Set the flag indicating that we're in the process of
updating the table.@@ -203,14 +204,6 @@ CoreNewDebugImageInfoEntry (
// Copy the old table into the new one // CopyMem (NewTable, Table,
TableSize);- //- // Free the old table- //- CoreFreePool (Table);- //-
// Update the table header- //- Table = NewTable;
mDebugInfoTableHeader.EfiDebugImageInfoTable = NewTable; // //
Enlarge the max table entries and set the first empty entry index to@@ -
218,24 +211,34 @@ CoreNewDebugImageInfoEntry (
// Index = mMaxTableEntries; mMaxTableEntries +=
EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;+ //+ // Free the old
table+ //+ CoreFreePool (Table);+ //+ // Update the table header+
//+ Table = NewTable; } // // Allocate data for new entry //-
Table[Index].NormalImage = AllocateZeroPool (sizeof
(EFI_DEBUG_IMAGE_INFO_NORMAL));- if (Table[Index].NormalImage !=
NULL) {+ NormalImage = AllocateZeroPool (sizeof
(EFI_DEBUG_IMAGE_INFO_NORMAL));+ if (NormalImage != NULL) { //
// Update the entry //- Table[Index].NormalImage->ImageInfoType
= (UINT32) ImageInfoType;- Table[Index].NormalImage-
LoadedImageProtocolInstance = LoadedImage;-
Table[Index].NormalImage->ImageHandle = ImageHandle;+
NormalImage->ImageInfoType = (UINT32) ImageInfoType;+
NormalImage->LoadedImageProtocolInstance = LoadedImage;+
NormalImage->ImageHandle = ImageHandle; //- // Increase the
number of EFI_DEBUG_IMAGE_INFO elements and set the
mDebugInfoTable in modified status.+ // Set the mDebugInfoTable in
modified status, insert the entry, and+ // increase the number of
EFI_DEBUG_IMAGE_INFO elements. //-
mDebugInfoTableHeader.TableSize++;
mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;+ Table[Index].NormalImage
= NormalImage;+ mDebugInfoTableHeader.TableSize++; }
mDebugInfoTableHeader.UpdateStatus &=
~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS; }@@ -253,8 +256,9
@@ CoreRemoveDebugImageInfoEntry (
EFI_HANDLE ImageHandle ) {- EFI_DEBUG_IMAGE_INFO *Table;- UINTN
Index;+ EFI_DEBUG_IMAGE_INFO *Table;+ UINTN Index;+
EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;
mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS; @@ -263,16 +267,20
@@ CoreRemoveDebugImageInfoEntry (
for (Index = 0; Index < mMaxTableEntries; Index++) { if
(Table[Index].NormalImage != NULL && Table[Index].NormalImage-
ImageHandle == ImageHandle) { //- // Found a match. Free up the
record, then NULL the pointer to indicate the slot- // is free.+ // Found a
match. Set the mDebugInfoTable in modified status and NULL the+ //
pointer to indicate the slot is free and. //- CoreFreePool
(Table[Index].NormalImage);+ NormalImage =
Table[Index].NormalImage;+ mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED; Table[Index].NormalImage
= NULL; //- // Decrease the number of EFI_DEBUG_IMAGE_INFO
elements and set the mDebugInfoTable in modified status.+ // Decrease
the number of EFI_DEBUG_IMAGE_INFO elements. //
mDebugInfoTableHeader.TableSize--;-
mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;+ //+ // Free up the
record.+ //+ CoreFreePool (NormalImage); break; } }--
2.31.1


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@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Bi, Dandan <dandan.bi@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: [PATCH] MdeModulePkg/DxeCore: Consistent
DebugImageInfoTable updates

In theory, modifications to the DebugImageInfoTable may cause exceptions.
If the exception handler parses the table, this can lead to subsequent
exceptions if the table state is inconsistent.

Ensure the DebugImageInfoTable remains consistent during modifications.
This includes:
1) Free the old table only only after the new table has been published.
Mitigates use-after-free of the old table.
2) Do not insert an image entry till it is fully initialised. Entries may be inserted
in the live range if an entry was deleted previously.
Mitigaes the usage of inconsistent entries.
3) Free the old image entry only after the table has been updated with the
NULL value. Mitigates use-after-free of the old entry.
4) Set the MODIFIED state before performing any modifications.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 60 +++++++++++------
---
1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index a75d4158280b..7bd970115111 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -165,10 +165,11 @@ CoreNewDebugImageInfoEntry (
IN EFI_HANDLE ImageHandle ) {- EFI_DEBUG_IMAGE_INFO
*Table;- EFI_DEBUG_IMAGE_INFO *NewTable;- UINTN Index;-
UINTN TableSize;+ EFI_DEBUG_IMAGE_INFO *Table;+
EFI_DEBUG_IMAGE_INFO *NewTable;+ UINTN Index;+
UINTN TableSize;+ EFI_DEBUG_IMAGE_INFO_NORMAL
*NormalImage; // // Set the flag indicating that we're in the process of
updating the table.@@ -203,14 +204,6 @@ CoreNewDebugImageInfoEntry (
// Copy the old table into the new one // CopyMem (NewTable, Table,
TableSize);- //- // Free the old table- //- CoreFreePool (Table);- //-
// Update the table header- //- Table = NewTable;
mDebugInfoTableHeader.EfiDebugImageInfoTable = NewTable; // //
Enlarge the max table entries and set the first empty entry index to@@ -
218,24 +211,34 @@ CoreNewDebugImageInfoEntry (
// Index = mMaxTableEntries; mMaxTableEntries +=
EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;+ //+ // Free the old
table+ //+ CoreFreePool (Table);+ //+ // Update the table header+
//+ Table = NewTable; } // // Allocate data for new entry //-
Table[Index].NormalImage = AllocateZeroPool (sizeof
(EFI_DEBUG_IMAGE_INFO_NORMAL));- if (Table[Index].NormalImage !=
NULL) {+ NormalImage = AllocateZeroPool (sizeof
(EFI_DEBUG_IMAGE_INFO_NORMAL));+ if (NormalImage != NULL) { //
// Update the entry //- Table[Index].NormalImage->ImageInfoType
= (UINT32) ImageInfoType;- Table[Index].NormalImage-
LoadedImageProtocolInstance = LoadedImage;-
Table[Index].NormalImage->ImageHandle = ImageHandle;+
NormalImage->ImageInfoType = (UINT32) ImageInfoType;+
NormalImage->LoadedImageProtocolInstance = LoadedImage;+
NormalImage->ImageHandle = ImageHandle; //- // Increase the
number of EFI_DEBUG_IMAGE_INFO elements and set the
mDebugInfoTable in modified status.+ // Set the mDebugInfoTable in
modified status, insert the entry, and+ // increase the number of
EFI_DEBUG_IMAGE_INFO elements. //-
mDebugInfoTableHeader.TableSize++;
mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;+ Table[Index].NormalImage
= NormalImage;+ mDebugInfoTableHeader.TableSize++; }
mDebugInfoTableHeader.UpdateStatus &=
~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS; }@@ -253,8 +256,9
@@ CoreRemoveDebugImageInfoEntry (
EFI_HANDLE ImageHandle ) {- EFI_DEBUG_IMAGE_INFO *Table;- UINTN
Index;+ EFI_DEBUG_IMAGE_INFO *Table;+ UINTN Index;+
EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;
mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS; @@ -263,16 +267,20
@@ CoreRemoveDebugImageInfoEntry (
for (Index = 0; Index < mMaxTableEntries; Index++) { if
(Table[Index].NormalImage != NULL && Table[Index].NormalImage-
ImageHandle == ImageHandle) { //- // Found a match. Free up the
record, then NULL the pointer to indicate the slot- // is free.+ // Found a
match. Set the mDebugInfoTable in modified status and NULL the+ //
pointer to indicate the slot is free and. //- CoreFreePool
(Table[Index].NormalImage);+ NormalImage =
Table[Index].NormalImage;+ mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED; Table[Index].NormalImage
= NULL; //- // Decrease the number of EFI_DEBUG_IMAGE_INFO
elements and set the mDebugInfoTable in modified status.+ // Decrease
the number of EFI_DEBUG_IMAGE_INFO elements. //
mDebugInfoTableHeader.TableSize--;-
mDebugInfoTableHeader.UpdateStatus |=
EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;+ //+ // Free up the
record.+ //+ CoreFreePool (NormalImage); break; } }--
2.31.1


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

Marvin Häuser
 

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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>

Signed-off-by: DunTan <dun.tan@intel.com>
---
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@gmail.com>
Sent: Friday, July 30, 2021 10:45 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Caden Kline <cadenkline9@gmail.com>
---
.../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
 

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

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@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
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@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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
 

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


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

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

+Star and Jiewen for confirmation.

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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
 

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@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
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@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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@intel.com>

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

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

+Star and Jiewen for confirmation.

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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
 

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@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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@intel.com>

+Star and Jiewen for confirmation.

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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@intel.com>

Can you kindly share how you found this issue?

Thanks,
Ray

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
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@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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@posteo.de>
Sent: Monday, August 9, 2021 3:40 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
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@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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


[PATCH] MdeModulePkg PCD: FSP NotifyPhase APIs caused 100ms delay

GregX Yeh
 

https://bugzilla.tianocore.org/show_bug.cgi?id=3D3525

After PciSegmentLib using Dynamic PCD for Pcie base address such
long delay found in FSP. The root cause is some of the PCD service
PPIs not shadowed to memory and flash cache may have been disabled
in NotifyPhase stage. Solution is to shadow all PCD service PPIs
to memory.

Signed-off-by: GregX Yeh <gregx.yeh@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
---
MdeModulePkg/Universal/PCD/Pei/Pcd.c | 71 +++++++++++++++++++++++++++-
1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/PCD/Pei/Pcd.c b/MdeModulePkg/Universal/=
PCD/Pei/Pcd.c
index 9c6346924f..f31e0be35f 100644
--- a/MdeModulePkg/Universal/PCD/Pei/Pcd.c
+++ b/MdeModulePkg/Universal/PCD/Pei/Pcd.c
@@ -1,7 +1,7 @@
/** @file=0D
All Pcd Ppi services are implemented here.=0D
=0D
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=0D
(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
@@ -339,6 +339,75 @@ PcdPeimInit (
{=0D
EFI_STATUS Status;=0D
=0D
+ Status =3D PeiServicesRegisterForShadow (FileHandle);=0D
+ if (Status =3D=3D EFI_ALREADY_STARTED) {=0D
+ //=0D
+ // This is now starting in memory, the second time starting.=0D
+ //=0D
+ EFI_PEI_PPI_DESCRIPTOR *OldPpiList;=0D
+ EFI_PEI_PPI_DESCRIPTOR *OldPpiList2;=0D
+ VOID *Ppi;=0D
+ VOID *Ppi2;=0D
+=0D
+ OldPpiList =3D NULL;=0D
+ Status =3D PeiServicesLocatePpi (=0D
+ &gPcdPpiGuid,=0D
+ 0,=0D
+ &OldPpiList,=0D
+ &Ppi=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
+ if (OldPpiList !=3D NULL) {=0D
+ Status =3D PeiServicesReInstallPpi (OldPpiList, &mPpiList[0]);=0D
+ ASSERT_EFI_ERROR (Status);=0D
+ }=0D
+=0D
+ OldPpiList2 =3D NULL;=0D
+ Status =3D PeiServicesLocatePpi (=0D
+ &gGetPcdInfoPpiGuid,=0D
+ 0,=0D
+ &OldPpiList2,=0D
+ &Ppi2=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
+ if (OldPpiList2 !=3D NULL) {=0D
+ Status =3D PeiServicesReInstallPpi (OldPpiList2, &mPpiList2[0]);=0D
+ ASSERT_EFI_ERROR (Status);=0D
+ }=0D
+=0D
+ OldPpiList =3D NULL;=0D
+ Status =3D PeiServicesLocatePpi (=0D
+ &gEfiPeiPcdPpiGuid,=0D
+ 0,=0D
+ &OldPpiList,=0D
+ &Ppi=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
+ if (OldPpiList !=3D NULL) {=0D
+ Status =3D PeiServicesReInstallPpi (OldPpiList, &mPpiList[1]);=0D
+ ASSERT_EFI_ERROR (Status);=0D
+ }=0D
+=0D
+ OldPpiList2 =3D NULL;=0D
+ Status =3D PeiServicesLocatePpi (=0D
+ &gEfiGetPcdInfoPpiGuid,=0D
+ 0,=0D
+ &OldPpiList2,=0D
+ &Ppi2=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
+ if (OldPpiList2 !=3D NULL) {=0D
+ Status =3D PeiServicesReInstallPpi (OldPpiList2, &mPpiList2[1]);=0D
+ ASSERT_EFI_ERROR (Status);=0D
+ }=0D
+=0D
+ return Status;=0D
+ }=0D
+=0D
BuildPcdDatabase (FileHandle);=0D
=0D
//=0D
--=20
2.32.0.windows.1


Re: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Yao, Jiewen
 

Would you please tell us how many TPM2 chip you have tested?

I think we need consider the compatibility of exiting TPM2 chips, to make sure the code still work.


Thank you
Yao Jiewen

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Saturday, July 17, 2021 5:18 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
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@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
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


Re: [PATCH] SecurityPkg: Debug code to audit BIOS TPM extend operations.

Yao, Jiewen
 

Some feedback:

1) I think it is OK to add Tpm2PcrReadForActiveBank() API.
But I feel we will add too many noise to dump Tpm2PcrReadForActiveBank() in the code everytime.
I am not sure why it is needed.
What is the problem statement?

2) Below definition does not follow EDKII coding style. Please use 2 "space" as indent.
EFI_STATUS
EFIAPI
Tpm2PcrReadForActiveBank (
IN TPMI_DH_PCR PcrHandle,
OUT TPML_DIGEST *HashList
)

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Friday, July 30, 2021 6:43 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>; Yao,
Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
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@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
---
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


Re: [PATCH] Reallocate TPM Active PCRs based on platform support.

Yao, Jiewen
 

Hi Rodrigo
I don't understand the problem statement.

This code has been there for long time. What is changed recently ?

Thank you
Yao Jiewen

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Thursday, August 5, 2021 7:28 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
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@intel.com>

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
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


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

Min Xu
 

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@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
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


[PATCH 2/3] efi_gdb.py: - Add gdb EFI commands and pretty Print

Andrew Fish
 

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

Use efi_debugging.py Python Classes to implement EFI gdb commands:
(gdb) help efi
Commands for debugging EFI. efi <cmd>

List of efi subcommands:

efi devicepath -- Display an EFI device path.
efi guid -- Display info about EFI GUID's.
efi hob -- Dump EFI HOBs. Type 'hob -h' for more info.
efi symbols -- Load Symbols for EFI. Type 'efi_symbols -h' for more info.
efi table -- Dump EFI System Tables. Type 'table -h' for more info.

This module is coded against a generic gdb remote serial stub. It should
work with QEMU, JTAG debugger, or a generic EFI gdb remote serial stub.
No modifications of EFI is required to load symbols.

Example usage:
OvmfPkg/build.sh qemu -gdb tcp::9000
gdb -ex "target remote localhost:9000" -ex "source efi_gdb.py"

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Andrew Fish <afish@apple.com>
---
efi_gdb.py | 918 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 918 insertions(+)
create mode 100755 efi_gdb.py

diff --git a/efi_gdb.py b/efi_gdb.py
new file mode 100755
index 000000000000..f3e7fd9d0c28
--- /dev/null
+++ b/efi_gdb.py
@@ -0,0 +1,918 @@
+#!/usr/bin/python3
+'''
+Copyright 2021 (c) Apple Inc. All rights reserved.
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+EFI gdb commands based on efi_debugging classes.
+
+Example usage:
+OvmfPkg/build.sh qemu -gdb tcp::9000
+gdb -ex "target remote localhost:9000" -ex "source efi_gdb.py"
+
+(gdb) help efi
+Commands for debugging EFI. efi <cmd>
+
+List of efi subcommands:
+
+efi devicepath -- Display an EFI device path.
+efi guid -- Display info about EFI GUID's.
+efi hob -- Dump EFI HOBs. Type 'hob -h' for more info.
+efi symbols -- Load Symbols for EFI. Type 'efi_symbols -h' for more info.
+efi table -- Dump EFI System Tables. Type 'table -h' for more info.
+
+This module is coded against a generic gdb remote serial stub. It should work
+with QEMU, JTAG debugger, or a generic EFI gdb remote serial stub.
+
+If you are debugging with QEMU or a JTAG hardware debugger you can insert
+a CpuDeadLoop(); in your code, attach with gdb, and then `p Index=1` to
+step past. If you have a debug stub in EFI you can use CpuBreakpoint();.
+'''
+
+from gdb.printing import RegexpCollectionPrettyPrinter
+from gdb.printing import register_pretty_printer
+import gdb
+import os
+import sys
+import uuid
+import optparse
+import shlex
+
+# gdb will not import from the same path as this script.
+# so lets fix that for gdb...
+sys.path.append(os.path.dirname(os.path.abspath(__file__)))
+
+from efi_debugging import PeTeImage, patch_ctypes # noqa: E402
+from efi_debugging import EfiHob, GuidNames, EfiStatusClass # noqa: E402
+from efi_debugging import EfiBootMode, EfiDevicePath # noqa: E402
+from efi_debugging import EfiConfigurationTable, EfiTpl # noqa: E402
+
+
+class GdbFileObject(object):
+ '''Provide a file like object required by efi_debugging'''
+
+ def __init__(self):
+ self.inferior = gdb.selected_inferior()
+ self.offset = 0
+
+ def tell(self):
+ return self.offset
+
+ def read(self, size=-1):
+ if size == -1:
+ # arbitrary default size
+ size = 0x1000000
+
+ try:
+ data = self.inferior.read_memory(self.offset, size)
+ except MemoryError:
+ data = bytearray(size)
+ assert False
+ if len(data) != size:
+ raise MemoryError(
+ f'gdb could not read memory 0x{size:x}'
+ + f' bytes from 0x{self.offset:08x}')
+ else:
+ # convert memoryview object to a bytestring.
+ return data.tobytes()
+
+ def readable(self):
+ return True
+
+ def seek(self, offset, whence=0):
+ if whence == 0:
+ self.offset = offset
+ elif whence == 1:
+ self.offset += offset
+ else:
+ # whence == 2 is seek from end
+ raise NotImplementedError
+
+ def seekable(self):
+ return True
+
+ def write(self, data):
+ self.inferior.write_memory(self.offset, data)
+ return len(data)
+
+ def writable(self):
+ return True
+
+ def truncate(self, size=None):
+ raise NotImplementedError
+
+ def flush(self):
+ raise NotImplementedError
+
+ def fileno(self):
+ raise NotImplementedError
+
+
+class EfiSymbols:
+ """Class to manage EFI Symbols"""
+
+ loaded = {}
+ stride = None
+ range = None
+ verbose = False
+
+ def __init__(self, file=None):
+ EfiSymbols.file = file if file else GdbFileObject()
+
+ @ classmethod
+ def __str__(cls):
+ return ''.join(f'{value}\n' for value in cls.loaded.values())
+
+ @ classmethod
+ def configure_search(cls, stride, range=None, verbose=False):
+ cls.stride = stride
+ cls.range = range
+ cls.verbose = verbose
+
+ @ classmethod
+ def clear(cls):
+ cls.loaded = {}
+
+ @ classmethod
+ def add_symbols_for_pecoff(cls, pecoff):
+ '''Tell lldb the location of the .text and .data sections.'''
+
+ if pecoff.TextAddress in cls.loaded:
+ return 'Already Loaded: '
+ try:
+ res = 'Loading Symbols Failed:'
+ res = gdb.execute('add-symbol-file ' + pecoff.CodeViewPdb +
+ ' ' + hex(pecoff.TextAddress) +
+ ' -s .data ' + hex(pecoff.DataAddress),
+ False, True)
+
+ cls.loaded[pecoff.TextAddress] = pecoff
+ if cls.verbose:
+ print(f'\n{res:s}\n')
+ return ''
+ except gdb.error:
+ return res
+
+ @ classmethod
+ def address_to_symbols(cls, address, reprobe=False):
+ '''
+ Given an address search backwards for a PE/COFF (or TE) header
+ and load symbols. Return a status string.
+ '''
+ if not isinstance(address, int):
+ address = int(address)
+
+ pecoff = cls.address_in_loaded_pecoff(address)
+ if not reprobe and pecoff is not None:
+ # skip the probe of the remote
+ return f'{pecoff} is already loaded'
+
+ pecoff = PeTeImage(cls.file, None)
+ if pecoff.pcToPeCoff(address, cls.stride, cls.range):
+ res = cls.add_symbols_for_pecoff(pecoff)
+ return f'{res}{pecoff}'
+ else:
+ return f'0x{address:08x} not in a PE/COFF (or TE) image'
+
+ @ classmethod
+ def address_in_loaded_pecoff(cls, address):
+ if not isinstance(address, int):
+ address = int(address)
+
+ for value in cls.loaded.values():
+ if (address >= value.LoadAddress and
+ address <= value.EndLoadAddress):
+ return value
+
+ return None
+
+ @ classmethod
+ def unload_symbols(cls, address):
+ if not isinstance(address, int):
+ address = int(address)
+
+ pecoff = cls.address_in_loaded_pecoff(address)
+ try:
+ res = 'Unloading Symbols Failed:'
+ res = gdb.execute(
+ f'remove-symbol-file -a {hex(pecoff.TextAddress):s}',
+ False, True)
+ del cls.loaded[pecoff.LoadAddress]
+ return res
+ except gdb.error:
+ return res
+
+
+class CHAR16_PrettyPrinter(object):
+
+ def __init__(self, val):
+ self.val = val
+
+ def to_string(self):
+ if int(self.val) < 0x20:
+ return f"L'\\x{int(self.val):02x}'"
+ else:
+ return f"L'{chr(self.val):s}'"
+
+
+class EFI_TPL_PrettyPrinter(object):
+
+ def __init__(self, val):
+ self.val = val
+
+ def to_string(self):
+ return str(EfiTpl(int(self.val)))
+
+
+class EFI_STATUS_PrettyPrinter(object):
+
+ def __init__(self, val):
+ self.val = val
+
+ def to_string(self):
+ status = int(self.val)
+ return f'{str(EfiStatusClass(status)):s} (0x{status:08x})'
+
+
+class EFI_BOOT_MODE_PrettyPrinter(object):
+
+ def __init__(self, val):
+ self.val = val
+
+ def to_string(self):
+ return str(EfiBootMode(int(self.val)))
+
+
+class EFI_GUID_PrettyPrinter(object):
+ """Print 'EFI_GUID' as 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'"""
+
+ def __init__(self, val):
+ self.val = val
+
+ def to_string(self):
+ # if we could get a byte like object of *(unsigned char (*)[16])
+ # then we could just use uuid.UUID() to convert
+ Data1 = int(self.val['Data1'])
+ Data2 = int(self.val['Data2'])
+ Data3 = int(self.val['Data3'])
+ Data4 = self.val['Data4']
+ guid = f'{Data1:08X}-{Data2:04X}-'
+ guid += f'{Data3:04X}-'
+ guid += f'{int(Data4[0]):02X}{int(Data4[1]):02X}-'
+ guid += f'{int(Data4[2]):02X}{int(Data4[3]):02X}'
+ guid += f'{int(Data4[4]):02X}{int(Data4[5]):02X}'
+ guid += f'{int(Data4[6]):02X}{int(Data4[7]):02X}'
+ return str(GuidNames(guid))
+
+
+def build_pretty_printer():
+ # Turn off via: disable pretty-printer global EFI
+ pp = RegexpCollectionPrettyPrinter("EFI")
+ # you can also tell gdb `x/sh <address>` to print CHAR16 string
+ pp.add_printer('CHAR16', '^CHAR16$', CHAR16_PrettyPrinter)
+ pp.add_printer('EFI_BOOT_MODE', '^EFI_BOOT_MODE$',
+ EFI_BOOT_MODE_PrettyPrinter)
+ pp.add_printer('EFI_GUID', '^EFI_GUID$', EFI_GUID_PrettyPrinter)
+ pp.add_printer('EFI_STATUS', '^EFI_STATUS$', EFI_STATUS_PrettyPrinter)
+ pp.add_printer('EFI_TPL', '^EFI_TPL$', EFI_TPL_PrettyPrinter)
+ return pp
+
+
+class EfiDevicePathCmd (gdb.Command):
+ """Display an EFI device path. Type 'efi devicepath -h' for more info"""
+
+ def __init__(self):
+ super(EfiDevicePathCmd, self).__init__(
+ "efi devicepath", gdb.COMMAND_NONE)
+
+ self.file = GdbFileObject()
+
+ def create_options(self, arg, from_tty):
+ usage = "usage: %prog [options] [arg]"
+ description = (
+ "Command that can load EFI PE/COFF and TE image symbols. ")
+
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='efi devicepath',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='hex dump extra data',
+ default=False)
+
+ self.parser.add_option(
+ '-n',
+ '--node',
+ action='store_true',
+ dest='node',
+ help='dump a single device path node',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ return self.parser.parse_args(shlex.split(arg))
+
+ def invoke(self, arg, from_tty):
+ '''gdb command to dump EFI device paths'''
+
+ try:
+ (options, _) = self.create_options(arg, from_tty)
+ if options.help:
+ self.parser.print_help()
+ return
+
+ dev_addr = int(gdb.parse_and_eval(arg))
+ except ValueError:
+ print("Invalid argument!")
+ return
+
+ if options.node:
+ print(EfiDevicePath(
+ self.file).device_path_node_str(dev_addr,
+ options.verbose))
+ else:
+ device_path = EfiDevicePath(self.file, dev_addr, options.verbose)
+ if device_path.valid():
+ print(device_path)
+
+
+class EfiGuidCmd (gdb.Command):
+ """Display info about EFI GUID's. Type 'efi guid -h' for more info"""
+
+ def __init__(self):
+ super(EfiGuidCmd, self).__init__("efi guid",
+ gdb.COMMAND_NONE,
+ gdb.COMPLETE_EXPRESSION)
+ self.file = GdbFileObject()
+
+ def create_options(self, arg, from_tty):
+ usage = "usage: %prog [options] [arg]"
+ description = (
+ "Show EFI_GUID values and the C name of the EFI_GUID variables"
+ "in the C code. If symbols are loaded the Guid.xref file"
+ "can be processed and the complete GUID database can be shown."
+ "This command also suports generating new GUID's, and showing"
+ "the value used to initialize the C variable.")
+
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='efi guid',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-n',
+ '--new',
+ action='store_true',
+ dest='new',
+ help='Generate a new GUID',
+ default=False)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='Also display GUID C structure values',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ return self.parser.parse_args(shlex.split(arg))
+
+ def invoke(self, arg, from_tty):
+ '''gdb command to dump EFI System Tables'''
+
+ try:
+ (options, args) = self.create_options(arg, from_tty)
+ if options.help:
+ self.parser.print_help()
+ return
+ if len(args) >= 1:
+ # guid { 0x414e6bdd, 0xe47b, 0x47cc,
+ # { 0xb2, 0x44, 0xbb, 0x61, 0x02, 0x0c,0xf5, 0x16 }}
+ # this generates multiple args
+ guid = ' '.join(args)
+ except ValueError:
+ print('bad arguments!')
+ return
+
+ if options.new:
+ guid = uuid.uuid4()
+ print(str(guid).upper())
+ print(GuidNames.to_c_guid(guid))
+ return
+
+ if len(args) > 0:
+ if GuidNames.is_guid_str(arg):
+ # guid 05AD34BA-6F02-4214-952E-4DA0398E2BB9
+ key = guid.upper()
+ name = GuidNames.to_name(key)
+ elif GuidNames.is_c_guid(arg):
+ # guid { 0x414e6bdd, 0xe47b, 0x47cc,
+ # { 0xb2, 0x44, 0xbb, 0x61, 0x02, 0x0c,0xf5, 0x16 }}
+ key = GuidNames.from_c_guid(arg)
+ name = GuidNames.to_name(key)
+ else:
+ # guid gEfiDxeServicesTableGuid
+ name = guid
+ try:
+ key = GuidNames.to_guid(name)
+ name = GuidNames.to_name(key)
+ except ValueError:
+ return
+
+ extra = f'{GuidNames.to_c_guid(key)}: ' if options.verbose else ''
+ print(f'{key}: {extra}{name}')
+
+ else:
+ for key, value in GuidNames._dict_.items():
+ if options.verbose:
+ extra = f'{GuidNames.to_c_guid(key)}: '
+ else:
+ extra = ''
+ print(f'{key}: {extra}{value}')
+
+
+class EfiHobCmd (gdb.Command):
+ """Dump EFI HOBs. Type 'hob -h' for more info."""
+
+ def __init__(self):
+ super(EfiHobCmd, self).__init__("efi hob", gdb.COMMAND_NONE)
+ self.file = GdbFileObject()
+
+ def create_options(self, arg, from_tty):
+ usage = "usage: %prog [options] [arg]"
+ description = (
+ "Command that can load EFI PE/COFF and TE image symbols. ")
+
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='efi hob',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-a',
+ '--address',
+ type="int",
+ dest='address',
+ help='Parse HOBs from address',
+ default=None)
+
+ self.parser.add_option(
+ '-t',
+ '--type',
+ type="int",
+ dest='type',
+ help='Only dump HOBS of his type',
+ default=None)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='hex dump extra data',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ return self.parser.parse_args(shlex.split(arg))
+
+ def invoke(self, arg, from_tty):
+ '''gdb command to dump EFI System Tables'''
+
+ try:
+ (options, _) = self.create_options(arg, from_tty)
+ if options.help:
+ self.parser.print_help()
+ return
+ except ValueError:
+ print('bad arguments!')
+ return
+
+ if options.address:
+ try:
+ value = gdb.parse_and_eval(options.address)
+ address = int(value)
+ except ValueError:
+ address = None
+ else:
+ address = None
+
+ hob = EfiHob(self.file,
+ address,
+ options.verbose).get_hob_by_type(options.type)
+ print(hob)
+
+
+class EfiTablesCmd (gdb.Command):
+ """Dump EFI System Tables. Type 'table -h' for more info."""
+
+ def __init__(self):
+ super(EfiTablesCmd, self).__init__("efi table", gdb.COMMAND_NONE)
+
+ self.file = GdbFileObject()
+
+ def create_options(self, arg, from_tty):
+ usage = "usage: %prog [options] [arg]"
+ description = "Dump EFI System Tables. Requires symbols to be loaded"
+
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='efi table',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ return self.parser.parse_args(shlex.split(arg))
+
+ def invoke(self, arg, from_tty):
+ '''gdb command to dump EFI System Tables'''
+
+ try:
+ (options, _) = self.create_options(arg, from_tty)
+ if options.help:
+ self.parser.print_help()
+ return
+ except ValueError:
+ print('bad arguments!')
+ return
+
+ gST = gdb.lookup_global_symbol('gST')
+ if gST is None:
+ print('Error: This command requires symbols for gST to be loaded')
+ return
+
+ table = EfiConfigurationTable(
+ self.file, int(gST.value(gdb.selected_frame())))
+ if table:
+ print(table, '\n')
+
+
+class EfiSymbolsCmd (gdb.Command):
+ """Load Symbols for EFI. Type 'efi symbols -h' for more info."""
+
+ def __init__(self):
+ super(EfiSymbolsCmd, self).__init__("efi symbols",
+ gdb.COMMAND_NONE,
+ gdb.COMPLETE_EXPRESSION)
+ self.file = GdbFileObject()
+ self.gST = None
+ self.efi_symbols = EfiSymbols(self.file)
+
+ def create_options(self, arg, from_tty):
+ usage = "usage: %prog [options]"
+ description = (
+ "Command that can load EFI PE/COFF and TE image symbols. "
+ "If you are having trouble in PEI try adding --pei. "
+ "Given any address search backward for the PE/COFF (or TE header) "
+ "and then parse the PE/COFF image to get debug info. "
+ "The address can come from the current pc, pc values in the "
+ "frame, or an address provided to the command"
+ "")
+
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='efi symbols',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-a',
+ '--address',
+ type="str",
+ dest='address',
+ help='Load symbols for image that contains address',
+ default=None)
+
+ self.parser.add_option(
+ '-c',
+ '--clear',
+ action='store_true',
+ dest='clear',
+ help='Clear the cache of loaded images',
+ default=False)
+
+ self.parser.add_option(
+ '-f',
+ '--frame',
+ action='store_true',
+ dest='frame',
+ help='Load symbols for current stack frame',
+ default=False)
+
+ self.parser.add_option(
+ '-p',
+ '--pc',
+ action='store_true',
+ dest='pc',
+ help='Load symbols for pc',
+ default=False)
+
+ self.parser.add_option(
+ '--pei',
+ action='store_true',
+ dest='pei',
+ help='Load symbols for PEI (searches every 4 bytes)',
+ default=False)
+
+ self.parser.add_option(
+ '-e',
+ '--extended',
+ action='store_true',
+ dest='extended',
+ help='Try to load all symbols based on config tables',
+ default=False)
+
+ self.parser.add_option(
+ '-r',
+ '--range',
+ type="long",
+ dest='range',
+ help='How far to search backward for start of PE/COFF Image',
+ default=None)
+
+ self.parser.add_option(
+ '-s',
+ '--stride',
+ type="long",
+ dest='stride',
+ help='Boundary to search for PE/COFF header',
+ default=None)
+
+ self.parser.add_option(
+ '-t',
+ '--thread',
+ action='store_true',
+ dest='thread',
+ help='Load symbols for the frames of all threads',
+ default=False)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='Show more info on symbols loading in gdb',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ return self.parser.parse_args(shlex.split(arg))
+
+ def save_user_state(self):
+ self.pagination = gdb.parameter("pagination")
+ if self.pagination:
+ gdb.execute("set pagination off")
+
+ self.user_selected_thread = gdb.selected_thread()
+ self.user_selected_frame = gdb.selected_frame()
+
+ def restore_user_state(self):
+ self.user_selected_thread.switch()
+ self.user_selected_frame.select()
+
+ if self.pagination:
+ gdb.execute("set pagination on")
+
+ def canonical_address(self, address):
+ '''
+ Scrub out 48-bit non canonical addresses
+ Raw frames in gdb can have some funky values
+ '''
+
+ # Skip lowest 256 bytes to avoid interrupt frames
+ if address > 0xFF and address < 0x00007FFFFFFFFFFF:
+ return True
+ if address >= 0xFFFF800000000000:
+ return True
+
+ return False
+
+ def pc_set_for_frames(self):
+ '''Return a set for the PC's in the current frame'''
+ pc_list = []
+ frame = gdb.newest_frame()
+ while frame:
+ pc = int(frame.read_register('pc'))
+ if self.canonical_address(pc):
+ pc_list.append(pc)
+ frame = frame.older()
+
+ return set(pc_list)
+
+ def invoke(self, arg, from_tty):
+ '''gdb command to symbolicate all the frames from all the threads'''
+
+ try:
+ (options, _) = self.create_options(arg, from_tty)
+ if options.help:
+ self.parser.print_help()
+ return
+ except ValueError:
+ print('bad arguments!')
+ return
+
+ self.dont_repeat()
+
+ self.save_user_state()
+
+ if options.clear:
+ self.efi_symbols.clear()
+ return
+
+ if options.pei:
+ # XIP code can be 4 byte aligned in the FV
+ options.stride = 4
+ options.range = 0x100000
+ self.efi_symbols.configure_search(options.stride,
+ options.range,
+ options.verbose)
+
+ if options.thread:
+ thread_list = gdb.selected_inferior().threads()
+ else:
+ thread_list = (gdb.selected_thread(),)
+
+ address = None
+ if options.address:
+ value = gdb.parse_and_eval(options.address)
+ address = int(value)
+ elif options.pc:
+ address = gdb.selected_frame().pc()
+
+ if address:
+ res = self.efi_symbols.address_to_symbols(address)
+ print(res)
+ else:
+
+ for thread in thread_list:
+ thread.switch()
+
+ # You can not iterate over frames as you load symbols. Loading
+ # symbols changes the frames gdb can see due to inlining and
+ # boom. So we loop adding symbols for the current frame, and
+ # we test to see if new frames have shown up. If new frames
+ # show up we process those new frames. Thus 1st pass is the
+ # raw frame, and other passes are only new PC values.
+ NewPcSet = self.pc_set_for_frames()
+ while NewPcSet:
+ PcSet = self.pc_set_for_frames()
+ for pc in NewPcSet:
+ res = self.efi_symbols.address_to_symbols(pc)
+ print(res)
+
+ NewPcSet = PcSet.symmetric_difference(
+ self.pc_set_for_frames())
+
+ # find the EFI System tables the 1st time
+ if self.gST is None:
+ gST = gdb.lookup_global_symbol('gST')
+ if gST is not None:
+ self.gST = int(gST.value(gdb.selected_frame()))
+ table = EfiConfigurationTable(self.file, self.gST)
+ else:
+ table = None
+ else:
+ table = EfiConfigurationTable(self.file, self.gST)
+
+ if options.extended and table:
+ # load symbols from EFI System Table entry
+ for address, _ in table.DebugImageInfo():
+ res = self.efi_symbols.address_to_symbols(address)
+ print(res)
+
+ # sync up the GUID database from the build output
+ for m in gdb.objfiles():
+ if GuidNames.add_build_guid_file(str(m.filename)):
+ break
+
+ self.restore_user_state()
+
+
+class EfiCmd (gdb.Command):
+ """Commands for debugging EFI. efi <cmd>"""
+
+ def __init__(self):
+ super(EfiCmd, self).__init__("efi",
+ gdb.COMMAND_NONE,
+ gdb.COMPLETE_NONE,
+ True)
+
+ def invoke(self, arg, from_tty):
+ '''default to loading symbols'''
+ if '-h' in arg or '--help' in arg:
+ gdb.execute('help efi')
+ else:
+ # default to loading all symbols
+ gdb.execute('efi symbols --extended')
+
+
+class LoadEmulatorEfiSymbols(gdb.Breakpoint):
+ '''
+ breakpoint for EmulatorPkg to load symbols
+ Note: make sure SecGdbScriptBreak is not optimized away!
+ Also turn off the dlopen() flow like on macOS.
+ '''
+ def stop(self):
+ symbols = EfiSymbols()
+ # Emulator adds SizeOfHeaders so we need file alignment to search
+ symbols.configure_search(0x20)
+
+ frame = gdb.newest_frame()
+
+ try:
+ # gdb was looking at spill address, pre spill :(
+ LoadAddress = frame.read_register('rdx')
+ AddSymbolFlag = frame.read_register('rcx')
+ except gdb.error:
+ LoadAddress = frame.read_var('LoadAddress')
+ AddSymbolFlag = frame.read_var('AddSymbolFlag')
+
+ if AddSymbolFlag == 1:
+ res = symbols.address_to_symbols(LoadAddress)
+ else:
+ res = symbols.unload_symbols(LoadAddress)
+ print(res)
+
+ # keep running
+ return False
+
+
+# Get python backtraces to debug errors in this script
+gdb.execute("set python print-stack full")
+
+# tell efi_debugging how to walk data structures with pointers
+try:
+ pointer_width = gdb.lookup_type('int').pointer().sizeof
+except ValueError:
+ pointer_width = 8
+patch_ctypes(pointer_width)
+
+register_pretty_printer(None, build_pretty_printer(), replace=True)
+
+# gdb commands that we are adding
+# add `efi` prefix gdb command
+EfiCmd()
+
+# subcommands for `efi`
+EfiSymbolsCmd()
+EfiTablesCmd()
+EfiHobCmd()
+EfiDevicePathCmd()
+EfiGuidCmd()
+
+#
+bp = LoadEmulatorEfiSymbols('SecGdbScriptBreak', internal=True)
+if bp.pending:
+ try:
+ gdb.selected_frame()
+ # Not the emulator so do this when you attach
+ gdb.execute('efi symbols --frame --extended', True)
+ gdb.execute('bt')
+ # If you want to skip the above commands comment them out
+ pass
+ except gdb.error:
+ # If you load the script and there is no target ignore the error.
+ pass
+else:
+ # start the emulator
+ gdb.execute('run')
--
2.30.1 (Apple Git-130)


[PATCH 3/3] efi_lldb.py: - Add lldb EFI commands and pretty Print

Andrew Fish
 

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

Use efi_debugging.py Python Classes to implement EFI gdb commands:
efi_symbols, guid, table, hob, and devicepath

You can attach to any standard gdb or kdp remote server and get EFI
symbols. No modifications of EFI are required.

Example usage:
OvmfPkg/build.sh qemu -gdb tcp::9000
lldb -o "gdb-remote localhost:9000" -o "command script import efi_lldb.py"
Note you may also have to teach lldb about QEMU:
-o "settings set plugin.process.gdb-remote.target-definition-file
x86_64_target_definition.py"

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Andrew Fish <afish@apple.com>
---
efi_lldb.py | 1044 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 1044 insertions(+)
create mode 100755 efi_lldb.py

diff --git a/efi_lldb.py b/efi_lldb.py
new file mode 100755
index 000000000000..6487e41bf50c
--- /dev/null
+++ b/efi_lldb.py
@@ -0,0 +1,1044 @@
+#!/usr/bin/python3
+'''
+Copyright (c) Apple Inc. 2021
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+Example usage:
+OvmfPkg/build.sh qemu -gdb tcp::9000
+lldb -o "gdb-remote localhost:9000" -o "command script import efi_lldb.py"
+'''
+
+import optparse
+import shlex
+import subprocess
+import uuid
+import sys
+import os
+from pathlib import Path
+from efi_debugging import EfiDevicePath, EfiConfigurationTable, EfiTpl
+from efi_debugging import EfiHob, GuidNames, EfiStatusClass, EfiBootMode
+from efi_debugging import PeTeImage, patch_ctypes
+
+try:
+ # Just try for LLDB in case PYTHONPATH is already correctly setup
+ import lldb
+except ImportError:
+ try:
+ env = os.environ.copy()
+ env['LLDB_DEFAULT_PYTHON_VERSION'] = str(sys.version_info.major)
+ lldb_python_path = subprocess.check_output(
+ ["xcrun", "lldb", "-P"], env=env).decode("utf-8").strip()
+ sys.path.append(lldb_python_path)
+ import lldb
+ except ValueError:
+ print("Couldn't find LLDB.framework from lldb -P")
+ print("PYTHONPATH should match the currently selected lldb")
+ sys.exit(-1)
+
+
+class LldbFileObject(object):
+ '''
+ Class that fakes out file object to abstract lldb from the generic code.
+ For lldb this is memory so we don't have a concept of the end of the file.
+ '''
+
+ def __init__(self, process):
+ # _exe_ctx is lldb.SBExecutionContext
+ self._process = process
+ self._offset = 0
+ self._SBError = lldb.SBError()
+
+ def tell(self):
+ return self._offset
+
+ def read(self, size=-1):
+ if size == -1:
+ # arbitrary default size
+ size = 0x1000000
+
+ data = self._process.ReadMemory(self._offset, size, self._SBError)
+ if self._SBError.fail:
+ raise MemoryError(
+ f'lldb could not read memory 0x{size:x} '
+ f' bytes from 0x{self._offset:08x}')
+ else:
+ return data
+
+ def readable(self):
+ return True
+
+ def seek(self, offset, whence=0):
+ if whence == 0:
+ self._offset = offset
+ elif whence == 1:
+ self._offset += offset
+ else:
+ # whence == 2 is seek from end
+ raise NotImplementedError
+
+ def seekable(self):
+ return True
+
+ def write(self, data):
+ result = self._process.WriteMemory(self._offset, data, self._SBError)
+ if self._SBError.fail:
+ raise MemoryError(
+ f'lldb could not write memory to 0x{self._offset:08x}')
+ return result
+
+ def writable(self):
+ return True
+
+ def truncate(self, size=None):
+ raise NotImplementedError
+
+ def flush(self):
+ raise NotImplementedError
+
+ def fileno(self):
+ raise NotImplementedError
+
+
+class EfiSymbols:
+ """
+ Class to manage EFI Symbols
+ You need to pass file, and exe_ctx to load symbols.
+ You can print(EfiSymbols()) to see the currently loaded symbols
+ """
+
+ loaded = {}
+ stride = None
+ range = None
+ verbose = False
+
+ def __init__(self, target=None):
+ if target:
+ EfiSymbols.target = target
+ EfiSymbols._file = LldbFileObject(target.process)
+
+ @ classmethod
+ def __str__(cls):
+ return ''.join(f'{pecoff}\n' for (pecoff, _) in cls.loaded.values())
+
+ @ classmethod
+ def configure_search(cls, stride, range, verbose=False):
+ cls.stride = stride
+ cls.range = range
+ cls.verbose = verbose
+
+ @ classmethod
+ def clear(cls):
+ cls.loaded = {}
+
+ @ classmethod
+ def add_symbols_for_pecoff(cls, pecoff):
+ '''Tell lldb the location of the .text and .data sections.'''
+
+ if pecoff.LoadAddress in cls.loaded:
+ return 'Already Loaded: '
+
+ module = cls.target.AddModule(None, None, str(pecoff.CodeViewUuid))
+ if not module:
+ module = cls.target.AddModule(pecoff.CodeViewPdb,
+ None,
+ str(pecoff.CodeViewUuid))
+ if module.IsValid():
+ SBError = cls.target.SetModuleLoadAddress(
+ module, pecoff.LoadAddress + pecoff.TeAdjust)
+ if SBError.success:
+ cls.loaded[pecoff.LoadAddress] = (pecoff, module)
+ return ''
+
+ return 'Symbols NOT FOUND: '
+
+ @ classmethod
+ def address_to_symbols(cls, address, reprobe=False):
+ '''
+ Given an address search backwards for a PE/COFF (or TE) header
+ and load symbols. Return a status string.
+ '''
+ if not isinstance(address, int):
+ address = int(address)
+
+ pecoff, _ = cls.address_in_loaded_pecoff(address)
+ if not reprobe and pecoff is not None:
+ # skip the probe of the remote
+ return f'{pecoff} is already loaded'
+
+ pecoff = PeTeImage(cls._file, None)
+ if pecoff.pcToPeCoff(address, cls.stride, cls.range):
+ res = cls.add_symbols_for_pecoff(pecoff)
+ return f'{res}{pecoff}'
+ else:
+ return f'0x{address:08x} not in a PE/COFF (or TE) image'
+
+ @ classmethod
+ def address_in_loaded_pecoff(cls, address):
+ if not isinstance(address, int):
+ address = int(address)
+
+ for (pecoff, module) in cls.loaded.values():
+ if (address >= pecoff.LoadAddress and
+ address <= pecoff.EndLoadAddress):
+
+ return pecoff, module
+
+ return None, None
+
+ @ classmethod
+ def unload_symbols(cls, address):
+ pecoff, module = cls.address_in_loaded_pecoff(address)
+ if module:
+ name = str(module)
+ cls.target.ClearModuleLoadAddress(module)
+ cls.target.RemoveModule(module)
+ del cls.loaded[pecoff.LoadAddress]
+ return f'{name:s} was unloaded'
+ return f'0x{address:x} was not in a loaded image'
+
+
+def arg_to_address(frame, arg):
+ ''' convert an lldb command arg into a memory address (addr_t)'''
+
+ if arg is None:
+ return None
+
+ arg_str = arg if isinstance(arg, str) else str(arg)
+ SBValue = frame.EvaluateExpression(arg_str)
+ if SBValue.error.fail:
+ return arg
+
+ if (SBValue.TypeIsPointerType() or
+ SBValue.value_type == lldb.eValueTypeRegister or
+ SBValue.value_type == lldb.eValueTypeRegisterSet or
+ SBValue.value_type == lldb.eValueTypeConstResult):
+ try:
+ addr = SBValue.GetValueAsAddress()
+ except ValueError:
+ addr = SBValue.unsigned
+ else:
+ try:
+ addr = SBValue.address_of.GetValueAsAddress()
+ except ValueError:
+ addr = SBValue.address_of.unsigned
+
+ return addr
+
+
+def arg_to_data(frame, arg):
+ ''' convert an lldb command arg into a data vale (uint32_t/uint64_t)'''
+ if not isinstance(arg, str):
+ arg_str = str(str)
+
+ SBValue = frame.EvaluateExpression(arg_str)
+ return SBValue.unsigned
+
+
+class EfiDevicePathCommand:
+
+ def create_options(self):
+ ''' standard lldb command help/options parser'''
+ usage = "usage: %prog [options]"
+ description = '''Command that can EFI Config Tables
+'''
+
+ # Pass add_help_option = False, since this keeps the command in line
+ # with lldb commands, and we wire up "help command" to work by
+ # providing the long & short help methods below.
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='devicepath',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='hex dump extra data',
+ default=False)
+
+ self.parser.add_option(
+ '-n',
+ '--node',
+ action='store_true',
+ dest='node',
+ help='dump a single device path node',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ def get_short_help(self):
+ '''standard lldb function method'''
+ return "Display EFI Tables"
+
+ def get_long_help(self):
+ '''standard lldb function method'''
+ return self.help_string
+
+ def __init__(self, debugger, internal_dict):
+ '''standard lldb function method'''
+ self.create_options()
+ self.help_string = self.parser.format_help()
+
+ def __call__(self, debugger, command, exe_ctx, result):
+ '''standard lldb function method'''
+ # Use the Shell Lexer to properly parse up command options just like a
+ # shell would
+ command_args = shlex.split(command)
+
+ try:
+ (options, args) = self.parser.parse_args(command_args)
+ dev_list = []
+ for arg in args:
+ dev_list.append(arg_to_address(exe_ctx.frame, arg))
+ except ValueError:
+ # if you don't handle exceptions, passing an incorrect argument
+ # to the OptionParser will cause LLDB to exit (courtesy of
+ # OptParse dealing with argument errors by throwing SystemExit)
+ result.SetError("option parsing failed")
+ return
+
+ if options.help:
+ self.parser.print_help()
+ return
+
+ file = LldbFileObject(exe_ctx.process)
+
+ for dev_addr in dev_list:
+ if options.node:
+ print(EfiDevicePath(file).device_path_node_str(
+ dev_addr, options.verbose))
+ else:
+ device_path = EfiDevicePath(file, dev_addr, options.verbose)
+ if device_path.valid():
+ print(device_path)
+
+
+class EfiHobCommand:
+ def create_options(self):
+ ''' standard lldb command help/options parser'''
+ usage = "usage: %prog [options]"
+ description = '''Command that can EFI dump EFI HOBs'''
+
+ # Pass add_help_option = False, since this keeps the command in line
+ # with lldb commands, and we wire up "help command" to work by
+ # providing the long & short help methods below.
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='table',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-a',
+ '--address',
+ type="int",
+ dest='address',
+ help='Parse HOBs from address',
+ default=None)
+
+ self.parser.add_option(
+ '-t',
+ '--type',
+ type="int",
+ dest='type',
+ help='Only dump HOBS of his type',
+ default=None)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='hex dump extra data',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ def get_short_help(self):
+ '''standard lldb function method'''
+ return "Display EFI Hobs"
+
+ def get_long_help(self):
+ '''standard lldb function method'''
+ return self.help_string
+
+ def __init__(self, debugger, internal_dict):
+ '''standard lldb function method'''
+ self.create_options()
+ self.help_string = self.parser.format_help()
+
+ def __call__(self, debugger, command, exe_ctx, result):
+ '''standard lldb function method'''
+ # Use the Shell Lexer to properly parse up command options just like a
+ # shell would
+ command_args = shlex.split(command)
+
+ try:
+ (options, _) = self.parser.parse_args(command_args)
+ except ValueError:
+ # if you don't handle exceptions, passing an incorrect argument
+ # to the OptionParser will cause LLDB to exit (courtesy of
+ # OptParse dealing with argument errors by throwing SystemExit)
+ result.SetError("option parsing failed")
+ return
+
+ if options.help:
+ self.parser.print_help()
+ return
+
+ address = arg_to_address(exe_ctx.frame, options.address)
+
+ file = LldbFileObject(exe_ctx.process)
+ hob = EfiHob(file, address, options.verbose).get_hob_by_type(
+ options.type)
+ print(hob)
+
+
+class EfiTableCommand:
+
+ def create_options(self):
+ ''' standard lldb command help/options parser'''
+ usage = "usage: %prog [options]"
+ description = '''Command that can display EFI Config Tables
+'''
+
+ # Pass add_help_option = False, since this keeps the command in line
+ # with lldb commands, and we wire up "help command" to work by
+ # providing the long & short help methods below.
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='table',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ def get_short_help(self):
+ '''standard lldb function method'''
+ return "Display EFI Tables"
+
+ def get_long_help(self):
+ '''standard lldb function method'''
+ return self.help_string
+
+ def __init__(self, debugger, internal_dict):
+ '''standard lldb function method'''
+ self.create_options()
+ self.help_string = self.parser.format_help()
+
+ def __call__(self, debugger, command, exe_ctx, result):
+ '''standard lldb function method'''
+ # Use the Shell Lexer to properly parse up command options just like a
+ # shell would
+ command_args = shlex.split(command)
+
+ try:
+ (options, _) = self.parser.parse_args(command_args)
+ except ValueError:
+ # if you don't handle exceptions, passing an incorrect argument
+ # to the OptionParser will cause LLDB to exit (courtesy of
+ # OptParse dealing with argument errors by throwing SystemExit)
+ result.SetError("option parsing failed")
+ return
+
+ if options.help:
+ self.parser.print_help()
+ return
+
+ gST = exe_ctx.target.FindFirstGlobalVariable('gST')
+ if gST.error.fail:
+ print('Error: This command requires symbols for gST to be loaded')
+ return
+
+ file = LldbFileObject(exe_ctx.process)
+ table = EfiConfigurationTable(file, gST.unsigned)
+ if table:
+ print(table, '\n')
+
+
+class EfiGuidCommand:
+
+ def create_options(self):
+ ''' standard lldb command help/options parser'''
+ usage = "usage: %prog [options]"
+ description = '''
+ Command that can display all EFI GUID's or give info on a
+ specific GUID's
+ '''
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='guid',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-n',
+ '--new',
+ action='store_true',
+ dest='new',
+ help='Generate a new GUID',
+ default=False)
+
+ self.parser.add_option(
+ '-v',
+ '--verbose',
+ action='store_true',
+ dest='verbose',
+ help='Also display GUID C structure values',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ def get_short_help(self):
+ '''standard lldb function method'''
+ return "Display EFI GUID's"
+
+ def get_long_help(self):
+ '''standard lldb function method'''
+ return self.help_string
+
+ def __init__(self, debugger, internal_dict):
+ '''standard lldb function method'''
+ self.create_options()
+ self.help_string = self.parser.format_help()
+
+ def __call__(self, debugger, command, exe_ctx, result):
+ '''standard lldb function method'''
+ # Use the Shell Lexer to properly parse up command options just like a
+ # shell would
+ command_args = shlex.split(command)
+
+ try:
+ (options, args) = self.parser.parse_args(command_args)
+ if len(args) >= 1:
+ # guid { 0x414e6bdd, 0xe47b, 0x47cc,
+ # { 0xb2, 0x44, 0xbb, 0x61, 0x02, 0x0c,0xf5, 0x16 }}
+ # this generates multiple args
+ arg = ' '.join(args)
+ except ValueError:
+ # if you don't handle exceptions, passing an incorrect argument
+ # to the OptionParser will cause LLDB to exit (courtesy of
+ # OptParse dealing with argument errors by throwing SystemExit)
+ result.SetError("option parsing failed")
+ return
+
+ if options.help:
+ self.parser.print_help()
+ return
+
+ if options.new:
+ guid = uuid.uuid4()
+ print(str(guid).upper())
+ print(GuidNames.to_c_guid(guid))
+ return
+
+ if len(args) > 0:
+ if GuidNames.is_guid_str(arg):
+ # guid 05AD34BA-6F02-4214-952E-4DA0398E2BB9
+ key = arg.lower()
+ name = GuidNames.to_name(key)
+ elif GuidNames.is_c_guid(arg):
+ # guid { 0x414e6bdd, 0xe47b, 0x47cc,
+ # { 0xb2, 0x44, 0xbb, 0x61, 0x02, 0x0c,0xf5, 0x16 }}
+ key = GuidNames.from_c_guid(arg)
+ name = GuidNames.to_name(key)
+ else:
+ # guid gEfiDxeServicesTableGuid
+ name = arg
+ try:
+ key = GuidNames.to_guid(name)
+ name = GuidNames.to_name(key)
+ except ValueError:
+ return
+
+ extra = f'{GuidNames.to_c_guid(key)}: ' if options.verbose else ''
+ print(f'{key}: {extra}{name}')
+
+ else:
+ for key, value in GuidNames._dict_.items():
+ if options.verbose:
+ extra = f'{GuidNames.to_c_guid(key)}: '
+ else:
+ extra = ''
+ print(f'{key}: {extra}{value}')
+
+
+class EfiSymbolicateCommand(object):
+ '''Class to abstract an lldb command'''
+
+ def create_options(self):
+ ''' standard lldb command help/options parser'''
+ usage = "usage: %prog [options]"
+ description = '''Command that can load EFI PE/COFF and TE image
+ symbols. If you are having trouble in PEI try adding --pei.
+ '''
+
+ # Pass add_help_option = False, since this keeps the command in line
+ # with lldb commands, and we wire up "help command" to work by
+ # providing the long & short help methods below.
+ self.parser = optparse.OptionParser(
+ description=description,
+ prog='efi_symbols',
+ usage=usage,
+ add_help_option=False)
+
+ self.parser.add_option(
+ '-a',
+ '--address',
+ type="int",
+ dest='address',
+ help='Load symbols for image at address',
+ default=None)
+
+ self.parser.add_option(
+ '-f',
+ '--frame',
+ action='store_true',
+ dest='frame',
+ help='Load symbols for current stack frame',
+ default=False)
+
+ self.parser.add_option(
+ '-p',
+ '--pc',
+ action='store_true',
+ dest='pc',
+ help='Load symbols for pc',
+ default=False)
+
+ self.parser.add_option(
+ '--pei',
+ action='store_true',
+ dest='pei',
+ help='Load symbols for PEI (searches every 4 bytes)',
+ default=False)
+
+ self.parser.add_option(
+ '-e',
+ '--extended',
+ action='store_true',
+ dest='extended',
+ help='Try to load all symbols based on config tables.',
+ default=False)
+
+ self.parser.add_option(
+ '-r',
+ '--range',
+ type="long",
+ dest='range',
+ help='How far to search backward for start of PE/COFF Image',
+ default=None)
+
+ self.parser.add_option(
+ '-s',
+ '--stride',
+ type="long",
+ dest='stride',
+ help='Boundary to search for PE/COFF header',
+ default=None)
+
+ self.parser.add_option(
+ '-t',
+ '--thread',
+ action='store_true',
+ dest='thread',
+ help='Load symbols for the frames of all threads',
+ default=False)
+
+ self.parser.add_option(
+ '-h',
+ '--help',
+ action='store_true',
+ dest='help',
+ help='Show help for the command',
+ default=False)
+
+ def get_short_help(self):
+ '''standard lldb function method'''
+ return (
+ "Load symbols based on an address that is part of"
+ " a PE/COFF EFI image.")
+
+ def get_long_help(self):
+ '''standard lldb function method'''
+ return self.help_string
+
+ def __init__(self, debugger, unused):
+ '''standard lldb function method'''
+ self.create_options()
+ self.help_string = self.parser.format_help()
+
+ def lldb_print(self, lldb_str):
+ # capture command out like an lldb command
+ self.result.PutCString(lldb_str)
+ # flush the output right away
+ self.result.SetImmediateOutputFile(
+ self.exe_ctx.target.debugger.GetOutputFile())
+
+ def __call__(self, debugger, command, exe_ctx, result):
+ '''standard lldb function method'''
+ # Use the Shell Lexer to properly parse up command options just like a
+ # shell would
+ command_args = shlex.split(command)
+
+ try:
+ (options, _) = self.parser.parse_args(command_args)
+ except ValueError:
+ # if you don't handle exceptions, passing an incorrect argument
+ # to the OptionParser will cause LLDB to exit (courtesy of
+ # OptParse dealing with argument errors by throwing SystemExit)
+ result.SetError("option parsing failed")
+ return
+
+ if options.help:
+ self.parser.print_help()
+ return
+
+ file = LldbFileObject(exe_ctx.process)
+ efi_symbols = EfiSymbols(exe_ctx.target)
+ self.result = result
+ self.exe_ctx = exe_ctx
+
+ if options.pei:
+ # XIP code ends up on a 4 byte boundary.
+ options.stride = 4
+ options.range = 0x100000
+ efi_symbols.configure_search(options.stride, options.range)
+
+ if not options.pc and options.address is None:
+ # default to
+ options.frame = True
+
+ if options.frame:
+ if not exe_ctx.frame.IsValid():
+ result.SetError("invalid frame")
+ return
+
+ threads = exe_ctx.process.threads if options.thread else [
+ exe_ctx.thread]
+
+ for thread in threads:
+ for frame in thread:
+ res = efi_symbols.address_to_symbols(frame.pc)
+ self.lldb_print(res)
+
+ else:
+ if options.address is not None:
+ address = options.address
+ elif options.pc:
+ try:
+ address = exe_ctx.thread.GetSelectedFrame().pc
+ except ValueError:
+ result.SetError("invalid pc")
+ return
+ else:
+ address = 0
+
+ res = efi_symbols.address_to_symbols(address.pc)
+ print(res)
+
+ if options.extended:
+
+ gST = exe_ctx.target.FindFirstGlobalVariable('gST')
+ if gST.error.fail:
+ print('Error: This command requires symbols to be loaded')
+ else:
+ table = EfiConfigurationTable(file, gST.unsigned)
+ for address, _ in table.DebugImageInfo():
+ res = efi_symbols.address_to_symbols(address)
+ self.lldb_print(res)
+
+ # keep trying module file names until we find a GUID xref file
+ for m in exe_ctx.target.modules:
+ if GuidNames.add_build_guid_file(str(m.file)):
+ break
+
+
+def CHAR16_TypeSummary(valobj, internal_dict):
+ '''
+ Display CHAR16 as a String in the debugger.
+ Note: utf-8 is returned as that is the value for the debugger.
+ '''
+ SBError = lldb.SBError()
+ Str = ''
+ if valobj.TypeIsPointerType():
+ if valobj.GetValueAsUnsigned() == 0:
+ return "NULL"
+
+ # CHAR16 * max string size 1024
+ for i in range(1024):
+ Char = valobj.GetPointeeData(i, 1).GetUnsignedInt16(SBError, 0)
+ if SBError.fail or Char == 0:
+ break
+ Str += chr(Char)
+ return 'L"' + Str + '"'
+
+ if valobj.num_children == 0:
+ # CHAR16
+ return "L'" + chr(valobj.unsigned) + "'"
+
+ else:
+ # CHAR16 []
+ for i in range(valobj.num_children):
+ Char = valobj.GetChildAtIndex(i).data.GetUnsignedInt16(SBError, 0)
+ if Char == 0:
+ break
+ Str += chr(Char)
+ return 'L"' + Str + '"'
+
+ return Str
+
+
+def CHAR8_TypeSummary(valobj, internal_dict):
+ '''
+ Display CHAR8 as a String in the debugger.
+ Note: utf-8 is returned as that is the value for the debugger.
+ '''
+ SBError = lldb.SBError()
+ Str = ''
+ if valobj.TypeIsPointerType():
+ if valobj.GetValueAsUnsigned() == 0:
+ return "NULL"
+
+ # CHAR8 * max string size 1024
+ for i in range(1024):
+ Char = valobj.GetPointeeData(i, 1).GetUnsignedInt8(SBError, 0)
+ if SBError.fail or Char == 0:
+ break
+ Str += chr(Char)
+ Str = '"' + Str + '"'
+ return Str
+
+ if valobj.num_children == 0:
+ # CHAR8
+ return "'" + chr(valobj.unsigned) + "'"
+ else:
+ # CHAR8 []
+ for i in range(valobj.num_children):
+ Char = valobj.GetChildAtIndex(i).data.GetUnsignedInt8(SBError, 0)
+ if SBError.fail or Char == 0:
+ break
+ Str += chr(Char)
+ return '"' + Str + '"'
+
+ return Str
+
+
+def EFI_STATUS_TypeSummary(valobj, internal_dict):
+ if valobj.TypeIsPointerType():
+ return ''
+ return str(EfiStatusClass(valobj.unsigned))
+
+
+def EFI_TPL_TypeSummary(valobj, internal_dict):
+ if valobj.TypeIsPointerType():
+ return ''
+ return str(EfiTpl(valobj.unsigned))
+
+
+def EFI_GUID_TypeSummary(valobj, internal_dict):
+ if valobj.TypeIsPointerType():
+ return ''
+ return str(GuidNames(bytes(valobj.data.uint8)))
+
+
+def EFI_BOOT_MODE_TypeSummary(valobj, internal_dict):
+ if valobj.TypeIsPointerType():
+ return ''
+ '''Return #define name for EFI_BOOT_MODE'''
+ return str(EfiBootMode(valobj.unsigned))
+
+
+def lldb_type_formaters(debugger, mod_name):
+ '''Teach lldb about EFI types'''
+
+ category = debugger.GetDefaultCategory()
+ FormatBool = lldb.SBTypeFormat(lldb.eFormatBoolean)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("BOOLEAN"), FormatBool)
+
+ FormatHex = lldb.SBTypeFormat(lldb.eFormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("UINT64"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("INT64"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("UINT32"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("INT32"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("UINT16"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("INT16"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("UINT8"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("INT8"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("UINTN"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("INTN"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("CHAR8"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("CHAR16"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier(
+ "EFI_PHYSICAL_ADDRESS"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier(
+ "PHYSICAL_ADDRESS"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier("EFI_LBA"), FormatHex)
+ category.AddTypeFormat(
+ lldb.SBTypeNameSpecifier("EFI_BOOT_MODE"), FormatHex)
+ category.AddTypeFormat(lldb.SBTypeNameSpecifier(
+ "EFI_FV_FILETYPE"), FormatHex)
+
+ #
+ # Smart type printing for EFI
+ #
+
+ debugger.HandleCommand(
+ f'type summary add GUID - -python-function '
+ f'{mod_name}.EFI_GUID_TypeSummary')
+ debugger.HandleCommand(
+ f'type summary add EFI_GUID --python-function '
+ f'{mod_name}.EFI_GUID_TypeSummary')
+ debugger.HandleCommand(
+ f'type summary add EFI_STATUS --python-function '
+ f'{mod_name}.EFI_STATUS_TypeSummary')
+ debugger.HandleCommand(
+ f'type summary add EFI_TPL - -python-function '
+ f'{mod_name}.EFI_TPL_TypeSummary')
+ debugger.HandleCommand(
+ f'type summary add EFI_BOOT_MODE --python-function '
+ f'{mod_name}.EFI_BOOT_MODE_TypeSummary')
+
+ debugger.HandleCommand(
+ f'type summary add CHAR16 --python-function '
+ f'{mod_name}.CHAR16_TypeSummary')
+
+ # W605 this is the correct escape sequence for the lldb command
+ debugger.HandleCommand(
+ f'type summary add --regex "CHAR16 \[[0-9]+\]" ' # noqa: W605
+ f'--python-function {mod_name}.CHAR16_TypeSummary')
+
+ debugger.HandleCommand(
+ f'type summary add CHAR8 --python-function '
+ f'{mod_name}.CHAR8_TypeSummary')
+
+ # W605 this is the correct escape sequence for the lldb command
+ debugger.HandleCommand(
+ f'type summary add --regex "CHAR8 \[[0-9]+\]" ' # noqa: W605
+ f'--python-function {mod_name}.CHAR8_TypeSummary')
+
+
+class LldbWorkaround:
+ needed = True
+
+ @classmethod
+ def activate(cls):
+ if cls.needed:
+ lldb.debugger.HandleCommand("process handle SIGALRM -n false")
+ cls.needed = False
+
+
+def LoadEmulatorEfiSymbols(frame, bp_loc, internal_dict):
+ #
+ # This is an lldb breakpoint script, and assumes the breakpoint is on a
+ # function with the same prototype as SecGdbScriptBreak(). The
+ # argument names are important as lldb looks them up.
+ #
+ # VOID
+ # SecGdbScriptBreak (
+ # char *FileName,
+ # int FileNameLength,
+ # long unsigned int LoadAddress,
+ # int AddSymbolFlag
+ # )
+ # {
+ # return;
+ # }
+ #
+ # When the emulator loads a PE/COFF image, it calls the stub function with
+ # the filename of the symbol file, the length of the FileName, the
+ # load address and a flag to indicate if this is a load or unload operation
+ #
+ LldbWorkaround().activate()
+
+ symbols = EfiSymbols(frame.thread.process.target)
+ LoadAddress = frame.FindVariable("LoadAddress").unsigned
+ if frame.FindVariable("AddSymbolFlag").unsigned == 1:
+ res = symbols.address_to_symbols(LoadAddress)
+ else:
+ res = symbols.unload_symbols(LoadAddress)
+ print(res)
+
+ # make breakpoint command continue
+ return False
+
+
+def __lldb_init_module(debugger, internal_dict):
+ '''
+ This initializer is being run from LLDB in the embedded command interpreter
+ '''
+
+ mod_name = Path(__file__).stem
+ lldb_type_formaters(debugger, mod_name)
+
+ # Add any commands contained in this module to LLDB
+ debugger.HandleCommand(
+ f'command script add -c {mod_name}.EfiSymbolicateCommand efi_symbols')
+ debugger.HandleCommand(
+ f'command script add -c {mod_name}.EfiGuidCommand guid')
+ debugger.HandleCommand(
+ f'command script add -c {mod_name}.EfiTableCommand table')
+ debugger.HandleCommand(
+ f'command script add -c {mod_name}.EfiHobCommand hob')
+ debugger.HandleCommand(
+ f'command script add -c {mod_name}.EfiDevicePathCommand devicepath')
+
+ print('EFI specific commands have been installed.')
+
+ # patch the ctypes c_void_p values if the debuggers OS and EFI have
+ # different ideas on the size of the debug.
+ try:
+ patch_ctypes(debugger.GetSelectedTarget().addr_size)
+ except ValueError:
+ # incase the script is imported and the debugger has not target
+ # defaults to sizeof(UINTN) == sizeof(UINT64)
+ patch_ctypes()
+
+ try:
+ target = debugger.GetSelectedTarget()
+ if target.FindFunctions('SecGdbScriptBreak').symbols:
+ breakpoint = target.BreakpointCreateByName('SecGdbScriptBreak')
+ # Set the emulator breakpoints, if we are in the emulator
+ cmd = 'breakpoint command add -s python -F '
+ cmd += f'efi_lldb.LoadEmulatorEfiSymbols {breakpoint.GetID()}'
+ debugger.HandleCommand(cmd)
+ print('Type r to run emulator.')
+ else:
+ raise ValueError("No Emulator Symbols")
+
+ except ValueError:
+ # default action when the script is imported
+ debugger.HandleCommand("efi_symbols --frame --extended")
+ debugger.HandleCommand("register read")
+ debugger.HandleCommand("bt all")
+
+
+if __name__ == '__main__':
+ pass
--
2.30.1 (Apple Git-130)

7141 - 7160 of 85991