Re: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM.

["Michael D Kinney" <michael.d.kinney@intel.com>]

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