Re: [edk2-devel] [PATCH v5 4/9] UefiCpuPkg/SecMigrationPei: Add initial PEIM (CVE-2019-11098)

["Laszlo Ersek" <lersek@redhat.com>]

On 07/09/20 03:56, Guomin Jiang wrote:
> From: Michael Kubacki <michael.a.kubacki@intel.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
> 
> Adds a PEIM that republishes structures produced in SEC. This
> is done because SEC modules may not be shadowed in some platforms
> due to space constraints or special alignment requirements. The
> SecMigrationPei module locates interfaces that may be published in
> SEC and reinstalls the interface with permanent memory addresses.
> 
> This is important if pre-memory address access is forbidden after
> memory initialization and data such as a PPI descriptor, PPI GUID,
> or PPI inteface reside in pre-memory.
> 
> 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>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                     |   4 +
>  UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
>  UefiCpuPkg/SecCore/SecCore.inf                |   2 +
>  .../SecMigrationPei/SecMigrationPei.inf       |  67 +++
>  UefiCpuPkg/Include/Ppi/RepublishSecPpi.h      |  54 +++
>  UefiCpuPkg/SecCore/SecMain.h                  |   1 +
>  UefiCpuPkg/SecMigrationPei/SecMigrationPei.h  | 154 +++++++
>  UefiCpuPkg/SecCore/SecMain.c                  |  26 +-
>  UefiCpuPkg/SecMigrationPei/SecMigrationPei.c  | 381 ++++++++++++++++++
>  .../SecMigrationPei/SecMigrationPei.uni       |  13 +
>  10 files changed, 701 insertions(+), 2 deletions(-)
>  create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf
>  create mode 100644 UefiCpuPkg/Include/Ppi/RepublishSecPpi.h
>  create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.h
>  create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.c
>  create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.uni

> +/**
> +  This function is the entry point which installs an instance of REPUBLISH_SEC_PPI_PPI.
> +
> +  It install the RepublishSecPpi depent on PcdMigrateTemporaryRamFirmwareVolumes, install
> +  the PPI when the PcdMigrateTemporaryRamFirmwareVolumes enabled.
> +
> +  @param[in]  FileHandle   Pointer to image file handle.
> +  @param[in]  PeiServices  Pointer to PEI Services Table
> +
> +  @retval EFI_ABORTED  Disable evacuate temporary memory feature by disable
> +                       PcdMigrateTemporaryRamFirmwareVolumes.
> +          EFI_SUCCESS  An instance of REPUBLISH_SEC_PPI_PPI was installed successfully.

I think "@retval" should be used in front of every single return value,
that is, EFI_SUCCESS as well.

But, please do not resubmit the series just because of this remark; it's
not important enough.

If you need to post a v6 for another reason, then please consider adding
the "@retval" word here.

My Acked-by stands with or without "@retval".

Thanks!
Laszlo

> +  @retval Others       An error occurred installing and instance of REPUBLISH_SEC_PPI_PPI.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SecMigrationPeiInitialize (
> +  IN EFI_PEI_FILE_HANDLE     FileHandle,
> +  IN CONST EFI_PEI_SERVICES  **PeiServices
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = EFI_ABORTED;
> +
> +  if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) {
> +    Status = PeiServicesInstallPpi (&mEdkiiRepublishSecPpiDescriptor);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  return Status;
> +}