* [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem()
@ 2020-06-16 9:04 Zhiguang Liu
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-16 9:04 UTC (permalink / raw)
To: devel; +Cc: Star Zeng, Liming Gao, Jian J Wang, Hao A Wu
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2002
This commit will update the logic in function SmmVariableGetStatistics()
so that the pointer fields ('Next' and 'Name') in structure
VARIABLE_INFO_ENTRY will not be copied into the SMM communication buffer.
Doing so will prevent SMM pointers address from being leaked into non-SMM
environment.
Please note that newly introduced internal function CopyVarInfoEntry()
will not use CopyMem() to copy the whole VARIABLE_INFO_ENTRY structure and
then zero out the 'Next' and 'Name' fields. This is for preventing race
conditions where the pointers value might still be read.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index caca5c3241..74e756bc00 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -315,6 +315,35 @@ GetFvbCountAndBuffer (
}
+/**
+ Copy only the meaningful fields of the variable statistics information from
+ source buffer to the destination buffer. Other fields are filled with zero.
+
+ @param[out] DstInfoEntry A pointer to the buffer of destination variable
+ information entry.
+ @param[in] SrcInfoEntry A pointer to the buffer of source variable
+ information entry.
+
+**/
+static
+VOID
+CopyVarInfoEntry (
+ OUT VARIABLE_INFO_ENTRY *DstInfoEntry,
+ IN VARIABLE_INFO_ENTRY *SrcInfoEntry
+ )
+{
+ DstInfoEntry->Next = NULL;
+ DstInfoEntry->Name = NULL;
+
+ CopyGuid (&DstInfoEntry->VendorGuid, &SrcInfoEntry->VendorGuid);
+ DstInfoEntry->Attributes = SrcInfoEntry->Attributes;
+ DstInfoEntry->ReadCount = SrcInfoEntry->ReadCount;
+ DstInfoEntry->WriteCount = SrcInfoEntry->WriteCount;
+ DstInfoEntry->DeleteCount = SrcInfoEntry->DeleteCount;
+ DstInfoEntry->CacheCount = SrcInfoEntry->CacheCount;
+ DstInfoEntry->Volatile = SrcInfoEntry->Volatile;
+}
+
/**
Get the variable statistics information from the information buffer pointed by gVariableInfo.
@@ -377,7 +406,7 @@ SmmVariableGetStatistics (
*InfoSize = StatisticsInfoSize;
return EFI_BUFFER_TOO_SMALL;
}
- CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY));
+ CopyVarInfoEntry (InfoEntry, VariableInfo);
CopyMem (InfoName, VariableInfo->Name, NameSize);
*InfoSize = StatisticsInfoSize;
return EFI_SUCCESS;
@@ -417,7 +446,7 @@ SmmVariableGetStatistics (
return EFI_BUFFER_TOO_SMALL;
}
- CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY));
+ CopyVarInfoEntry (InfoEntry, VariableInfo);
CopyMem (InfoName, VariableInfo->Name, NameSize);
*InfoSize = StatisticsInfoSize;
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM.
2020-06-16 9:04 [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Zhiguang Liu
@ 2020-06-16 9:04 ` Zhiguang Liu
2020-06-16 14:45 ` [edk2-devel] " Laszlo Ersek
` (2 more replies)
2020-06-16 9:04 ` [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries Zhiguang Liu
` (3 subsequent siblings)
4 siblings, 3 replies; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-16 9:04 UTC (permalink / raw)
To: devel; +Cc: Feng Tian, Star Zeng, Michael D Kinney, Laszlo Ersek, Jiewen Yao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE database.
The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to SMM database.
Exposing LOADED_IMAGE_PROTOCOL may cause SMM information leakage
to non-SMM component.
Cc: Feng Tian <feng.tian@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104 +++++++++++---------------------------------------------------------------------------------------------
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 ----------------------------------
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++--
3 files changed, 13 insertions(+), 129 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
index 76ee9e0b89..2be2866234 100644
--- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
+++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
@@ -316,7 +316,7 @@ SmmLoadImage (
EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv;
PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
- PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle);
+ PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
Buffer = NULL;
Size = 0;
@@ -546,51 +546,14 @@ SmmLoadImage (
DriverEntry->ImageBuffer = DstBuffer;
DriverEntry->NumberOfPage = PageCount;
- //
- // Allocate a Loaded Image Protocol in EfiBootServicesData
- //
- Status = gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&DriverEntry->LoadedImage);
- if (EFI_ERROR (Status)) {
- if (Buffer != NULL) {
- gBS->FreePool (Buffer);
- }
- SmmFreePages (DstBuffer, PageCount);
- return Status;
- }
-
- ZeroMem (DriverEntry->LoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL));
//
// Fill in the remaining fields of the Loaded Image Protocol instance.
- // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed.
//
- DriverEntry->LoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
- DriverEntry->LoadedImage->ParentHandle = gSmmCorePrivate->SmmIplImageHandle;
- DriverEntry->LoadedImage->SystemTable = gST;
- DriverEntry->LoadedImage->DeviceHandle = DeviceHandle;
-
DriverEntry->SmmLoadedImage.Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
DriverEntry->SmmLoadedImage.ParentHandle = gSmmCorePrivate->SmmIplImageHandle;
DriverEntry->SmmLoadedImage.SystemTable = gST;
DriverEntry->SmmLoadedImage.DeviceHandle = DeviceHandle;
- //
- // Make an EfiBootServicesData buffer copy of FilePath
- //
- Status = gBS->AllocatePool (EfiBootServicesData, GetDevicePathSize (FilePath), (VOID **)&DriverEntry->LoadedImage->FilePath);
- if (EFI_ERROR (Status)) {
- if (Buffer != NULL) {
- gBS->FreePool (Buffer);
- }
- SmmFreePages (DstBuffer, PageCount);
- return Status;
- }
- CopyMem (DriverEntry->LoadedImage->FilePath, FilePath, GetDevicePathSize (FilePath));
-
- DriverEntry->LoadedImage->ImageBase = (VOID *)(UINTN) ImageContext.ImageAddress;
- DriverEntry->LoadedImage->ImageSize = ImageContext.ImageSize;
- DriverEntry->LoadedImage->ImageCodeType = EfiRuntimeServicesCode;
- DriverEntry->LoadedImage->ImageDataType = EfiRuntimeServicesData;
-
//
// Make a buffer copy of FilePath
//
@@ -599,7 +562,6 @@ SmmLoadImage (
if (Buffer != NULL) {
gBS->FreePool (Buffer);
}
- gBS->FreePool (DriverEntry->LoadedImage->FilePath);
SmmFreePages (DstBuffer, PageCount);
return Status;
}
@@ -610,16 +572,6 @@ SmmLoadImage (
DriverEntry->SmmLoadedImage.ImageCodeType = EfiRuntimeServicesCode;
DriverEntry->SmmLoadedImage.ImageDataType = EfiRuntimeServicesData;
- //
- // Create a new image handle in the UEFI handle database for the SMM Driver
- //
- DriverEntry->ImageHandle = NULL;
- Status = gBS->InstallMultipleProtocolInterfaces (
- &DriverEntry->ImageHandle,
- &gEfiLoadedImageProtocolGuid, DriverEntry->LoadedImage,
- NULL
- );
-
//
// Create a new image handle in the SMM handle database for the SMM Driver
//
@@ -631,7 +583,7 @@ SmmLoadImage (
&DriverEntry->SmmLoadedImage
);
- PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle);
+ PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle);
//
// Print the load address and the PDB file name if it is available
@@ -856,7 +808,7 @@ SmmDispatcher (
// Untrused to Scheduled it would have already been loaded so we may need to
// skip the LoadImage
//
- if (DriverEntry->ImageHandle == NULL) {
+ if (DriverEntry->SmmImageHandle == NULL) {
Status = SmmLoadImage (DriverEntry);
//
@@ -885,8 +837,8 @@ SmmDispatcher (
REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
EFI_PROGRESS_CODE,
EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN,
- &DriverEntry->ImageHandle,
- sizeof (DriverEntry->ImageHandle)
+ &DriverEntry->SmmImageHandle,
+ sizeof (DriverEntry->SmmImageHandle)
);
//
@@ -898,9 +850,10 @@ SmmDispatcher (
// For each SMM driver, pass NULL as ImageHandle
//
RegisterSmramProfileImage (DriverEntry, TRUE);
- PERF_START_IMAGE_BEGIN (DriverEntry->ImageHandle);
- Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint)(DriverEntry->ImageHandle, gST);
- PERF_START_IMAGE_END (DriverEntry->ImageHandle);
+ DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n", DriverEntry->ImageEntryPoint));
+ PERF_START_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
+ Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint)(DriverEntry->SmmImageHandle, gST);
+ PERF_START_IMAGE_END (DriverEntry->SmmImageHandle);
if (EFI_ERROR(Status)){
DEBUG ((
DEBUG_ERROR,
@@ -910,20 +863,6 @@ SmmDispatcher (
));
UnregisterSmramProfileImage (DriverEntry, TRUE);
SmmFreePages(DriverEntry->ImageBuffer, DriverEntry->NumberOfPage);
- //
- // Uninstall LoadedImage
- //
- Status = gBS->UninstallProtocolInterface (
- DriverEntry->ImageHandle,
- &gEfiLoadedImageProtocolGuid,
- DriverEntry->LoadedImage
- );
- if (!EFI_ERROR (Status)) {
- if (DriverEntry->LoadedImage->FilePath != NULL) {
- gBS->FreePool (DriverEntry->LoadedImage->FilePath);
- }
- gBS->FreePool (DriverEntry->LoadedImage);
- }
Status = SmmUninstallProtocolInterface (
DriverEntry->SmmImageHandle,
&gEfiLoadedImageProtocolGuid,
@@ -939,8 +878,8 @@ SmmDispatcher (
REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
EFI_PROGRESS_CODE,
EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END,
- &DriverEntry->ImageHandle,
- sizeof (DriverEntry->ImageHandle)
+ &DriverEntry->SmmImageHandle,
+ sizeof (DriverEntry->SmmImageHandle)
);
if (!PreviousSmmEntryPointRegistered && gSmmCorePrivate->SmmEntryPointRegistered) {
@@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler (
//
// If this is the SMM core fill in it's DevicePath & DeviceHandle
//
- if (mSmmCoreLoadedImage->FilePath == NULL) {
- //
- // Maybe one special FV contains only one SMM_CORE module, so its device path must
- // be initialized completely.
- //
- EfiInitializeFwVolDevicepathNode (&mFvDevicePath.File, &NameGuid);
- SetDevicePathEndNode (&mFvDevicePath.End);
-
- //
- // Make an EfiBootServicesData buffer copy of FilePath
- //
- Status = gBS->AllocatePool (
- EfiBootServicesData,
- GetDevicePathSize ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath),
- (VOID **)&mSmmCoreLoadedImage->FilePath
- );
- ASSERT_EFI_ERROR (Status);
- CopyMem (mSmmCoreLoadedImage->FilePath, &mFvDevicePath, GetDevicePathSize ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath));
-
- mSmmCoreLoadedImage->DeviceHandle = FvHandle;
- }
if (mSmmCoreDriverEntry->SmmLoadedImage.FilePath == NULL) {
//
// Maybe one special FV contains only one SMM_CORE module, so its device path must
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index cfa9922cbd..c1b18b047e 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR *mFullSmramRanges;
EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry;
-EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage;
-
/**
Place holder function until all the SMM System Table Service are available.
@@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage (
)
{
EFI_STATUS Status;
- EFI_HANDLE Handle;
-
- //
- // Allocate a Loaded Image Protocol in EfiBootServicesData
- //
- Status = gBS->AllocatePool (EfiBootServicesData, sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&mSmmCoreLoadedImage);
- ASSERT_EFI_ERROR (Status);
-
- ZeroMem (mSmmCoreLoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL));
- //
- // Fill in the remaining fields of the Loaded Image Protocol instance.
- // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed.
- //
- mSmmCoreLoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
- mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate->SmmIplImageHandle;
- mSmmCoreLoadedImage->SystemTable = gST;
-
- mSmmCoreLoadedImage->ImageBase = (VOID *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase;
- mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate->PiSmmCoreImageSize;
- mSmmCoreLoadedImage->ImageCodeType = EfiRuntimeServicesCode;
- mSmmCoreLoadedImage->ImageDataType = EfiRuntimeServicesData;
-
- //
- // Create a new image handle in the UEFI handle database for the SMM Driver
- //
- Handle = NULL;
- Status = gBS->InstallMultipleProtocolInterfaces (
- &Handle,
- &gEfiLoadedImageProtocolGuid, mSmmCoreLoadedImage,
- NULL
- );
- ASSERT_EFI_ERROR (Status);
//
// Allocate a Loaded Image Protocol in SMM
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index 50a7fc0000..3bbd1e1603 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -122,8 +122,8 @@ typedef struct {
BOOLEAN Initialized;
BOOLEAN DepexProtocolError;
- EFI_HANDLE ImageHandle;
- EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
+ EFI_HANDLE ImageHandle_Reserved1;
+ EFI_LOADED_IMAGE_PROTOCOL *LoadedImage_Reserved2;
//
// Image EntryPoint in SMRAM
//
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries
2020-06-16 9:04 [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Zhiguang Liu
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
@ 2020-06-16 9:04 ` Zhiguang Liu
2020-06-16 22:41 ` Michael D Kinney
2020-07-02 13:51 ` Liming Gao
2020-06-16 9:04 ` [PATCH 4/5] SecurityPkg: " Zhiguang Liu
` (2 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-16 9:04 UTC (permalink / raw)
To: devel; +Cc: Kinney Michael D, Gao Liming
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
Remove DXE_SMM_DRIVER support for some libraries because they
have the risks of leaking data from SMM mode to non-SMM mode.
Cc: Kinney Michael D <michael.d.kinney@intel.com>
Cc: Gao Liming <liming.gao@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
| 2 +-
MdePkg/Library/DxeHstiLib/DxeHstiLib.inf | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--git a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
index 33eeab405f..acbe62e352 100644
--- a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
+++ b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
@@ -17,7 +17,7 @@
FILE_GUID = f773469b-e265-4b0c-b0a6-2f971fbfe72b
MODULE_TYPE = DXE_DRIVER
VERSION_STRING = 1.0
- LIBRARY_CLASS = ExtractGuidedSectionLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
+ LIBRARY_CLASS = ExtractGuidedSectionLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
CONSTRUCTOR = DxeExtractGuidedSectionLibConstructor
diff --git a/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf b/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
index c719481cdd..8c1cb3d0cc 100644
--- a/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
+++ b/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
@@ -14,7 +14,7 @@
FILE_GUID = 7DE1C620-F587-4116-A36D-40F3467B9A0C
MODULE_TYPE = DXE_DRIVER
VERSION_STRING = 1.0
- LIBRARY_CLASS = HstiLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
+ LIBRARY_CLASS = HstiLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
[Sources]
HstiAip.c
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/5] SecurityPkg: Remove DXE_SMM_DRIVER support for some libraries
2020-06-16 9:04 [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Zhiguang Liu
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
2020-06-16 9:04 ` [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries Zhiguang Liu
@ 2020-06-16 9:04 ` Zhiguang Liu
2020-06-16 14:47 ` [edk2-devel] " Laszlo Ersek
2020-06-16 9:04 ` [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe Zhiguang Liu
2020-06-16 22:38 ` [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Michael D Kinney
4 siblings, 1 reply; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-16 9:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
Remove DXE_SMM_DRIVER support for some libraries because they
have the risks of leaking data from SMM mode to non-SMM mode.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
index 1e1a639857..9494d04b1d 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
@@ -20,7 +20,7 @@
FILE_GUID = 0CA970E1-43FA-4402-BC0A-81AF336BFFD6
MODULE_TYPE = DXE_DRIVER
VERSION_STRING = 1.0
- LIBRARY_CLASS = NULL|DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
+ LIBRARY_CLASS = NULL|DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
CONSTRUCTOR = DxeImageVerificationLibConstructor
#
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
2020-06-16 9:04 [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Zhiguang Liu
` (2 preceding siblings ...)
2020-06-16 9:04 ` [PATCH 4/5] SecurityPkg: " Zhiguang Liu
@ 2020-06-16 9:04 ` Zhiguang Liu
2020-06-16 15:06 ` [edk2-devel] " Laszlo Ersek
2020-06-16 22:38 ` [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Michael D Kinney
4 siblings, 1 reply; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-16 9:04 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
To avoid leaking information from SMM, uninstall
EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 37 +++++++++++++++++++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
2 files changed, 38 insertions(+)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index db68e1316e..a1b209e125 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -520,6 +520,33 @@ SmmReadyToLockEventNotify (
return EFI_SUCCESS;
}
+/**
+ SMM End of Dxe event notification handler.
+
+ To avoid leaking information from SMM, uninstall EFI_SMM_CONFIGURATION_PROTOCOL
+ at end of Dxe.
+
+ @param[in] Protocol Points to the protocol's unique identifier.
+ @param[in] Interface Points to the interface instance.
+ @param[in] Handle The handle on which the interface was installed.
+
+ @retval EFI_SUCCESS Notification handler runs successfully.
+ **/
+EFI_STATUS
+EFIAPI
+SmmEndOfDxeNotify (
+ IN CONST EFI_GUID *Protocol,
+ IN VOID *Interface,
+ IN EFI_HANDLE Handle
+ )
+{
+ gBS->UninstallProtocolInterface (
+ gSmmCpuPrivate->SmmCpuHandle,
+ &gEfiSmmConfigurationProtocolGuid, &gSmmCpuPrivate->SmmConfiguration
+ );
+ return EFI_SUCCESS;
+}
+
/**
The module Entry Point of the CPU SMM driver.
@@ -1038,6 +1065,16 @@ PiCpuSmmEntry (
);
ASSERT_EFI_ERROR (Status);
+ //
+ // register SMM End of Dxe notification
+ //
+ Status = gSmst->SmmRegisterProtocolNotify (
+ &gEfiSmmEndOfDxeProtocolGuid,
+ SmmEndOfDxeNotify,
+ &Registration
+ );
+ ASSERT_EFI_ERROR (Status);
+
//
// Initialize SMM Profile feature
//
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 76b1462996..bb994814d6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -105,6 +105,7 @@
gEfiSmmConfigurationProtocolGuid ## PRODUCES
gEfiSmmCpuProtocolGuid ## PRODUCES
gEfiSmmReadyToLockProtocolGuid ## NOTIFY
+ gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
gEfiSmmCpuServiceProtocolGuid ## PRODUCES
gEdkiiSmmMemoryAttributeProtocolGuid ## PRODUCES
gEfiMmMpProtocolGuid ## PRODUCES
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM.
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
@ 2020-06-16 14:45 ` Laszlo Ersek
2020-06-16 22:39 ` Michael D Kinney
2020-06-16 22:44 ` Michael D Kinney
2 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2020-06-16 14:45 UTC (permalink / raw)
To: devel, zhiguang.liu
Cc: Feng Tian, Star Zeng, Michael D Kinney, Jiewen Yao, Eric Dong,
Hao A Wu, Jian J Wang, Ray Ni
On 06/16/20 11:04, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
>
> This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE database.
> The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to SMM database.
>
> Exposing LOADED_IMAGE_PROTOCOL may cause SMM information leakage
> to non-SMM component.
>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104 +++++++++++---------------------------------------------------------------------------------------------
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 ----------------------------------
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++--
> 3 files changed, 13 insertions(+), 129 deletions(-)
The CC list on this patch is lacking. Per
"BaseTools/Scripts/GetMaintainer.py", you should have included the
following CC's:
Eric Dong <eric.dong@intel.com>
Hao A Wu <hao.a.wu@intel.com>
Jian J Wang <jian.j.wang@intel.com>
Ray Ni <ray.ni@intel.com>
adding them now.
> diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> index 76ee9e0b89..2be2866234 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> @@ -316,7 +316,7 @@ SmmLoadImage (
> EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv;
> PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
>
> - PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle);
> + PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
>
> Buffer = NULL;
> Size = 0;
> @@ -546,51 +546,14 @@ SmmLoadImage (
> DriverEntry->ImageBuffer = DstBuffer;
> DriverEntry->NumberOfPage = PageCount;
>
> - //
> - // Allocate a Loaded Image Protocol in EfiBootServicesData
> - //
> - Status = gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&DriverEntry->LoadedImage);
> - if (EFI_ERROR (Status)) {
> - if (Buffer != NULL) {
> - gBS->FreePool (Buffer);
> - }
> - SmmFreePages (DstBuffer, PageCount);
> - return Status;
> - }
(Side comment: the error path that's being removed here seems to contain
a resource leak. We're past PeCoffLoaderLoadImage(), but we're not
calling PeCoffLoaderUnloadImage().)
> -
> - ZeroMem (DriverEntry->LoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL));
> //
> // Fill in the remaining fields of the Loaded Image Protocol instance.
> - // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed.
> //
> - DriverEntry->LoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> - DriverEntry->LoadedImage->ParentHandle = gSmmCorePrivate->SmmIplImageHandle;
> - DriverEntry->LoadedImage->SystemTable = gST;
> - DriverEntry->LoadedImage->DeviceHandle = DeviceHandle;
> -
> DriverEntry->SmmLoadedImage.Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> DriverEntry->SmmLoadedImage.ParentHandle = gSmmCorePrivate->SmmIplImageHandle;
> DriverEntry->SmmLoadedImage.SystemTable = gST;
> DriverEntry->SmmLoadedImage.DeviceHandle = DeviceHandle;
>
> - //
> - // Make an EfiBootServicesData buffer copy of FilePath
> - //
> - Status = gBS->AllocatePool (EfiBootServicesData, GetDevicePathSize (FilePath), (VOID **)&DriverEntry->LoadedImage->FilePath);
> - if (EFI_ERROR (Status)) {
> - if (Buffer != NULL) {
> - gBS->FreePool (Buffer);
> - }
> - SmmFreePages (DstBuffer, PageCount);
> - return Status;
> - }
(Side comment: at this point, we've used to leak even
"DriverEntry->LoadedImage".)
> - CopyMem (DriverEntry->LoadedImage->FilePath, FilePath, GetDevicePathSize (FilePath));
> -
> - DriverEntry->LoadedImage->ImageBase = (VOID *)(UINTN) ImageContext.ImageAddress;
> - DriverEntry->LoadedImage->ImageSize = ImageContext.ImageSize;
> - DriverEntry->LoadedImage->ImageCodeType = EfiRuntimeServicesCode;
> - DriverEntry->LoadedImage->ImageDataType = EfiRuntimeServicesData;
> -
> //
> // Make a buffer copy of FilePath
> //
> @@ -599,7 +562,6 @@ SmmLoadImage (
> if (Buffer != NULL) {
> gBS->FreePool (Buffer);
> }
> - gBS->FreePool (DriverEntry->LoadedImage->FilePath);
> SmmFreePages (DstBuffer, PageCount);
> return Status;
> }
(side comment: DriverEntry->LoadedImage was still leaked)
> @@ -610,16 +572,6 @@ SmmLoadImage (
> DriverEntry->SmmLoadedImage.ImageCodeType = EfiRuntimeServicesCode;
> DriverEntry->SmmLoadedImage.ImageDataType = EfiRuntimeServicesData;
>
> - //
> - // Create a new image handle in the UEFI handle database for the SMM Driver
> - //
> - DriverEntry->ImageHandle = NULL;
> - Status = gBS->InstallMultipleProtocolInterfaces (
> - &DriverEntry->ImageHandle,
> - &gEfiLoadedImageProtocolGuid, DriverEntry->LoadedImage,
> - NULL
> - );
> -
> //
> // Create a new image handle in the SMM handle database for the SMM Driver
> //
> @@ -631,7 +583,7 @@ SmmLoadImage (
> &DriverEntry->SmmLoadedImage
> );
>
> - PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle);
> + PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle);
>
> //
> // Print the load address and the PDB file name if it is available
> @@ -856,7 +808,7 @@ SmmDispatcher (
> // Untrused to Scheduled it would have already been loaded so we may need to
> // skip the LoadImage
> //
> - if (DriverEntry->ImageHandle == NULL) {
> + if (DriverEntry->SmmImageHandle == NULL) {
> Status = SmmLoadImage (DriverEntry);
>
> //
> @@ -885,8 +837,8 @@ SmmDispatcher (
> REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
> EFI_PROGRESS_CODE,
> EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN,
> - &DriverEntry->ImageHandle,
> - sizeof (DriverEntry->ImageHandle)
> + &DriverEntry->SmmImageHandle,
> + sizeof (DriverEntry->SmmImageHandle)
> );
>
> //
> @@ -898,9 +850,10 @@ SmmDispatcher (
> // For each SMM driver, pass NULL as ImageHandle
> //
> RegisterSmramProfileImage (DriverEntry, TRUE);
> - PERF_START_IMAGE_BEGIN (DriverEntry->ImageHandle);
> - Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint)(DriverEntry->ImageHandle, gST);
> - PERF_START_IMAGE_END (DriverEntry->ImageHandle);
> + DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n", DriverEntry->ImageEntryPoint));
(1) This DEBUG message belongs in a separate patch, similarly to commit
a1ddad95933e ("MdeModulePkg/PiSmmCore: log SMM image start failure",
2020-03-04).
> + PERF_START_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
> + Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint)(DriverEntry->SmmImageHandle, gST);
> + PERF_START_IMAGE_END (DriverEntry->SmmImageHandle);
> if (EFI_ERROR(Status)){
> DEBUG ((
> DEBUG_ERROR,
> @@ -910,20 +863,6 @@ SmmDispatcher (
> ));
> UnregisterSmramProfileImage (DriverEntry, TRUE);
> SmmFreePages(DriverEntry->ImageBuffer, DriverEntry->NumberOfPage);
> - //
> - // Uninstall LoadedImage
> - //
> - Status = gBS->UninstallProtocolInterface (
> - DriverEntry->ImageHandle,
> - &gEfiLoadedImageProtocolGuid,
> - DriverEntry->LoadedImage
> - );
> - if (!EFI_ERROR (Status)) {
> - if (DriverEntry->LoadedImage->FilePath != NULL) {
> - gBS->FreePool (DriverEntry->LoadedImage->FilePath);
> - }
> - gBS->FreePool (DriverEntry->LoadedImage);
> - }
> Status = SmmUninstallProtocolInterface (
> DriverEntry->SmmImageHandle,
> &gEfiLoadedImageProtocolGuid,
> @@ -939,8 +878,8 @@ SmmDispatcher (
> REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
> EFI_PROGRESS_CODE,
> EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END,
> - &DriverEntry->ImageHandle,
> - sizeof (DriverEntry->ImageHandle)
> + &DriverEntry->SmmImageHandle,
> + sizeof (DriverEntry->SmmImageHandle)
> );
>
> if (!PreviousSmmEntryPointRegistered && gSmmCorePrivate->SmmEntryPointRegistered) {
> @@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler (
> //
> // If this is the SMM core fill in it's DevicePath & DeviceHandle
> //
> - if (mSmmCoreLoadedImage->FilePath == NULL) {
> - //
> - // Maybe one special FV contains only one SMM_CORE module, so its device path must
> - // be initialized completely.
> - //
> - EfiInitializeFwVolDevicepathNode (&mFvDevicePath.File, &NameGuid);
> - SetDevicePathEndNode (&mFvDevicePath.End);
> -
> - //
> - // Make an EfiBootServicesData buffer copy of FilePath
> - //
> - Status = gBS->AllocatePool (
> - EfiBootServicesData,
> - GetDevicePathSize ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath),
> - (VOID **)&mSmmCoreLoadedImage->FilePath
> - );
> - ASSERT_EFI_ERROR (Status);
> - CopyMem (mSmmCoreLoadedImage->FilePath, &mFvDevicePath, GetDevicePathSize ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath));
> -
> - mSmmCoreLoadedImage->DeviceHandle = FvHandle;
> - }
> if (mSmmCoreDriverEntry->SmmLoadedImage.FilePath == NULL) {
> //
> // Maybe one special FV contains only one SMM_CORE module, so its device path must
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index cfa9922cbd..c1b18b047e 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR *mFullSmramRanges;
>
> EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry;
>
> -EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage;
> -
> /**
> Place holder function until all the SMM System Table Service are available.
>
> @@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage (
> )
> {
> EFI_STATUS Status;
> - EFI_HANDLE Handle;
> -
> - //
> - // Allocate a Loaded Image Protocol in EfiBootServicesData
> - //
> - Status = gBS->AllocatePool (EfiBootServicesData, sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&mSmmCoreLoadedImage);
> - ASSERT_EFI_ERROR (Status);
> -
> - ZeroMem (mSmmCoreLoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL));
> - //
> - // Fill in the remaining fields of the Loaded Image Protocol instance.
> - // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed.
> - //
> - mSmmCoreLoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> - mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate->SmmIplImageHandle;
> - mSmmCoreLoadedImage->SystemTable = gST;
> -
> - mSmmCoreLoadedImage->ImageBase = (VOID *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase;
> - mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate->PiSmmCoreImageSize;
> - mSmmCoreLoadedImage->ImageCodeType = EfiRuntimeServicesCode;
> - mSmmCoreLoadedImage->ImageDataType = EfiRuntimeServicesData;
> -
> - //
> - // Create a new image handle in the UEFI handle database for the SMM Driver
> - //
> - Handle = NULL;
> - Status = gBS->InstallMultipleProtocolInterfaces (
> - &Handle,
> - &gEfiLoadedImageProtocolGuid, mSmmCoreLoadedImage,
> - NULL
> - );
> - ASSERT_EFI_ERROR (Status);
>
> //
> // Allocate a Loaded Image Protocol in SMM
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> index 50a7fc0000..3bbd1e1603 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> @@ -122,8 +122,8 @@ typedef struct {
> BOOLEAN Initialized;
> BOOLEAN DepexProtocolError;
>
> - EFI_HANDLE ImageHandle;
> - EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
> + EFI_HANDLE ImageHandle_Reserved1;
> + EFI_LOADED_IMAGE_PROTOCOL *LoadedImage_Reserved2;
(2) Why are we not removing these fields?
(And even if we wanted to keep them, the new names do not conform to the
coding style.)
> //
> // Image EntryPoint in SMRAM
> //
>
Otherwise the patch looks good to me, but I've only done a superficial
check.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 4/5] SecurityPkg: Remove DXE_SMM_DRIVER support for some libraries
2020-06-16 9:04 ` [PATCH 4/5] SecurityPkg: " Zhiguang Liu
@ 2020-06-16 14:47 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2020-06-16 14:47 UTC (permalink / raw)
To: devel, zhiguang.liu; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang, Min Xu
On 06/16/20 11:04, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
> Remove DXE_SMM_DRIVER support for some libraries because they
> have the risks of leaking data from SMM mode to non-SMM mode.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> index 1e1a639857..9494d04b1d 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> @@ -20,7 +20,7 @@
> FILE_GUID = 0CA970E1-43FA-4402-BC0A-81AF336BFFD6
> MODULE_TYPE = DXE_DRIVER
> VERSION_STRING = 1.0
> - LIBRARY_CLASS = NULL|DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
> + LIBRARY_CLASS = NULL|DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
> CONSTRUCTOR = DxeImageVerificationLibConstructor
>
> #
>
"Min Xu <min.m.xu@intel.com>" is missing from the CC list, according to
"BaseTools/Scripts/GetMaintainer.py" (fixing that now).
thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
2020-06-16 9:04 ` [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe Zhiguang Liu
@ 2020-06-16 15:06 ` Laszlo Ersek
2020-06-17 5:30 ` Zhiguang Liu
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2020-06-16 15:06 UTC (permalink / raw)
To: devel, zhiguang.liu; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On 06/16/20 11:04, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
> To avoid leaking information from SMM, uninstall
> EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 37 +++++++++++++++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
> 2 files changed, 38 insertions(+)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index db68e1316e..a1b209e125 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -520,6 +520,33 @@ SmmReadyToLockEventNotify (
> return EFI_SUCCESS;
> }
>
> +/**
> + SMM End of Dxe event notification handler.
> +
> + To avoid leaking information from SMM, uninstall EFI_SMM_CONFIGURATION_PROTOCOL
> + at end of Dxe.
> +
> + @param[in] Protocol Points to the protocol's unique identifier.
> + @param[in] Interface Points to the interface instance.
> + @param[in] Handle The handle on which the interface was installed.
> +
> + @retval EFI_SUCCESS Notification handler runs successfully.
> + **/
> +EFI_STATUS
> +EFIAPI
> +SmmEndOfDxeNotify (
> + IN CONST EFI_GUID *Protocol,
> + IN VOID *Interface,
> + IN EFI_HANDLE Handle
> + )
> +{
> + gBS->UninstallProtocolInterface (
> + gSmmCpuPrivate->SmmCpuHandle,
> + &gEfiSmmConfigurationProtocolGuid, &gSmmCpuPrivate->SmmConfiguration
> + );
> + return EFI_SUCCESS;
> +}
(1) I suggest setting "gSmmCpuPrivate->SmmCpuHandle" to NULL here.
(2) I also suggest de-registering the gEfiSmmEndOfDxeProtocolGuid
notification.
Thanks
Laszlo
> +
> /**
> The module Entry Point of the CPU SMM driver.
>
> @@ -1038,6 +1065,16 @@ PiCpuSmmEntry (
> );
> ASSERT_EFI_ERROR (Status);
>
> + //
> + // register SMM End of Dxe notification
> + //
> + Status = gSmst->SmmRegisterProtocolNotify (
> + &gEfiSmmEndOfDxeProtocolGuid,
> + SmmEndOfDxeNotify,
> + &Registration
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> //
> // Initialize SMM Profile feature
> //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 76b1462996..bb994814d6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -105,6 +105,7 @@
> gEfiSmmConfigurationProtocolGuid ## PRODUCES
> gEfiSmmCpuProtocolGuid ## PRODUCES
> gEfiSmmReadyToLockProtocolGuid ## NOTIFY
> + gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
> gEfiSmmCpuServiceProtocolGuid ## PRODUCES
> gEdkiiSmmMemoryAttributeProtocolGuid ## PRODUCES
> gEfiMmMpProtocolGuid ## PRODUCES
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem()
2020-06-16 9:04 [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Zhiguang Liu
` (3 preceding siblings ...)
2020-06-16 9:04 ` [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe Zhiguang Liu
@ 2020-06-16 22:38 ` Michael D Kinney
2020-06-17 5:25 ` Zhiguang Liu
4 siblings, 1 reply; 17+ messages in thread
From: Michael D Kinney @ 2020-06-16 22:38 UTC (permalink / raw)
To: devel@edk2.groups.io, Liu, Zhiguang, Kinney, Michael D
Cc: Zeng, Star, Gao, Liming, Wang, Jian J, Wu, Hao A
Zhiguang,
An implementation of CopyGuid() could use CopyMem().
Does CopyGuid() also need to be avoided?
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Zhiguang Liu
> Sent: Tuesday, June 16, 2020 2:05 AM
> To: devel@edk2.groups.io
> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid
> SMM pointers being leaked by not using CopyMem()
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2002
>
> This commit will update the logic in function
> SmmVariableGetStatistics()
> so that the pointer fields ('Next' and 'Name') in
> structure
> VARIABLE_INFO_ENTRY will not be copied into the SMM
> communication buffer.
>
> Doing so will prevent SMM pointers address from being
> leaked into non-SMM
> environment.
>
> Please note that newly introduced internal function
> CopyVarInfoEntry()
> will not use CopyMem() to copy the whole
> VARIABLE_INFO_ENTRY structure and
> then zero out the 'Next' and 'Name' fields. This is for
> preventing race
> conditions where the pointers value might still be read.
>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> .c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> .c
> index caca5c3241..74e756bc00 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> .c
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> .c
> @@ -315,6 +315,35 @@ GetFvbCountAndBuffer (
> }
>
>
>
>
>
> +/**
>
> + Copy only the meaningful fields of the variable
> statistics information from
>
> + source buffer to the destination buffer. Other fields
> are filled with zero.
>
> +
>
> + @param[out] DstInfoEntry A pointer to the buffer
> of destination variable
>
> + information entry.
>
> + @param[in] SrcInfoEntry A pointer to the buffer
> of source variable
>
> + information entry.
>
> +
>
> +**/
>
> +static
>
> +VOID
>
> +CopyVarInfoEntry (
>
> + OUT VARIABLE_INFO_ENTRY *DstInfoEntry,
>
> + IN VARIABLE_INFO_ENTRY *SrcInfoEntry
>
> + )
>
> +{
>
> + DstInfoEntry->Next = NULL;
>
> + DstInfoEntry->Name = NULL;
>
> +
>
> + CopyGuid (&DstInfoEntry->VendorGuid, &SrcInfoEntry-
> >VendorGuid);
>
> + DstInfoEntry->Attributes = SrcInfoEntry->Attributes;
>
> + DstInfoEntry->ReadCount = SrcInfoEntry->ReadCount;
>
> + DstInfoEntry->WriteCount = SrcInfoEntry->WriteCount;
>
> + DstInfoEntry->DeleteCount = SrcInfoEntry-
> >DeleteCount;
>
> + DstInfoEntry->CacheCount = SrcInfoEntry->CacheCount;
>
> + DstInfoEntry->Volatile = SrcInfoEntry->Volatile;
>
> +}
>
> +
>
> /**
>
> Get the variable statistics information from the
> information buffer pointed by gVariableInfo.
>
>
>
> @@ -377,7 +406,7 @@ SmmVariableGetStatistics (
> *InfoSize = StatisticsInfoSize;
>
> return EFI_BUFFER_TOO_SMALL;
>
> }
>
> - CopyMem (InfoEntry, VariableInfo, sizeof
> (VARIABLE_INFO_ENTRY));
>
> + CopyVarInfoEntry (InfoEntry, VariableInfo);
>
> CopyMem (InfoName, VariableInfo->Name, NameSize);
>
> *InfoSize = StatisticsInfoSize;
>
> return EFI_SUCCESS;
>
> @@ -417,7 +446,7 @@ SmmVariableGetStatistics (
> return EFI_BUFFER_TOO_SMALL;
>
> }
>
>
>
> - CopyMem (InfoEntry, VariableInfo, sizeof
> (VARIABLE_INFO_ENTRY));
>
> + CopyVarInfoEntry (InfoEntry, VariableInfo);
>
> CopyMem (InfoName, VariableInfo->Name, NameSize);
>
> *InfoSize = StatisticsInfoSize;
>
>
>
> --
> 2.25.1.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this
> group.
>
> View/Reply Online (#61324):
> https://edk2.groups.io/g/devel/message/61324
> Mute This Topic: https://groups.io/mt/74912557/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM.
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
2020-06-16 14:45 ` [edk2-devel] " Laszlo Ersek
@ 2020-06-16 22:39 ` Michael D Kinney
2020-06-23 6:12 ` Zhiguang Liu
2020-06-16 22:44 ` Michael D Kinney
2 siblings, 1 reply; 17+ messages in thread
From: Michael D Kinney @ 2020-06-16 22:39 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io, Kinney, Michael D
Cc: Tian, Feng, Zeng, Star, Laszlo Ersek, Yao, Jiewen
Zhiguang,
There are some source level debug solutions that depend
on the EDI_LOADED_IMAGE_PROTOCOL structure. Does this
change reduce the source level debug capabilities for
SMM drivers under development?
Thanks,
Mike
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, June 16, 2020 2:05 AM
> To: devel@edk2.groups.io
> Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI
> LOADED_IMAGE from SMM.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
>
> This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE
> database.
> The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to
> SMM database.
>
> Exposing LOADED_IMAGE_PROTOCOL may cause SMM information
> leakage
> to non-SMM component.
>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104
> +++++++++++---------------------------------------------
> ------------------------------------------------
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 --------
> --------------------------
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++--
> 3 files changed, 13 insertions(+), 129 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> index 76ee9e0b89..2be2866234 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> @@ -316,7 +316,7 @@ SmmLoadImage (
> EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv;
>
> PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
>
>
>
> - PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle);
>
> + PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
>
>
>
> Buffer = NULL;
>
> Size = 0;
>
> @@ -546,51 +546,14 @@ SmmLoadImage (
> DriverEntry->ImageBuffer = DstBuffer;
>
> DriverEntry->NumberOfPage = PageCount;
>
>
>
> - //
>
> - // Allocate a Loaded Image Protocol in
> EfiBootServicesData
>
> - //
>
> - Status = gBS->AllocatePool (EfiBootServicesData,
> sizeof (EFI_LOADED_IMAGE_PROTOCOL), (VOID
> **)&DriverEntry->LoadedImage);
>
> - if (EFI_ERROR (Status)) {
>
> - if (Buffer != NULL) {
>
> - gBS->FreePool (Buffer);
>
> - }
>
> - SmmFreePages (DstBuffer, PageCount);
>
> - return Status;
>
> - }
>
> -
>
> - ZeroMem (DriverEntry->LoadedImage, sizeof
> (EFI_LOADED_IMAGE_PROTOCOL));
>
> //
>
> // Fill in the remaining fields of the Loaded Image
> Protocol instance.
>
> - // Note: ImageBase is an SMRAM address that can not
> be accessed outside of SMRAM if SMRAM window is closed.
>
> //
>
> - DriverEntry->LoadedImage->Revision =
> EFI_LOADED_IMAGE_PROTOCOL_REVISION;
>
> - DriverEntry->LoadedImage->ParentHandle =
> gSmmCorePrivate->SmmIplImageHandle;
>
> - DriverEntry->LoadedImage->SystemTable = gST;
>
> - DriverEntry->LoadedImage->DeviceHandle =
> DeviceHandle;
>
> -
>
> DriverEntry->SmmLoadedImage.Revision =
> EFI_LOADED_IMAGE_PROTOCOL_REVISION;
>
> DriverEntry->SmmLoadedImage.ParentHandle =
> gSmmCorePrivate->SmmIplImageHandle;
>
> DriverEntry->SmmLoadedImage.SystemTable = gST;
>
> DriverEntry->SmmLoadedImage.DeviceHandle =
> DeviceHandle;
>
>
>
> - //
>
> - // Make an EfiBootServicesData buffer copy of
> FilePath
>
> - //
>
> - Status = gBS->AllocatePool (EfiBootServicesData,
> GetDevicePathSize (FilePath), (VOID **)&DriverEntry-
> >LoadedImage->FilePath);
>
> - if (EFI_ERROR (Status)) {
>
> - if (Buffer != NULL) {
>
> - gBS->FreePool (Buffer);
>
> - }
>
> - SmmFreePages (DstBuffer, PageCount);
>
> - return Status;
>
> - }
>
> - CopyMem (DriverEntry->LoadedImage->FilePath,
> FilePath, GetDevicePathSize (FilePath));
>
> -
>
> - DriverEntry->LoadedImage->ImageBase = (VOID
> *)(UINTN) ImageContext.ImageAddress;
>
> - DriverEntry->LoadedImage->ImageSize =
> ImageContext.ImageSize;
>
> - DriverEntry->LoadedImage->ImageCodeType =
> EfiRuntimeServicesCode;
>
> - DriverEntry->LoadedImage->ImageDataType =
> EfiRuntimeServicesData;
>
> -
>
> //
>
> // Make a buffer copy of FilePath
>
> //
>
> @@ -599,7 +562,6 @@ SmmLoadImage (
> if (Buffer != NULL) {
>
> gBS->FreePool (Buffer);
>
> }
>
> - gBS->FreePool (DriverEntry->LoadedImage->FilePath);
>
> SmmFreePages (DstBuffer, PageCount);
>
> return Status;
>
> }
>
> @@ -610,16 +572,6 @@ SmmLoadImage (
> DriverEntry->SmmLoadedImage.ImageCodeType =
> EfiRuntimeServicesCode;
>
> DriverEntry->SmmLoadedImage.ImageDataType =
> EfiRuntimeServicesData;
>
>
>
> - //
>
> - // Create a new image handle in the UEFI handle
> database for the SMM Driver
>
> - //
>
> - DriverEntry->ImageHandle = NULL;
>
> - Status = gBS->InstallMultipleProtocolInterfaces (
>
> - &DriverEntry->ImageHandle,
>
> - &gEfiLoadedImageProtocolGuid,
> DriverEntry->LoadedImage,
>
> - NULL
>
> - );
>
> -
>
> //
>
> // Create a new image handle in the SMM handle
> database for the SMM Driver
>
> //
>
> @@ -631,7 +583,7 @@ SmmLoadImage (
> &DriverEntry->SmmLoadedImage
>
> );
>
>
>
> - PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle);
>
> + PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle);
>
>
>
> //
>
> // Print the load address and the PDB file name if it
> is available
>
> @@ -856,7 +808,7 @@ SmmDispatcher (
> // Untrused to Scheduled it would have already
> been loaded so we may need to
>
> // skip the LoadImage
>
> //
>
> - if (DriverEntry->ImageHandle == NULL) {
>
> + if (DriverEntry->SmmImageHandle == NULL) {
>
> Status = SmmLoadImage (DriverEntry);
>
>
>
> //
>
> @@ -885,8 +837,8 @@ SmmDispatcher (
> REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
>
> EFI_PROGRESS_CODE,
>
> EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN,
>
> - &DriverEntry->ImageHandle,
>
> - sizeof (DriverEntry->ImageHandle)
>
> + &DriverEntry->SmmImageHandle,
>
> + sizeof (DriverEntry->SmmImageHandle)
>
> );
>
>
>
> //
>
> @@ -898,9 +850,10 @@ SmmDispatcher (
> // For each SMM driver, pass NULL as ImageHandle
>
> //
>
> RegisterSmramProfileImage (DriverEntry, TRUE);
>
> - PERF_START_IMAGE_BEGIN (DriverEntry-
> >ImageHandle);
>
> - Status =
> ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry-
> >ImageEntryPoint)(DriverEntry->ImageHandle, gST);
>
> - PERF_START_IMAGE_END (DriverEntry->ImageHandle);
>
> + DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n",
> DriverEntry->ImageEntryPoint));
>
> + PERF_START_IMAGE_BEGIN (DriverEntry-
> >SmmImageHandle);
>
> + Status =
> ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry-
> >ImageEntryPoint)(DriverEntry->SmmImageHandle, gST);
>
> + PERF_START_IMAGE_END (DriverEntry-
> >SmmImageHandle);
>
> if (EFI_ERROR(Status)){
>
> DEBUG ((
>
> DEBUG_ERROR,
>
> @@ -910,20 +863,6 @@ SmmDispatcher (
> ));
>
> UnregisterSmramProfileImage (DriverEntry,
> TRUE);
>
> SmmFreePages(DriverEntry->ImageBuffer,
> DriverEntry->NumberOfPage);
>
> - //
>
> - // Uninstall LoadedImage
>
> - //
>
> - Status = gBS->UninstallProtocolInterface (
>
> - DriverEntry->ImageHandle,
>
> - &gEfiLoadedImageProtocolGuid,
>
> - DriverEntry->LoadedImage
>
> - );
>
> - if (!EFI_ERROR (Status)) {
>
> - if (DriverEntry->LoadedImage->FilePath !=
> NULL) {
>
> - gBS->FreePool (DriverEntry->LoadedImage-
> >FilePath);
>
> - }
>
> - gBS->FreePool (DriverEntry->LoadedImage);
>
> - }
>
> Status = SmmUninstallProtocolInterface (
>
> DriverEntry->SmmImageHandle,
>
> &gEfiLoadedImageProtocolGuid,
>
> @@ -939,8 +878,8 @@ SmmDispatcher (
> REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
>
> EFI_PROGRESS_CODE,
>
> EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END,
>
> - &DriverEntry->ImageHandle,
>
> - sizeof (DriverEntry->ImageHandle)
>
> + &DriverEntry->SmmImageHandle,
>
> + sizeof (DriverEntry->SmmImageHandle)
>
> );
>
>
>
> if (!PreviousSmmEntryPointRegistered &&
> gSmmCorePrivate->SmmEntryPointRegistered) {
>
> @@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler (
> //
>
> // If this is the SMM core fill in it's
> DevicePath & DeviceHandle
>
> //
>
> - if (mSmmCoreLoadedImage->FilePath == NULL)
> {
>
> - //
>
> - // Maybe one special FV contains only one
> SMM_CORE module, so its device path must
>
> - // be initialized completely.
>
> - //
>
> - EfiInitializeFwVolDevicepathNode
> (&mFvDevicePath.File, &NameGuid);
>
> - SetDevicePathEndNode
> (&mFvDevicePath.End);
>
> -
>
> - //
>
> - // Make an EfiBootServicesData buffer
> copy of FilePath
>
> - //
>
> - Status = gBS->AllocatePool (
>
> - EfiBootServicesData,
>
> - GetDevicePathSize
> ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath),
>
> - (VOID
> **)&mSmmCoreLoadedImage->FilePath
>
> - );
>
> - ASSERT_EFI_ERROR (Status);
>
> - CopyMem (mSmmCoreLoadedImage->FilePath,
> &mFvDevicePath, GetDevicePathSize
> ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath));
>
> -
>
> - mSmmCoreLoadedImage->DeviceHandle =
> FvHandle;
>
> - }
>
> if (mSmmCoreDriverEntry-
> >SmmLoadedImage.FilePath == NULL) {
>
> //
>
> // Maybe one special FV contains only one
> SMM_CORE module, so its device path must
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index cfa9922cbd..c1b18b047e 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR
> *mFullSmramRanges;
>
>
> EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry;
>
>
>
> -EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage;
>
> -
>
> /**
>
> Place holder function until all the SMM System Table
> Service are available.
>
>
>
> @@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage (
> )
>
> {
>
> EFI_STATUS Status;
>
> - EFI_HANDLE Handle;
>
> -
>
> - //
>
> - // Allocate a Loaded Image Protocol in
> EfiBootServicesData
>
> - //
>
> - Status = gBS->AllocatePool (EfiBootServicesData,
> sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID
> **)&mSmmCoreLoadedImage);
>
> - ASSERT_EFI_ERROR (Status);
>
> -
>
> - ZeroMem (mSmmCoreLoadedImage, sizeof
> (EFI_LOADED_IMAGE_PROTOCOL));
>
> - //
>
> - // Fill in the remaining fields of the Loaded Image
> Protocol instance.
>
> - // Note: ImageBase is an SMRAM address that can not
> be accessed outside of SMRAM if SMRAM window is closed.
>
> - //
>
> - mSmmCoreLoadedImage->Revision =
> EFI_LOADED_IMAGE_PROTOCOL_REVISION;
>
> - mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate-
> >SmmIplImageHandle;
>
> - mSmmCoreLoadedImage->SystemTable = gST;
>
> -
>
> - mSmmCoreLoadedImage->ImageBase = (VOID
> *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase;
>
> - mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate-
> >PiSmmCoreImageSize;
>
> - mSmmCoreLoadedImage->ImageCodeType =
> EfiRuntimeServicesCode;
>
> - mSmmCoreLoadedImage->ImageDataType =
> EfiRuntimeServicesData;
>
> -
>
> - //
>
> - // Create a new image handle in the UEFI handle
> database for the SMM Driver
>
> - //
>
> - Handle = NULL;
>
> - Status = gBS->InstallMultipleProtocolInterfaces (
>
> - &Handle,
>
> - &gEfiLoadedImageProtocolGuid,
> mSmmCoreLoadedImage,
>
> - NULL
>
> - );
>
> - ASSERT_EFI_ERROR (Status);
>
>
>
> //
>
> // Allocate a Loaded Image Protocol in SMM
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> index 50a7fc0000..3bbd1e1603 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> @@ -122,8 +122,8 @@ typedef struct {
> BOOLEAN Initialized;
>
> BOOLEAN DepexProtocolError;
>
>
>
> - EFI_HANDLE ImageHandle;
>
> - EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
>
> + EFI_HANDLE
> ImageHandle_Reserved1;
>
> + EFI_LOADED_IMAGE_PROTOCOL
> *LoadedImage_Reserved2;
>
> //
>
> // Image EntryPoint in SMRAM
>
> //
>
> --
> 2.25.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries
2020-06-16 9:04 ` [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries Zhiguang Liu
@ 2020-06-16 22:41 ` Michael D Kinney
2020-07-02 13:51 ` Liming Gao
1 sibling, 0 replies; 17+ messages in thread
From: Michael D Kinney @ 2020-06-16 22:41 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io, Kinney, Michael D; +Cc: Gao, Liming
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Mike
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, June 16, 2020 2:05 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> Liming <liming.gao@intel.com>
> Subject: [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER
> support for some libraries
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
> Remove DXE_SMM_DRIVER support for some libraries because
> they
> have the risks of leaking data from SMM mode to non-SMM
> mode.
>
> Cc: Kinney Michael D <michael.d.kinney@intel.com>
> Cc: Gao Liming <liming.gao@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>
> MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuid
> edSectionLib.inf | 2 +-
> MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git
> a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGu
> idedSectionLib.inf
> b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGu
> idedSectionLib.inf
> index 33eeab405f..acbe62e352 100644
> ---
> a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGu
> idedSectionLib.inf
> +++
> b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGu
> idedSectionLib.inf
> @@ -17,7 +17,7 @@
> FILE_GUID = f773469b-e265-4b0c-
> b0a6-2f971fbfe72b
>
> MODULE_TYPE = DXE_DRIVER
>
> VERSION_STRING = 1.0
>
> - LIBRARY_CLASS =
> ExtractGuidedSectionLib|DXE_CORE DXE_DRIVER
> DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION
> UEFI_DRIVER
>
> + LIBRARY_CLASS =
> ExtractGuidedSectionLib|DXE_CORE DXE_DRIVER
> DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
>
>
>
> CONSTRUCTOR =
> DxeExtractGuidedSectionLibConstructor
>
>
>
> diff --git a/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> b/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> index c719481cdd..8c1cb3d0cc 100644
> --- a/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> +++ b/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> @@ -14,7 +14,7 @@
> FILE_GUID = 7DE1C620-F587-4116-
> A36D-40F3467B9A0C
>
> MODULE_TYPE = DXE_DRIVER
>
> VERSION_STRING = 1.0
>
> - LIBRARY_CLASS = HstiLib|DXE_CORE
> DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER
> UEFI_APPLICATION UEFI_DRIVER
>
> + LIBRARY_CLASS = HstiLib|DXE_CORE
> DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION
> UEFI_DRIVER
>
>
>
> [Sources]
>
> HstiAip.c
>
> --
> 2.25.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM.
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
2020-06-16 14:45 ` [edk2-devel] " Laszlo Ersek
2020-06-16 22:39 ` Michael D Kinney
@ 2020-06-16 22:44 ` Michael D Kinney
2 siblings, 0 replies; 17+ messages in thread
From: Michael D Kinney @ 2020-06-16 22:44 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io, Kinney, Michael D
Cc: Tian, Feng, Zeng, Star, Laszlo Ersek, Yao, Jiewen
Zhiguang,
The commit message should not have more than one Signed-off-by.
Use of Suggested-by may be more appropriate here.
Mike
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, June 16, 2020 2:05 AM
> To: devel@edk2.groups.io
> Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI
> LOADED_IMAGE from SMM.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
>
> This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE
> database.
> The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to
> SMM database.
>
> Exposing LOADED_IMAGE_PROTOCOL may cause SMM information
> leakage
> to non-SMM component.
>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104
> +++++++++++---------------------------------------------
> ------------------------------------------------
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 --------
> --------------------------
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++--
> 3 files changed, 13 insertions(+), 129 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> index 76ee9e0b89..2be2866234 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> @@ -316,7 +316,7 @@ SmmLoadImage (
> EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv;
>
> PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
>
>
>
> - PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle);
>
> + PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
>
>
>
> Buffer = NULL;
>
> Size = 0;
>
> @@ -546,51 +546,14 @@ SmmLoadImage (
> DriverEntry->ImageBuffer = DstBuffer;
>
> DriverEntry->NumberOfPage = PageCount;
>
>
>
> - //
>
> - // Allocate a Loaded Image Protocol in
> EfiBootServicesData
>
> - //
>
> - Status = gBS->AllocatePool (EfiBootServicesData,
> sizeof (EFI_LOADED_IMAGE_PROTOCOL), (VOID
> **)&DriverEntry->LoadedImage);
>
> - if (EFI_ERROR (Status)) {
>
> - if (Buffer != NULL) {
>
> - gBS->FreePool (Buffer);
>
> - }
>
> - SmmFreePages (DstBuffer, PageCount);
>
> - return Status;
>
> - }
>
> -
>
> - ZeroMem (DriverEntry->LoadedImage, sizeof
> (EFI_LOADED_IMAGE_PROTOCOL));
>
> //
>
> // Fill in the remaining fields of the Loaded Image
> Protocol instance.
>
> - // Note: ImageBase is an SMRAM address that can not
> be accessed outside of SMRAM if SMRAM window is closed.
>
> //
>
> - DriverEntry->LoadedImage->Revision =
> EFI_LOADED_IMAGE_PROTOCOL_REVISION;
>
> - DriverEntry->LoadedImage->ParentHandle =
> gSmmCorePrivate->SmmIplImageHandle;
>
> - DriverEntry->LoadedImage->SystemTable = gST;
>
> - DriverEntry->LoadedImage->DeviceHandle =
> DeviceHandle;
>
> -
>
> DriverEntry->SmmLoadedImage.Revision =
> EFI_LOADED_IMAGE_PROTOCOL_REVISION;
>
> DriverEntry->SmmLoadedImage.ParentHandle =
> gSmmCorePrivate->SmmIplImageHandle;
>
> DriverEntry->SmmLoadedImage.SystemTable = gST;
>
> DriverEntry->SmmLoadedImage.DeviceHandle =
> DeviceHandle;
>
>
>
> - //
>
> - // Make an EfiBootServicesData buffer copy of
> FilePath
>
> - //
>
> - Status = gBS->AllocatePool (EfiBootServicesData,
> GetDevicePathSize (FilePath), (VOID **)&DriverEntry-
> >LoadedImage->FilePath);
>
> - if (EFI_ERROR (Status)) {
>
> - if (Buffer != NULL) {
>
> - gBS->FreePool (Buffer);
>
> - }
>
> - SmmFreePages (DstBuffer, PageCount);
>
> - return Status;
>
> - }
>
> - CopyMem (DriverEntry->LoadedImage->FilePath,
> FilePath, GetDevicePathSize (FilePath));
>
> -
>
> - DriverEntry->LoadedImage->ImageBase = (VOID
> *)(UINTN) ImageContext.ImageAddress;
>
> - DriverEntry->LoadedImage->ImageSize =
> ImageContext.ImageSize;
>
> - DriverEntry->LoadedImage->ImageCodeType =
> EfiRuntimeServicesCode;
>
> - DriverEntry->LoadedImage->ImageDataType =
> EfiRuntimeServicesData;
>
> -
>
> //
>
> // Make a buffer copy of FilePath
>
> //
>
> @@ -599,7 +562,6 @@ SmmLoadImage (
> if (Buffer != NULL) {
>
> gBS->FreePool (Buffer);
>
> }
>
> - gBS->FreePool (DriverEntry->LoadedImage->FilePath);
>
> SmmFreePages (DstBuffer, PageCount);
>
> return Status;
>
> }
>
> @@ -610,16 +572,6 @@ SmmLoadImage (
> DriverEntry->SmmLoadedImage.ImageCodeType =
> EfiRuntimeServicesCode;
>
> DriverEntry->SmmLoadedImage.ImageDataType =
> EfiRuntimeServicesData;
>
>
>
> - //
>
> - // Create a new image handle in the UEFI handle
> database for the SMM Driver
>
> - //
>
> - DriverEntry->ImageHandle = NULL;
>
> - Status = gBS->InstallMultipleProtocolInterfaces (
>
> - &DriverEntry->ImageHandle,
>
> - &gEfiLoadedImageProtocolGuid,
> DriverEntry->LoadedImage,
>
> - NULL
>
> - );
>
> -
>
> //
>
> // Create a new image handle in the SMM handle
> database for the SMM Driver
>
> //
>
> @@ -631,7 +583,7 @@ SmmLoadImage (
> &DriverEntry->SmmLoadedImage
>
> );
>
>
>
> - PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle);
>
> + PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle);
>
>
>
> //
>
> // Print the load address and the PDB file name if it
> is available
>
> @@ -856,7 +808,7 @@ SmmDispatcher (
> // Untrused to Scheduled it would have already
> been loaded so we may need to
>
> // skip the LoadImage
>
> //
>
> - if (DriverEntry->ImageHandle == NULL) {
>
> + if (DriverEntry->SmmImageHandle == NULL) {
>
> Status = SmmLoadImage (DriverEntry);
>
>
>
> //
>
> @@ -885,8 +837,8 @@ SmmDispatcher (
> REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
>
> EFI_PROGRESS_CODE,
>
> EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN,
>
> - &DriverEntry->ImageHandle,
>
> - sizeof (DriverEntry->ImageHandle)
>
> + &DriverEntry->SmmImageHandle,
>
> + sizeof (DriverEntry->SmmImageHandle)
>
> );
>
>
>
> //
>
> @@ -898,9 +850,10 @@ SmmDispatcher (
> // For each SMM driver, pass NULL as ImageHandle
>
> //
>
> RegisterSmramProfileImage (DriverEntry, TRUE);
>
> - PERF_START_IMAGE_BEGIN (DriverEntry-
> >ImageHandle);
>
> - Status =
> ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry-
> >ImageEntryPoint)(DriverEntry->ImageHandle, gST);
>
> - PERF_START_IMAGE_END (DriverEntry->ImageHandle);
>
> + DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n",
> DriverEntry->ImageEntryPoint));
>
> + PERF_START_IMAGE_BEGIN (DriverEntry-
> >SmmImageHandle);
>
> + Status =
> ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry-
> >ImageEntryPoint)(DriverEntry->SmmImageHandle, gST);
>
> + PERF_START_IMAGE_END (DriverEntry-
> >SmmImageHandle);
>
> if (EFI_ERROR(Status)){
>
> DEBUG ((
>
> DEBUG_ERROR,
>
> @@ -910,20 +863,6 @@ SmmDispatcher (
> ));
>
> UnregisterSmramProfileImage (DriverEntry,
> TRUE);
>
> SmmFreePages(DriverEntry->ImageBuffer,
> DriverEntry->NumberOfPage);
>
> - //
>
> - // Uninstall LoadedImage
>
> - //
>
> - Status = gBS->UninstallProtocolInterface (
>
> - DriverEntry->ImageHandle,
>
> - &gEfiLoadedImageProtocolGuid,
>
> - DriverEntry->LoadedImage
>
> - );
>
> - if (!EFI_ERROR (Status)) {
>
> - if (DriverEntry->LoadedImage->FilePath !=
> NULL) {
>
> - gBS->FreePool (DriverEntry->LoadedImage-
> >FilePath);
>
> - }
>
> - gBS->FreePool (DriverEntry->LoadedImage);
>
> - }
>
> Status = SmmUninstallProtocolInterface (
>
> DriverEntry->SmmImageHandle,
>
> &gEfiLoadedImageProtocolGuid,
>
> @@ -939,8 +878,8 @@ SmmDispatcher (
> REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
>
> EFI_PROGRESS_CODE,
>
> EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END,
>
> - &DriverEntry->ImageHandle,
>
> - sizeof (DriverEntry->ImageHandle)
>
> + &DriverEntry->SmmImageHandle,
>
> + sizeof (DriverEntry->SmmImageHandle)
>
> );
>
>
>
> if (!PreviousSmmEntryPointRegistered &&
> gSmmCorePrivate->SmmEntryPointRegistered) {
>
> @@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler (
> //
>
> // If this is the SMM core fill in it's
> DevicePath & DeviceHandle
>
> //
>
> - if (mSmmCoreLoadedImage->FilePath == NULL)
> {
>
> - //
>
> - // Maybe one special FV contains only one
> SMM_CORE module, so its device path must
>
> - // be initialized completely.
>
> - //
>
> - EfiInitializeFwVolDevicepathNode
> (&mFvDevicePath.File, &NameGuid);
>
> - SetDevicePathEndNode
> (&mFvDevicePath.End);
>
> -
>
> - //
>
> - // Make an EfiBootServicesData buffer
> copy of FilePath
>
> - //
>
> - Status = gBS->AllocatePool (
>
> - EfiBootServicesData,
>
> - GetDevicePathSize
> ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath),
>
> - (VOID
> **)&mSmmCoreLoadedImage->FilePath
>
> - );
>
> - ASSERT_EFI_ERROR (Status);
>
> - CopyMem (mSmmCoreLoadedImage->FilePath,
> &mFvDevicePath, GetDevicePathSize
> ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath));
>
> -
>
> - mSmmCoreLoadedImage->DeviceHandle =
> FvHandle;
>
> - }
>
> if (mSmmCoreDriverEntry-
> >SmmLoadedImage.FilePath == NULL) {
>
> //
>
> // Maybe one special FV contains only one
> SMM_CORE module, so its device path must
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index cfa9922cbd..c1b18b047e 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR
> *mFullSmramRanges;
>
>
> EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry;
>
>
>
> -EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage;
>
> -
>
> /**
>
> Place holder function until all the SMM System Table
> Service are available.
>
>
>
> @@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage (
> )
>
> {
>
> EFI_STATUS Status;
>
> - EFI_HANDLE Handle;
>
> -
>
> - //
>
> - // Allocate a Loaded Image Protocol in
> EfiBootServicesData
>
> - //
>
> - Status = gBS->AllocatePool (EfiBootServicesData,
> sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID
> **)&mSmmCoreLoadedImage);
>
> - ASSERT_EFI_ERROR (Status);
>
> -
>
> - ZeroMem (mSmmCoreLoadedImage, sizeof
> (EFI_LOADED_IMAGE_PROTOCOL));
>
> - //
>
> - // Fill in the remaining fields of the Loaded Image
> Protocol instance.
>
> - // Note: ImageBase is an SMRAM address that can not
> be accessed outside of SMRAM if SMRAM window is closed.
>
> - //
>
> - mSmmCoreLoadedImage->Revision =
> EFI_LOADED_IMAGE_PROTOCOL_REVISION;
>
> - mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate-
> >SmmIplImageHandle;
>
> - mSmmCoreLoadedImage->SystemTable = gST;
>
> -
>
> - mSmmCoreLoadedImage->ImageBase = (VOID
> *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase;
>
> - mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate-
> >PiSmmCoreImageSize;
>
> - mSmmCoreLoadedImage->ImageCodeType =
> EfiRuntimeServicesCode;
>
> - mSmmCoreLoadedImage->ImageDataType =
> EfiRuntimeServicesData;
>
> -
>
> - //
>
> - // Create a new image handle in the UEFI handle
> database for the SMM Driver
>
> - //
>
> - Handle = NULL;
>
> - Status = gBS->InstallMultipleProtocolInterfaces (
>
> - &Handle,
>
> - &gEfiLoadedImageProtocolGuid,
> mSmmCoreLoadedImage,
>
> - NULL
>
> - );
>
> - ASSERT_EFI_ERROR (Status);
>
>
>
> //
>
> // Allocate a Loaded Image Protocol in SMM
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> index 50a7fc0000..3bbd1e1603 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> @@ -122,8 +122,8 @@ typedef struct {
> BOOLEAN Initialized;
>
> BOOLEAN DepexProtocolError;
>
>
>
> - EFI_HANDLE ImageHandle;
>
> - EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
>
> + EFI_HANDLE
> ImageHandle_Reserved1;
>
> + EFI_LOADED_IMAGE_PROTOCOL
> *LoadedImage_Reserved2;
>
> //
>
> // Image EntryPoint in SMRAM
>
> //
>
> --
> 2.25.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem()
2020-06-16 22:38 ` [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Michael D Kinney
@ 2020-06-17 5:25 ` Zhiguang Liu
0 siblings, 0 replies; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-17 5:25 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zeng, Star, Gao, Liming, Wang, Jian J, Wu, Hao A
Hi Mike,
This code change is to avoid expose the SMM data and using CopyMem() to copy the whole structure will Copy the "next" filed which contain SMM address.
But the Guid is not private information and I think it is ok to use CopyMem() to copy Guid.
Maybe the title is confusing, I will change the patch title.
Thanks
Zhiguang
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 17, 2020 6:38 AM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers
> being leaked by not using CopyMem()
>
> Zhiguang,
>
> An implementation of CopyGuid() could use CopyMem().
> Does CopyGuid() also need to be avoided?
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > Behalf Of Zhiguang Liu
> > Sent: Tuesday, June 16, 2020 2:05 AM
> > To: devel@edk2.groups.io
> > Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid
> > SMM pointers being leaked by not using CopyMem()
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2002
> >
> > This commit will update the logic in function
> > SmmVariableGetStatistics()
> > so that the pointer fields ('Next' and 'Name') in
> > structure
> > VARIABLE_INFO_ENTRY will not be copied into the SMM
> > communication buffer.
> >
> > Doing so will prevent SMM pointers address from being
> > leaked into non-SMM
> > environment.
> >
> > Please note that newly introduced internal function
> > CopyVarInfoEntry()
> > will not use CopyMem() to copy the whole
> > VARIABLE_INFO_ENTRY structure and
> > then zero out the 'Next' and 'Name' fields. This is for
> > preventing race
> > conditions where the pointers value might still be read.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > | 33 +++++++++++++++++++++++++++++++--
> > 1 file changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> > .c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> > .c
> > index caca5c3241..74e756bc00 100644
> > ---
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> > .c
> > +++
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
> > .c
> > @@ -315,6 +315,35 @@ GetFvbCountAndBuffer (
> > }
> >
> >
> >
> >
> >
> > +/**
> >
> > + Copy only the meaningful fields of the variable
> > statistics information from
> >
> > + source buffer to the destination buffer. Other fields
> > are filled with zero.
> >
> > +
> >
> > + @param[out] DstInfoEntry A pointer to the buffer
> > of destination variable
> >
> > + information entry.
> >
> > + @param[in] SrcInfoEntry A pointer to the buffer
> > of source variable
> >
> > + information entry.
> >
> > +
> >
> > +**/
> >
> > +static
> >
> > +VOID
> >
> > +CopyVarInfoEntry (
> >
> > + OUT VARIABLE_INFO_ENTRY *DstInfoEntry,
> >
> > + IN VARIABLE_INFO_ENTRY *SrcInfoEntry
> >
> > + )
> >
> > +{
> >
> > + DstInfoEntry->Next = NULL;
> >
> > + DstInfoEntry->Name = NULL;
> >
> > +
> >
> > + CopyGuid (&DstInfoEntry->VendorGuid, &SrcInfoEntry-
> > >VendorGuid);
> >
> > + DstInfoEntry->Attributes = SrcInfoEntry->Attributes;
> >
> > + DstInfoEntry->ReadCount = SrcInfoEntry->ReadCount;
> >
> > + DstInfoEntry->WriteCount = SrcInfoEntry->WriteCount;
> >
> > + DstInfoEntry->DeleteCount = SrcInfoEntry-
> > >DeleteCount;
> >
> > + DstInfoEntry->CacheCount = SrcInfoEntry->CacheCount;
> >
> > + DstInfoEntry->Volatile = SrcInfoEntry->Volatile;
> >
> > +}
> >
> > +
> >
> > /**
> >
> > Get the variable statistics information from the
> > information buffer pointed by gVariableInfo.
> >
> >
> >
> > @@ -377,7 +406,7 @@ SmmVariableGetStatistics (
> > *InfoSize = StatisticsInfoSize;
> >
> > return EFI_BUFFER_TOO_SMALL;
> >
> > }
> >
> > - CopyMem (InfoEntry, VariableInfo, sizeof
> > (VARIABLE_INFO_ENTRY));
> >
> > + CopyVarInfoEntry (InfoEntry, VariableInfo);
> >
> > CopyMem (InfoName, VariableInfo->Name, NameSize);
> >
> > *InfoSize = StatisticsInfoSize;
> >
> > return EFI_SUCCESS;
> >
> > @@ -417,7 +446,7 @@ SmmVariableGetStatistics (
> > return EFI_BUFFER_TOO_SMALL;
> >
> > }
> >
> >
> >
> > - CopyMem (InfoEntry, VariableInfo, sizeof
> > (VARIABLE_INFO_ENTRY));
> >
> > + CopyVarInfoEntry (InfoEntry, VariableInfo);
> >
> > CopyMem (InfoName, VariableInfo->Name, NameSize);
> >
> > *InfoSize = StatisticsInfoSize;
> >
> >
> >
> > --
> > 2.25.1.windows.1
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this
> > group.
> >
> > View/Reply Online (#61324):
> > https://edk2.groups.io/g/devel/message/61324
> > Mute This Topic: https://groups.io/mt/74912557/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
2020-06-16 15:06 ` [edk2-devel] " Laszlo Ersek
@ 2020-06-17 5:30 ` Zhiguang Liu
2020-06-17 12:53 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-17 5:30 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray, Kumar, Rahul1
Hi Laszlo,
Thanks for the comments, I will take the first one.
But I can't find service to unregister protocol notify in EFI_SMM_SYSTEM_TABLE2.
Do you now how the unregister it in SMM driver?
Thanks
Zhiguang
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, June 16, 2020 11:07 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul1 <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH 5/5] UefiCpuPkg: Uninstall
> EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
>
> On 06/16/20 11:04, Zhiguang Liu wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
> > To avoid leaking information from SMM, uninstall
> > EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 37
> +++++++++++++++++++++++++++++++++++++
> > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > index db68e1316e..a1b209e125 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> > @@ -520,6 +520,33 @@ SmmReadyToLockEventNotify (
> > return EFI_SUCCESS;
> > }
> >
> > +/**
> > + SMM End of Dxe event notification handler.
> > +
> > + To avoid leaking information from SMM, uninstall
> > + EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
> > +
> > + @param[in] Protocol Points to the protocol's unique identifier.
> > + @param[in] Interface Points to the interface instance.
> > + @param[in] Handle The handle on which the interface was installed.
> > +
> > + @retval EFI_SUCCESS Notification handler runs successfully.
> > + **/
> > +EFI_STATUS
> > +EFIAPI
> > +SmmEndOfDxeNotify (
> > + IN CONST EFI_GUID *Protocol,
> > + IN VOID *Interface,
> > + IN EFI_HANDLE Handle
> > + )
> > +{
> > + gBS->UninstallProtocolInterface (
> > + gSmmCpuPrivate->SmmCpuHandle,
> > + &gEfiSmmConfigurationProtocolGuid, &gSmmCpuPrivate-
> >SmmConfiguration
> > + );
> > + return EFI_SUCCESS;
> > +}
>
> (1) I suggest setting "gSmmCpuPrivate->SmmCpuHandle" to NULL here.
>
> (2) I also suggest de-registering the gEfiSmmEndOfDxeProtocolGuid
> notification.
>
> Thanks
> Laszlo
>
> > +
> > /**
> > The module Entry Point of the CPU SMM driver.
> >
> > @@ -1038,6 +1065,16 @@ PiCpuSmmEntry (
> > );
> > ASSERT_EFI_ERROR (Status);
> >
> > + //
> > + // register SMM End of Dxe notification // Status =
> > + gSmst->SmmRegisterProtocolNotify (
> > + &gEfiSmmEndOfDxeProtocolGuid,
> > + SmmEndOfDxeNotify,
> > + &Registration
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > +
> > //
> > // Initialize SMM Profile feature
> > //
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> > index 76b1462996..bb994814d6 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> > @@ -105,6 +105,7 @@
> > gEfiSmmConfigurationProtocolGuid ## PRODUCES
> > gEfiSmmCpuProtocolGuid ## PRODUCES
> > gEfiSmmReadyToLockProtocolGuid ## NOTIFY
> > + gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
> > gEfiSmmCpuServiceProtocolGuid ## PRODUCES
> > gEdkiiSmmMemoryAttributeProtocolGuid ## PRODUCES
> > gEfiMmMpProtocolGuid ## PRODUCES
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe.
2020-06-17 5:30 ` Zhiguang Liu
@ 2020-06-17 12:53 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2020-06-17 12:53 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray, Kumar, Rahul1
On 06/17/20 07:30, Liu, Zhiguang wrote:
> Hi Laszlo,
> Thanks for the comments, I will take the first one.
> But I can't find service to unregister protocol notify in EFI_SMM_SYSTEM_TABLE2.
> Do you now how the unregister it in SMM driver?
It's a bit hidden, but it is explained by the PI spec, under
MmRegisterProtocolNotify():
If Function == NULL and Registration is an existing registration,
then the callback is unhooked. *Protocol must be validated it with
*Registration. If Registration is not found then EFI_NOT_FOUND is
returned.
So where you do
gSmst->SmmRegisterProtocolNotify (
&gEfiSmmEndOfDxeProtocolGuid,
SmmEndOfDxeNotify,
&Registration
);
in PiCpuSmmEntry(), you'd have to save the registration value into a
global variable (which would live in SMRAM):
VOID *mEndOfDxeRegistration;
gSmst->SmmRegisterProtocolNotify (
&gEfiSmmEndOfDxeProtocolGuid,
SmmEndOfDxeNotify,
&mEndOfDxeRegistration // <----- here
);
Then in SmmEndOfDxeNotify(), you could call
Status = gSmst->SmmRegisterProtocolNotify (
&gEfiSmmEndOfDxeProtocolGuid,
NULL,
&mEndOfDxeRegistration
);
ASSERT_EFI_ERROR (Status);
You can see examples in:
- MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
- MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c
- MdePkg/Library/SmmIoLib/SmmIoLib.c
- MdePkg/Library/SmmMemLib/SmmMemLib.c
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM.
2020-06-16 22:39 ` Michael D Kinney
@ 2020-06-23 6:12 ` Zhiguang Liu
0 siblings, 0 replies; 17+ messages in thread
From: Zhiguang Liu @ 2020-06-23 6:12 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Tian, Feng, Zeng, Star, Laszlo Ersek, Yao, Jiewen
Hi Mike,
I did some investigation. The driver does affect drivers or libraries that use EDI_LOADED_IMAGE_PROTOCOL or Image Handle.
In EDK2 repo, I find the following modules should be modified because of this code change. And after proper modification, the module will remain the same functionally.
MdePkg\Library\UefiDriverEntryPoint\UefiDriverEntryPoint.inf
MdePkg\Library\DxeServicesLib\DxeServicesLib.inf
MdeModulePkg\Library\SmmCorePerformanceLib\SmmCorePerformanceLib.inf
Do you know other modules that depend on the EDI_LOADED_IMAGE_PROTOCOL struct?
Because this code change also has effect on internal platform, I will do more research and send other patches in this patch set for review first.
Thanks
Zhiguang
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 17, 2020 6:40 AM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI
> LOADED_IMAGE from SMM.
>
> Zhiguang,
>
> There are some source level debug solutions that depend on the
> EDI_LOADED_IMAGE_PROTOCOL structure. Does this change reduce the
> source level debug capabilities for SMM drivers under development?
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: Liu, Zhiguang <zhiguang.liu@intel.com>
> > Sent: Tuesday, June 16, 2020 2:05 AM
> > To: devel@edk2.groups.io
> > Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star
> > <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI
> LOADED_IMAGE
> > from SMM.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
> >
> > This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE database.
> > The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to SMM
> database.
> >
> > Exposing LOADED_IMAGE_PROTOCOL may cause SMM information
> leakage to
> > non-SMM component.
> >
> > Cc: Feng Tian <feng.tian@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> > MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104
> > +++++++++++---------------------------------------------
> > ------------------------------------------------
> > MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 --------
> > --------------------------
> > MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++--
> > 3 files changed, 13 insertions(+), 129 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> > b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> > index 76ee9e0b89..2be2866234 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c
> > @@ -316,7 +316,7 @@ SmmLoadImage (
> > EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv;
> >
> > PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
> >
> >
> >
> > - PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle);
> >
> > + PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle);
> >
> >
> >
> > Buffer = NULL;
> >
> > Size = 0;
> >
> > @@ -546,51 +546,14 @@ SmmLoadImage (
> > DriverEntry->ImageBuffer = DstBuffer;
> >
> > DriverEntry->NumberOfPage = PageCount;
> >
> >
> >
> > - //
> >
> > - // Allocate a Loaded Image Protocol in EfiBootServicesData
> >
> > - //
> >
> > - Status = gBS->AllocatePool (EfiBootServicesData, sizeof
> > (EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&DriverEntry-
> >LoadedImage);
> >
> > - if (EFI_ERROR (Status)) {
> >
> > - if (Buffer != NULL) {
> >
> > - gBS->FreePool (Buffer);
> >
> > - }
> >
> > - SmmFreePages (DstBuffer, PageCount);
> >
> > - return Status;
> >
> > - }
> >
> > -
> >
> > - ZeroMem (DriverEntry->LoadedImage, sizeof
> > (EFI_LOADED_IMAGE_PROTOCOL));
> >
> > //
> >
> > // Fill in the remaining fields of the Loaded Image Protocol
> > instance.
> >
> > - // Note: ImageBase is an SMRAM address that can not be accessed
> > outside of SMRAM if SMRAM window is closed.
> >
> > //
> >
> > - DriverEntry->LoadedImage->Revision =
> > EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> >
> > - DriverEntry->LoadedImage->ParentHandle =
> > gSmmCorePrivate->SmmIplImageHandle;
> >
> > - DriverEntry->LoadedImage->SystemTable = gST;
> >
> > - DriverEntry->LoadedImage->DeviceHandle = DeviceHandle;
> >
> > -
> >
> > DriverEntry->SmmLoadedImage.Revision =
> > EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> >
> > DriverEntry->SmmLoadedImage.ParentHandle =
> > gSmmCorePrivate->SmmIplImageHandle;
> >
> > DriverEntry->SmmLoadedImage.SystemTable = gST;
> >
> > DriverEntry->SmmLoadedImage.DeviceHandle = DeviceHandle;
> >
> >
> >
> > - //
> >
> > - // Make an EfiBootServicesData buffer copy of FilePath
> >
> > - //
> >
> > - Status = gBS->AllocatePool (EfiBootServicesData, GetDevicePathSize
> > (FilePath), (VOID **)&DriverEntry-
> > >LoadedImage->FilePath);
> >
> > - if (EFI_ERROR (Status)) {
> >
> > - if (Buffer != NULL) {
> >
> > - gBS->FreePool (Buffer);
> >
> > - }
> >
> > - SmmFreePages (DstBuffer, PageCount);
> >
> > - return Status;
> >
> > - }
> >
> > - CopyMem (DriverEntry->LoadedImage->FilePath,
> > FilePath, GetDevicePathSize (FilePath));
> >
> > -
> >
> > - DriverEntry->LoadedImage->ImageBase = (VOID
> > *)(UINTN) ImageContext.ImageAddress;
> >
> > - DriverEntry->LoadedImage->ImageSize =
> > ImageContext.ImageSize;
> >
> > - DriverEntry->LoadedImage->ImageCodeType = EfiRuntimeServicesCode;
> >
> > - DriverEntry->LoadedImage->ImageDataType = EfiRuntimeServicesData;
> >
> > -
> >
> > //
> >
> > // Make a buffer copy of FilePath
> >
> > //
> >
> > @@ -599,7 +562,6 @@ SmmLoadImage (
> > if (Buffer != NULL) {
> >
> > gBS->FreePool (Buffer);
> >
> > }
> >
> > - gBS->FreePool (DriverEntry->LoadedImage->FilePath);
> >
> > SmmFreePages (DstBuffer, PageCount);
> >
> > return Status;
> >
> > }
> >
> > @@ -610,16 +572,6 @@ SmmLoadImage (
> > DriverEntry->SmmLoadedImage.ImageCodeType =
> EfiRuntimeServicesCode;
> >
> > DriverEntry->SmmLoadedImage.ImageDataType =
> EfiRuntimeServicesData;
> >
> >
> >
> > - //
> >
> > - // Create a new image handle in the UEFI handle database for the
> > SMM Driver
> >
> > - //
> >
> > - DriverEntry->ImageHandle = NULL;
> >
> > - Status = gBS->InstallMultipleProtocolInterfaces (
> >
> > - &DriverEntry->ImageHandle,
> >
> > - &gEfiLoadedImageProtocolGuid,
> > DriverEntry->LoadedImage,
> >
> > - NULL
> >
> > - );
> >
> > -
> >
> > //
> >
> > // Create a new image handle in the SMM handle database for the SMM
> > Driver
> >
> > //
> >
> > @@ -631,7 +583,7 @@ SmmLoadImage (
> > &DriverEntry->SmmLoadedImage
> >
> > );
> >
> >
> >
> > - PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle);
> >
> > + PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle);
> >
> >
> >
> > //
> >
> > // Print the load address and the PDB file name if it is available
> >
> > @@ -856,7 +808,7 @@ SmmDispatcher (
> > // Untrused to Scheduled it would have already been loaded so
> > we may need to
> >
> > // skip the LoadImage
> >
> > //
> >
> > - if (DriverEntry->ImageHandle == NULL) {
> >
> > + if (DriverEntry->SmmImageHandle == NULL) {
> >
> > Status = SmmLoadImage (DriverEntry);
> >
> >
> >
> > //
> >
> > @@ -885,8 +837,8 @@ SmmDispatcher (
> > REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
> >
> > EFI_PROGRESS_CODE,
> >
> > EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN,
> >
> > - &DriverEntry->ImageHandle,
> >
> > - sizeof (DriverEntry->ImageHandle)
> >
> > + &DriverEntry->SmmImageHandle,
> >
> > + sizeof (DriverEntry->SmmImageHandle)
> >
> > );
> >
> >
> >
> > //
> >
> > @@ -898,9 +850,10 @@ SmmDispatcher (
> > // For each SMM driver, pass NULL as ImageHandle
> >
> > //
> >
> > RegisterSmramProfileImage (DriverEntry, TRUE);
> >
> > - PERF_START_IMAGE_BEGIN (DriverEntry-
> > >ImageHandle);
> >
> > - Status =
> > ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry-
> > >ImageEntryPoint)(DriverEntry->ImageHandle, gST);
> >
> > - PERF_START_IMAGE_END (DriverEntry->ImageHandle);
> >
> > + DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n",
> > DriverEntry->ImageEntryPoint));
> >
> > + PERF_START_IMAGE_BEGIN (DriverEntry-
> > >SmmImageHandle);
> >
> > + Status =
> > ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry-
> > >ImageEntryPoint)(DriverEntry->SmmImageHandle, gST);
> >
> > + PERF_START_IMAGE_END (DriverEntry-
> > >SmmImageHandle);
> >
> > if (EFI_ERROR(Status)){
> >
> > DEBUG ((
> >
> > DEBUG_ERROR,
> >
> > @@ -910,20 +863,6 @@ SmmDispatcher (
> > ));
> >
> > UnregisterSmramProfileImage (DriverEntry, TRUE);
> >
> > SmmFreePages(DriverEntry->ImageBuffer,
> > DriverEntry->NumberOfPage);
> >
> > - //
> >
> > - // Uninstall LoadedImage
> >
> > - //
> >
> > - Status = gBS->UninstallProtocolInterface (
> >
> > - DriverEntry->ImageHandle,
> >
> > - &gEfiLoadedImageProtocolGuid,
> >
> > - DriverEntry->LoadedImage
> >
> > - );
> >
> > - if (!EFI_ERROR (Status)) {
> >
> > - if (DriverEntry->LoadedImage->FilePath !=
> > NULL) {
> >
> > - gBS->FreePool (DriverEntry->LoadedImage-
> > >FilePath);
> >
> > - }
> >
> > - gBS->FreePool (DriverEntry->LoadedImage);
> >
> > - }
> >
> > Status = SmmUninstallProtocolInterface (
> >
> > DriverEntry->SmmImageHandle,
> >
> > &gEfiLoadedImageProtocolGuid,
> >
> > @@ -939,8 +878,8 @@ SmmDispatcher (
> > REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
> >
> > EFI_PROGRESS_CODE,
> >
> > EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END,
> >
> > - &DriverEntry->ImageHandle,
> >
> > - sizeof (DriverEntry->ImageHandle)
> >
> > + &DriverEntry->SmmImageHandle,
> >
> > + sizeof (DriverEntry->SmmImageHandle)
> >
> > );
> >
> >
> >
> > if (!PreviousSmmEntryPointRegistered &&
> > gSmmCorePrivate->SmmEntryPointRegistered) {
> >
> > @@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler (
> > //
> >
> > // If this is the SMM core fill in it's DevicePath &
> > DeviceHandle
> >
> > //
> >
> > - if (mSmmCoreLoadedImage->FilePath == NULL)
> > {
> >
> > - //
> >
> > - // Maybe one special FV contains only one
> > SMM_CORE module, so its device path must
> >
> > - // be initialized completely.
> >
> > - //
> >
> > - EfiInitializeFwVolDevicepathNode
> > (&mFvDevicePath.File, &NameGuid);
> >
> > - SetDevicePathEndNode
> > (&mFvDevicePath.End);
> >
> > -
> >
> > - //
> >
> > - // Make an EfiBootServicesData buffer
> > copy of FilePath
> >
> > - //
> >
> > - Status = gBS->AllocatePool (
> >
> > - EfiBootServicesData,
> >
> > - GetDevicePathSize
> > ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath),
> >
> > - (VOID
> > **)&mSmmCoreLoadedImage->FilePath
> >
> > - );
> >
> > - ASSERT_EFI_ERROR (Status);
> >
> > - CopyMem (mSmmCoreLoadedImage->FilePath,
> > &mFvDevicePath, GetDevicePathSize
> > ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath));
> >
> > -
> >
> > - mSmmCoreLoadedImage->DeviceHandle =
> > FvHandle;
> >
> > - }
> >
> > if (mSmmCoreDriverEntry-
> > >SmmLoadedImage.FilePath == NULL) {
> >
> > //
> >
> > // Maybe one special FV contains only one SMM_CORE
> > module, so its device path must
> >
> > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> > b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> > index cfa9922cbd..c1b18b047e 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> > @@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR *mFullSmramRanges;
> >
> >
> > EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry;
> >
> >
> >
> > -EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage;
> >
> > -
> >
> > /**
> >
> > Place holder function until all the SMM System Table Service are
> > available.
> >
> >
> >
> > @@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage (
> > )
> >
> > {
> >
> > EFI_STATUS Status;
> >
> > - EFI_HANDLE Handle;
> >
> > -
> >
> > - //
> >
> > - // Allocate a Loaded Image Protocol in EfiBootServicesData
> >
> > - //
> >
> > - Status = gBS->AllocatePool (EfiBootServicesData,
> > sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID
> **)&mSmmCoreLoadedImage);
> >
> > - ASSERT_EFI_ERROR (Status);
> >
> > -
> >
> > - ZeroMem (mSmmCoreLoadedImage, sizeof
> (EFI_LOADED_IMAGE_PROTOCOL));
> >
> > - //
> >
> > - // Fill in the remaining fields of the Loaded Image Protocol
> > instance.
> >
> > - // Note: ImageBase is an SMRAM address that can not be accessed
> > outside of SMRAM if SMRAM window is closed.
> >
> > - //
> >
> > - mSmmCoreLoadedImage->Revision =
> > EFI_LOADED_IMAGE_PROTOCOL_REVISION;
> >
> > - mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate-
> > >SmmIplImageHandle;
> >
> > - mSmmCoreLoadedImage->SystemTable = gST;
> >
> > -
> >
> > - mSmmCoreLoadedImage->ImageBase = (VOID
> > *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase;
> >
> > - mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate-
> > >PiSmmCoreImageSize;
> >
> > - mSmmCoreLoadedImage->ImageCodeType = EfiRuntimeServicesCode;
> >
> > - mSmmCoreLoadedImage->ImageDataType = EfiRuntimeServicesData;
> >
> > -
> >
> > - //
> >
> > - // Create a new image handle in the UEFI handle database for the
> > SMM Driver
> >
> > - //
> >
> > - Handle = NULL;
> >
> > - Status = gBS->InstallMultipleProtocolInterfaces (
> >
> > - &Handle,
> >
> > - &gEfiLoadedImageProtocolGuid,
> > mSmmCoreLoadedImage,
> >
> > - NULL
> >
> > - );
> >
> > - ASSERT_EFI_ERROR (Status);
> >
> >
> >
> > //
> >
> > // Allocate a Loaded Image Protocol in SMM
> >
> > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> > b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> > index 50a7fc0000..3bbd1e1603 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> > @@ -122,8 +122,8 @@ typedef struct {
> > BOOLEAN Initialized;
> >
> > BOOLEAN DepexProtocolError;
> >
> >
> >
> > - EFI_HANDLE ImageHandle;
> >
> > - EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
> >
> > + EFI_HANDLE
> > ImageHandle_Reserved1;
> >
> > + EFI_LOADED_IMAGE_PROTOCOL
> > *LoadedImage_Reserved2;
> >
> > //
> >
> > // Image EntryPoint in SMRAM
> >
> > //
> >
> > --
> > 2.25.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries
2020-06-16 9:04 ` [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries Zhiguang Liu
2020-06-16 22:41 ` Michael D Kinney
@ 2020-07-02 13:51 ` Liming Gao
1 sibling, 0 replies; 17+ messages in thread
From: Liming Gao @ 2020-07-02 13:51 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Kinney, Michael D
Reviewed-by: Liming Gao <liming.gao@intel.com>
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, June 16, 2020 5:05 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317
> Remove DXE_SMM_DRIVER support for some libraries because they
> have the risks of leaking data from SMM mode to non-SMM mode.
>
> Cc: Kinney Michael D <michael.d.kinney@intel.com>
> Cc: Gao Liming <liming.gao@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf | 2 +-
> MdePkg/Library/DxeHstiLib/DxeHstiLib.inf | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
> b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
> index 33eeab405f..acbe62e352 100644
> --- a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
> +++ b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf
> @@ -17,7 +17,7 @@
> FILE_GUID = f773469b-e265-4b0c-b0a6-2f971fbfe72b
>
> MODULE_TYPE = DXE_DRIVER
>
> VERSION_STRING = 1.0
>
> - LIBRARY_CLASS = ExtractGuidedSectionLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER
> UEFI_APPLICATION UEFI_DRIVER
>
> + LIBRARY_CLASS = ExtractGuidedSectionLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION
> UEFI_DRIVER
>
>
>
> CONSTRUCTOR = DxeExtractGuidedSectionLibConstructor
>
>
>
> diff --git a/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf b/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> index c719481cdd..8c1cb3d0cc 100644
> --- a/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> +++ b/MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
> @@ -14,7 +14,7 @@
> FILE_GUID = 7DE1C620-F587-4116-A36D-40F3467B9A0C
>
> MODULE_TYPE = DXE_DRIVER
>
> VERSION_STRING = 1.0
>
> - LIBRARY_CLASS = HstiLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION
> UEFI_DRIVER
>
> + LIBRARY_CLASS = HstiLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
>
>
>
> [Sources]
>
> HstiAip.c
>
> --
> 2.25.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-07-02 13:51 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-16 9:04 [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Zhiguang Liu
2020-06-16 9:04 ` [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM Zhiguang Liu
2020-06-16 14:45 ` [edk2-devel] " Laszlo Ersek
2020-06-16 22:39 ` Michael D Kinney
2020-06-23 6:12 ` Zhiguang Liu
2020-06-16 22:44 ` Michael D Kinney
2020-06-16 9:04 ` [PATCH 3/5] MdePkg: Remove DXE_SMM_DRIVER support for some libraries Zhiguang Liu
2020-06-16 22:41 ` Michael D Kinney
2020-07-02 13:51 ` Liming Gao
2020-06-16 9:04 ` [PATCH 4/5] SecurityPkg: " Zhiguang Liu
2020-06-16 14:47 ` [edk2-devel] " Laszlo Ersek
2020-06-16 9:04 ` [PATCH 5/5] UefiCpuPkg: Uninstall EFI_SMM_CONFIGURATION_PROTOCOL at end of Dxe Zhiguang Liu
2020-06-16 15:06 ` [edk2-devel] " Laszlo Ersek
2020-06-17 5:30 ` Zhiguang Liu
2020-06-17 12:53 ` Laszlo Ersek
2020-06-16 22:38 ` [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem() Michael D Kinney
2020-06-17 5:25 ` Zhiguang Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox