* 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: [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 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 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