Re: [PATCH v5 5/9] MdeModulePkg/Core: Create Migrated FV Info Hob for calculating hash (CVE-2019-11098)

["Wang, Jian J" <jian.j.wang@intel.com>]

Guomin,

Some typo or syntax issues below.

> -----Original Message-----
> From: Jiang, Guomin <guomin.jiang@intel.com>
> Sent: Thursday, July 09, 2020 9:57 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>; De,
> Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>;
> West, Catharine <catharine.west@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v5 5/9] MdeModulePkg/Core: Create Migrated FV Info Hob for
> calculating hash (CVE-2019-11098)
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
> 
> When we allocate pool to save the rebased PEIMs, the address will change
> randomly, therefore the hash will change and result PCR0 change as well.
> To avoid this, we save the raw PEIMs and use it to calculate hash.
> 
> The MigratedFvInfo HOB will never produce when
> PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD control
> the total feature.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@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: Guomin Jiang <guomin.jiang@intel.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec                 |  3 ++
>  MdeModulePkg/Core/Pei/PeiMain.inf             |  1 +
>  MdeModulePkg/Core/Pei/PeiMain.h               |  1 +
>  MdeModulePkg/Include/Guid/MigratedFvInfo.h    | 22 +++++++++++++++
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 28 +++++++++++++++++++
>  5 files changed, 55 insertions(+)
>  create mode 100644 MdeModulePkg/Include/Guid/MigratedFvInfo.h
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index 16db17d0a873..40197dba862c 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -389,6 +389,9 @@ [Guids]
>    ## GUID indicates the capsule is to store Capsule On Disk file names.
>    gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93, 0x9a,
> 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }
> 
> +  ## Include/Guid/MigratedFvInfo.h
> +  gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6,
> 0xce, 0xfd, 0x17, 0x98, 0x71 } }
> +
>  [Ppis]
>    ## Include/Ppi/AtaController.h
>    gPeiAtaControllerPpiGuid       = { 0xa45e60d1, 0xc719, 0x44aa, { 0xb0, 0x7a,
> 0xaa, 0x77, 0x7f, 0x85, 0x90, 0x6d }}
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf
> b/MdeModulePkg/Core/Pei/PeiMain.inf
> index 5b36d516b3fa..0cf357371a16 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> @@ -77,6 +77,7 @@ [Guids]
>    ## CONSUMES   ## GUID      # Used to compare with FV's file system GUID and
> get the FV's file system format
>    gEfiFirmwareFileSystem3Guid
>    gStatusCodeCallbackGuid
> +  gEdkiiMigratedFvInfoGuid                      ## SOMETIMES_PRODUCES     ## HOB
> 
>  [Ppis]
>    gEfiPeiStatusCodePpiGuid                      ## SOMETIMES_CONSUMES #
> PeiReportStatusService is not ready if this PPI doesn't exist
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.h
> b/MdeModulePkg/Core/Pei/PeiMain.h
> index b0101dba5e30..cbf74d5b9d9a 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.h
> +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> @@ -44,6 +44,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Guid/FirmwareFileSystem2.h>
>  #include <Guid/FirmwareFileSystem3.h>
>  #include <Guid/AprioriFileName.h>
> +#include <Guid/MigratedFvInfo.h>
> 
>  ///
>  /// It is an FFS type extension used for PeiFindFileEx. It indicates current
> diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> new file mode 100644
> index 000000000000..061c17ed0e48
> --- /dev/null
> +++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
> @@ -0,0 +1,22 @@
> +/** @file
> +  Migrated FV information
> +
> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
> +#define __EDKII_MIGRATED_FV_INFO_GUID_H__
> +
> +typedef struct {
> +  UINT32           FvOrgBase;  // original FV address
> +  UINT32           FvNewBase;  // new FV address
> +  UINT32           FvDataBase; // original FV data
> +  UINT32           FvLength;   // Fv Length
> +} EDKII_MIGRATED_FV_INFO;
> +
> +extern EFI_GUID gEdkiiMigratedFvInfoGuid;
> +
> +#endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
> +
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index ef88b3423376..f654cea15c59 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1223,10 +1223,12 @@ EvacuateTempRam (
>    EFI_FIRMWARE_VOLUME_HEADER    *FvHeader;
>    EFI_FIRMWARE_VOLUME_HEADER    *ChildFvHeader;
>    EFI_FIRMWARE_VOLUME_HEADER    *MigratedFvHeader;
> +  EFI_FIRMWARE_VOLUME_HEADER    *RawDataFvHeader;
>    EFI_FIRMWARE_VOLUME_HEADER    *MigratedChildFvHeader;
> 
>    PEI_CORE_FV_HANDLE            PeiCoreFvHandle;
>    EFI_PEI_CORE_FV_LOCATION_PPI  *PeiCoreFvLocationPpi;
> +  EDKII_MIGRATED_FV_INFO        MigratedFvInfo;
> 
>    ASSERT (Private->PeiMemoryInstalled);
> 
> @@ -1263,6 +1265,9 @@ EvacuateTempRam (
>          (((EFI_PHYSICAL_ADDRESS)(UINTN) FvHeader + (FvHeader->FvLength - 1)) <
> Private->FreePhysicalMemoryTop)
>          )
>        ) {
> +      //
> +      // Allocate page to save the rebased PEIMs, the PEIMs will get control later

Do you mean 'control' -> 'dispatched'?

> +      //
>        Status =  PeiServicesAllocatePages (
>                    EfiBootServicesCode,
>                    EFI_SIZE_TO_PAGES ((UINTN) FvHeader->FvLength),
> @@ -1270,6 +1275,17 @@ EvacuateTempRam (
>                    );
>        ASSERT_EFI_ERROR (Status);
> 
> +      //
> +      // Allocate pool to save the raw PEIMs, it used to keep consistent context
> across
> +      // multiple boot and PCR0 will keep same no matter if allocate random page
> address.

Consider to revise the statement:
'no matter if ...' -> 'no matter if the address of allocated page is changed'

> +      //
> +      Status =  PeiServicesAllocatePages (
> +                  EfiBootServicesCode,
> +                  EFI_SIZE_TO_PAGES ((UINTN) FvHeader->FvLength),
> +                  (EFI_PHYSICAL_ADDRESS *) &RawDataFvHeader
> +                  );
> +      ASSERT_EFI_ERROR (Status);
> +
>        DEBUG ((
>          DEBUG_VERBOSE,
>          "  Migrating FV[%d] from 0x%08X to 0x%08X\n",
> @@ -1278,7 +1294,19 @@ EvacuateTempRam (
>          (UINTN) MigratedFvHeader
>          ));
> 
> +      //
> +      // Copy the context to the rebased pages and raw pages, and create hob to
> save the
> +      // information. the MigratedFvInfo HOB will never produce when

'the' -> 'The'
'produce' -> 'be produced'

Regards,
Jian

> +      // PcdMigrateTemporaryRamFirmwareVolumes is FALSE, because the PCD
> control the
> +      // feature.
> +      //
>        CopyMem (MigratedFvHeader, FvHeader, (UINTN) FvHeader->FvLength);
> +      CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN) FvHeader-
> >FvLength);
> +      MigratedFvInfo.FvOrgBase  = (UINT32) (UINTN) FvHeader;
> +      MigratedFvInfo.FvNewBase  = (UINT32) (UINTN) MigratedFvHeader;
> +      MigratedFvInfo.FvDataBase = (UINT32) (UINTN) RawDataFvHeader;
> +      MigratedFvInfo.FvLength   = (UINT32) (UINTN) FvHeader->FvLength;
> +      BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, sizeof
> (MigratedFvInfo));
> 
>        //
>        // Migrate any children for this FV now
> --
> 2.25.1.windows.1