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.t...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Jiewen Yao <jiewen....@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang....@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.d...@intel.com>
Hao A Wu <hao.a...@intel.com>
Jian J Wang <jian.j.w...@intel.com>
Ray Ni <ray...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61337): https://edk2.groups.io/g/devel/message/61337
Mute This Topic: https://groups.io/mt/74912558/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
