Re: [edk2-devel][Patch v2 3/7] MdeModulePkg: Add CapsuleOnDiskLoadPei PEIM.

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, June 19, 2019 5:00 PM
> To: devel@edk2.groups.io; Xu, Wei6; Wu, Hao A
> Cc: Wang, Jian J; Zhang, Chao B
> Subject: RE: [edk2-devel][Patch v2 3/7] MdeModulePkg: Add
> CapsuleOnDiskLoadPei PEIM.
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Xu,
> > Wei6
> > Sent: Wednesday, June 19, 2019 4:41 PM
> > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>
> > Subject: Re: [edk2-devel][Patch v2 3/7] MdeModulePkg: Add
> > CapsuleOnDiskLoadPei PEIM.
> >
> > > > +  ASSERT_EFI_ERROR (Status);
> > > > +
> > > > +  FileNameSize = PcdGetSize (PcdCoDRelocationFileName);  Status =
> > > > + PcdSetPtrS (PcdRecoveryFileName, &FileNameSize, (VOID *)
> > > > PcdGetPtr(PcdCoDRelocationFileName));
> > >
> > >
> > > Buffer for 'PcdRecoveryFileName' may not be big enough to hold the
> > > content in 'PcdCoDRelocationFileName'.
> > >
> > > I think there might be a chance for the above PcdSetPtrS() call to fail.
> > >
> >
> >
> > Thanks a lot for the comments.
> > Yes, 'PcdRecoveryFileName' should be larger than
> > 'PcdCoDRelocationFileName'.
> > I think no need to update the code, since these two PCDs are fixed during
> > build time.
> > I will update the description of 'PcdCoDRelocationFileName' to mention: it
> > must be smaller than 'PcdRecoveryFileName', otherwise failure may occur.
> 
> But your code doesn't check the status of PcdSetPtrS().


Please help to add check to the return status of PcdSetPtrS() and also add
description comments for 'PcdCoDRelocationFileName' to mention its impact
to 'PcdRecoveryFileName' together with the limitation.

Best Regards,
Hao Wu


> 
> >
> > Do you have comments about it?
> > Thanks again.
> >
> >
> > BR,
> > Wei
> >
> > > -----Original Message-----
> > > From: Wu, Hao A
> > > Sent: Wednesday, June 12, 2019 3:49 PM
> > > To: devel@edk2.groups.io; Xu, Wei6 <wei6.xu@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>
> > > Subject: RE: [edk2-devel][Patch v2 3/7] MdeModulePkg: Add
> > > CapsuleOnDiskLoadPei PEIM.
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > > Of Xu,
> > > > Wei6
> > > > Sent: Wednesday, June 05, 2019 11:42 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Wang, Jian J; Wu, Hao A; Zhang, Chao B; Xu, Wei6
> > > > Subject: [edk2-devel][Patch v2 3/7] MdeModulePkg: Add
> > > > CapsuleOnDiskLoadPei PEIM.
> > > >
> > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1852
> > > >
> > > > This module provides PPI to load Capsule On Disk temp relocation
> > > > file from Root Directory file system, retrieve the capsules from the
> > > > temp file and create capsule hobs for these capsules.
> > > >
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > Cc: Chao B Zhang <chao.b.zhang@intel.com>
> > > > Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
> > > > ---
> > > >  MdeModulePkg/MdeModulePkg.dsc                      |   4 +
> > > >  .../CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.c    | 442
> > > > +++++++++++++++++++++
> > > >  .../CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.inf  |  64 +++
> > > > .../CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.uni  |  15 +
> > > >  .../CapsuleOnDiskLoadPeiExtra.uni                  |  14 +
> > > >  5 files changed, 539 insertions(+)
> > > >  create mode 100644
> > > >
> > >
> >
> MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.c
> > > >  create mode 100644
> > > >
> > >
> >
> MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.in
> > > > f
> > > >  create mode 100644
> > > >
> > >
> >
> MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.u
> > > > ni
> > > >  create mode 100644
> > > >
> > >
> >
> MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPeiEx
> > > > tra.uni
> > >
> > > Since this a new module, could you help to follow the recommendation
> > > in
> > >
> https://edk2.groups.io/g/devel/message/39655?p=,,,20,0,0,0::Created,,U
> > > efi
> > > DebugLibStdErr,20,2,0,31318888
> > >
> > > to add/update 'static' (lower case) for global variables/functions
> > > whose scope is limited within a single file?
> > >
> > > >
> > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > > > b/MdeModulePkg/MdeModulePkg.dsc index 995fd805e1..615edddbcc
> > > 100644
> > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > @@ -197,10 +197,13 @@
> > > >
> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x06
> > > >
> > >
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
> > > >
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
> > > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
> > > >
> > > > +[PcdsDynamicExDefault]
> > > > +
> > > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdRecoveryFileName|L"FVMAIN.FV"
> > > > +
> > > >  [Components]
> > > >    MdeModulePkg/Application/HelloWorld/HelloWorld.inf
> > > >    MdeModulePkg/Application/DumpDynPcd/DumpDynPcd.inf
> > > >
> > MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.inf
> > > >
> > > > @@ -315,10 +318,11 @@
> > > >
> > > >
> > >
> >
> NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMainte
> > > > nanceManagerUiLib.inf
> > > >    }
> > > >
> > > >
> > >
> >
> MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManager
> > > > Dxe.inf
> > > >
> > > >
> > >
> >
> MdeModulePkg/Universal/BootManagerPolicyDxe/BootManagerPolicyDxe.i
> > > > nf
> > > >    MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> > > > +
> > > >
> > >
> >
> MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.in
> > > > f
> > > >
> > MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> > > >
> > >
> MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf
> > > >
> MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
> > > >
> > > >
> > >
> >
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleD
> > > > xe.inf
> > > >
> > > >
> > >
> >
> MdeModulePkg/Universal/Console/GraphicsOutputDxe/GraphicsOutputDx
> > > > e.inf
> > > > diff --git
> > > >
> > >
> >
> a/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > c
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > c
> > > > new file mode 100644
> > > > index 0000000000..40d25f3d3b
> > > > --- /dev/null
> > > > +++
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > c
> > > > @@ -0,0 +1,442 @@
> > > > +/** @file
> > > > +  Recovery module.
> > > > +
> > > > +  Caution: This module requires additional review when modified.
> > > > +  This module will have external input - Capsule-on-Disk Temp
> > > > + Relocation
> > > > image.
> > > > +  This external input must be validated carefully to avoid security
> > > > + issue like  buffer overflow, integer overflow.
> > > > +
> > > > +  RetrieveRelocatedCapsule() will receive untrusted input and do
> > > > + basic
> > > > validation.
> > > > +
> > > > +  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > +
> > > > +**/
> > > > +
> > > > +//
> > > > +// The package level header files this module uses // #include
> > > > +<Uefi.h> #include <PiPei.h>
> > > > +
> > > > +//
> > > > +// The protocols, PPI and GUID defintions for this module //
> > > > +#include <Ppi/MasterBootMode.h> #include
> > <Ppi/FirmwareVolumeInfo.h>
> > > #include
> > > > +<Ppi/ReadOnlyVariable2.h> #include <Ppi/Capsule.h> #include
> > > > +<Ppi/CapsuleOnDisk.h> #include <Ppi/DeviceRecoveryModule.h>
> > > > +
> > > > +#include <Guid/FirmwareFileSystem2.h> // // The Library classes
> > > > +this module consumes // #include <Library/DebugLib.h> #include
> > > > +<Library/PeimEntryPoint.h> #include <Library/PeiServicesLib.h>
> > > > +#include <Library/HobLib.h> #include <Library/BaseMemoryLib.h>
> > > > +#include <Library/MemoryAllocationLib.h> #include
> > > > +<Library/PcdLib.h> #include <Library/CapsuleLib.h> #include
> > > > +<Library/ReportStatusCodeLib.h>
> > > > +
> > > > +/**
> > > > +  Loads a DXE capsule from some media into memory and updates the
> > > HOB
> > > > table
> > > > +  with the DXE firmware volume information.
> > > > +
> > > > +  @param[in]  PeiServices   General-purpose services that are available
> > to
> > > > every PEIM.
> > > > +  @param[in]  This          Indicates the EFI_PEI_RECOVERY_MODULE_PPI
> > > > instance.
> > > > +
> > > > +  @retval EFI_SUCCESS        The capsule was loaded correctly.
> > > > +  @retval EFI_DEVICE_ERROR   A device error occurred.
> > > > +  @retval EFI_NOT_FOUND      A recovery DXE capsule cannot be found.
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +LoadCapsuleOnDisk (
> > > > +  IN EFI_PEI_SERVICES              **PeiServices,
> > > > +  IN EFI_PEI_CAPSULE_ON_DISK_PPI   *This
> > > > +  );
> > > > +
> > > > +EFI_PEI_CAPSULE_ON_DISK_PPI mCapsuleOnDiskPpi = {
> > > > +  LoadCapsuleOnDisk
> > > > +};
> > > > +
> > > > +EFI_PEI_PPI_DESCRIPTOR mCapsuleOnDiskPpiList = {
> > > > +  (EFI_PEI_PPI_DESCRIPTOR_PPI |
> > > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> > > > +  &gEdkiiPeiCapsuleOnDiskPpiGuid,
> > > > +  &mCapsuleOnDiskPpi
> > > > +};
> > > > +
> > > > +/**
> > > > +  Determine if capsule comes from memory by checking Capsule PPI.
> > > > +
> > > > +  @param[in]  PeiServices General purpose services available to
> > > > + every
> > > PEIM.
> > > > +
> > > > +  @retval TRUE   Capsule comes from memory.
> > > > +  @retval FALSE  No capsule comes from memory.
> > > > +
> > > > +**/
> > > > +STATIC
> > > > +BOOLEAN
> > > > +CheckCapsuleFromRam (
> > > > +  IN CONST EFI_PEI_SERVICES          **PeiServices
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS              Status;
> > > > +  PEI_CAPSULE_PPI         *Capsule;
> > > > +
> > > > +  Status = PeiServicesLocatePpi (
> > > > +             &gPeiCapsulePpiGuid,
> > >
> > >
> > > Suggest to use gEfiPeiCapsulePpiGuid here.
> > > gPeiCapsulePpiGuid is kept for compatibility before PI Version 1.4.
> > >
> > >
> > > > +             0,
> > > > +             NULL,
> > > > +             (VOID **) &Capsule
> > > > +             );
> > > > +  if (!EFI_ERROR(Status)) {
> > > > +    Status = Capsule->CheckCapsuleUpdate ((EFI_PEI_SERVICES
> > > > **)PeiServices);
> > > > +    if (!EFI_ERROR(Status)) {
> > > > +      return TRUE;
> > > > +    }
> > > > +  }
> > > > +
> > > > +  return FALSE;
> > > > +}
> > > > +
> > > > +/**
> > > > +  Determine if it is a Capsule On Disk mode.
> > > > +
> > > > +  @retval TRUE         Capsule On Disk mode.
> > > > +  @retval FALSE        Not capsule On Disk mode.
> > > > +
> > > > +**/
> > > > +BOOLEAN
> > > > +IsCapsuleOnDiskMode (
> > > > +  VOID
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS                      Status;
> > > > +  UINTN                           Size;
> > > > +  EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices;
> > > > +  BOOLEAN                         CodRelocInfo;
> > > > +
> > > > +  Status = PeiServicesLocatePpi (
> > > > +             &gEfiPeiReadOnlyVariable2PpiGuid,
> > > > +             0,
> > > > +             NULL,
> > > > +             (VOID **) &PPIVariableServices
> > > > +             );
> > > > +  ASSERT_EFI_ERROR (Status);
> > > > +
> > > > +  Size = sizeof (BOOLEAN);
> > > > +  Status = PPIVariableServices->GetVariable (
> > > > +                                  PPIVariableServices,
> > > > +                                  COD_RELOCATION_INFO_VAR_NAME,
> > > > +                                  &gEfiCapsuleVendorGuid,
> > > > +                                  NULL,
> > > > +                                  &Size,
> > > > +                                  &CodRelocInfo
> > > > +                                  );
> > > > +
> > > > +  if (EFI_ERROR (Status) || Size != sizeof(BOOLEAN) || CodRelocInfo
> > > > + != TRUE)
> > >
> > >
> > > For 'CodRelocInfo != TRUE', variable of BOOLEAN type can be directly
> > > used in the 'if' statement without comparing with 'TRUE' or 'FALSE'.
> > >
> > >
> > > > {
> > > > +    DEBUG (( DEBUG_ERROR, "Error Get CodRelocationInfo variable
> > > > + %r!\n",
> > > > Status));
> > > > +    return FALSE;
> > > > +  }
> > > > +
> > > > +  return TRUE;
> > > > +}
> > > > +
> > > > +/**
> > > > +  Gets capsule images from relocated capsule buffer.
> > > > +  Create Capsule hob for each Capsule.
> > > > +
> > > > +  Caution: This function may receive untrusted input.
> > > > +  Capsule-on-Disk Temp Relocation image is external input, so this
> > > > + function  will validate Capsule-on-Disk Temp Relocation image to
> > > > + make sure the
> > > > content
> > > > +  is read within the buffer.
> > > > +
> > > > +  @param[in]  RelocCapsuleBuf        Buffer pointer to the relocated
> > capsule.
> > > > +  @param[in]  RelocCapsuleTotalSize  Total size of the relocated
> capsule.
> > > > +
> > > > +  @retval EFI_SUCCESS     Succeed to get capsules and create hob.
> > > > +  @retval Others          Fail to get capsules and create hob.
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +RetrieveRelocatedCapsule (
> > > > +  IN UINT8                *RelocCapsuleBuf,
> > > > +  IN UINTN                RelocCapsuleTotalSize
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS               Status;
> > > > +  UINTN                    Index;
> > > > +  UINT8                    *CapsuleDataBufEnd;
> > > > +  UINT8                    *CapsulePtr;
> > > > +  UINT32                   CapsuleSize;
> > > > +  UINT64                   TotalImageSize;
> > > > +  UINTN                    CapsuleNum;
> > > > +
> > > > +  CapsuleNum = 0;
> > > > +
> > > > +  //
> > > > +  // Temp file contains at least 2 capsule (including 1 capsule
> > > > + name
> > > > + capsule)
> > > > & 1 UINT64
> > > > +  //
> > > > +  if (RelocCapsuleTotalSize < sizeof(UINT64) +
> > > > + sizeof(EFI_CAPSULE_HEADER)
> > > > * 2) {
> > > > +    return EFI_INVALID_PARAMETER;
> > > > +  }
> > > > +
> > > > +  CopyMem(&TotalImageSize, RelocCapsuleBuf, sizeof(UINT64));
> > > > +
> > > > +  DEBUG ((DEBUG_INFO, "ProcessRelocatedCapsule CapsuleBuf %x
> > > > TotalCapSize %lx\n",
> > > > +                      RelocCapsuleBuf, TotalImageSize));
> > > > +
> > > > +  RelocCapsuleBuf += sizeof(UINT64);
> > > > +
> > > > +  //
> > > > +  // TempCaspule file length check
> > > > +  //
> > > > +  if (MAX_ADDRESS - TotalImageSize <= sizeof(UINT64) ||
> > > > +      (UINT64)RelocCapsuleTotalSize != TotalImageSize + sizeof(UINT64)
> > ||
> > > > +      (UINTN)(MAX_ADDRESS -
> > > > (PHYSICAL_ADDRESS)(UINTN)RelocCapsuleBuf) <= TotalImageSize) {
> > > > +    return EFI_INVALID_PARAMETER;
> > > > +  }
> > > > +
> > > > +  CapsuleDataBufEnd = RelocCapsuleBuf + TotalImageSize;
> > > > +
> > > > +  //
> > > > +  // TempCapsule file integrity check over Capsule Header to ensure
> > > > + no data
> > > > corruption in NV Var & Relocation storage
> > > > +  //
> > > > +  CapsulePtr = RelocCapsuleBuf;
> > > > +
> > > > +  while (CapsulePtr < CapsuleDataBufEnd) {
> > > > +    if ((CapsuleDataBufEnd - CapsulePtr) < sizeof(EFI_CAPSULE_HEADER)
> > ||
> > > > +        ((EFI_CAPSULE_HEADER *)CapsulePtr)->CapsuleImageSize <
> > > > sizeof(EFI_CAPSULE_HEADER) ||
> > > > +        (UINTN)(MAX_ADDRESS -
> (PHYSICAL_ADDRESS)(UINTN)CapsulePtr)
> > > > + <
> > > > ((EFI_CAPSULE_HEADER *)CapsulePtr)->CapsuleImageSize
> > > > +        ) {
> > > > +      break;
> > > > +    }
> > > > +    CapsulePtr += ((EFI_CAPSULE_HEADER *)CapsulePtr)-
> > > >CapsuleImageSize;
> > > > +    CapsuleNum ++;
> > > > +  }
> > > > +
> > > > +  if (CapsulePtr != CapsuleDataBufEnd) {
> > > > +    Status = EFI_INVALID_PARAMETER;
> > > > +    goto EXIT;
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Capsule count must be less than PcdCapsuleMax, avoid building
> > > > + too
> > > > many CvHobs to occupy all the free space in HobList.
> > > > +  //
> > > > +  if (CapsuleNum > PcdGet16 (PcdCapsuleMax)) {
> > > > +    Status = EFI_INVALID_PARAMETER;
> > > > +    goto EXIT;
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Re-iterate the capsule buffer to create Capsule hob & Capsule
> > > > + Name Str
> > > > Hob for each Capsule saved in relocated capsule file
> > > > +  //
> > > > +  CapsulePtr = RelocCapsuleBuf;
> > > > +  Index      = 0;
> > > > +  while (CapsulePtr < CapsuleDataBufEnd) {
> > > > +    CapsuleSize = ((EFI_CAPSULE_HEADER *)CapsulePtr)-
> > > >CapsuleImageSize;
> > > > +    BuildCvHob ((EFI_PHYSICAL_ADDRESS)(UINTN)CapsulePtr,
> > > > + CapsuleSize);
> > > > +
> > > > +    DEBUG((DEBUG_INFO, "Capsule saved in address %x size %x\n",
> > > > CapsulePtr, CapsuleSize));
> > > > +
> > > > +    CapsulePtr += CapsuleSize;
> > > > +    Index++;
> > > > +  }
> > > > +
> > > > +EXIT:
> > > > +
> > > > +  return Status;
> > > > +}
> > > > +
> > > > +/**
> > > > +  Recovery module entrypoint
> > > > +
> > > > +  @param[in] FileHandle   Handle of the file being invoked.
> > > > +  @param[in] PeiServices  Describes the list of possible PEI Services.
> > > > +
> > > > +  @return EFI_SUCCESS Recovery module is initialized.
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +InitializeCapsuleOnDiskLoad (
> > > > +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> > > > +  IN CONST EFI_PEI_SERVICES     **PeiServices
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS  Status;
> > > > +  UINTN       BootMode;
> > > > +  UINTN       FileNameSize;
> > > > +
> > > > +  BootMode = GetBootModeHob();
> > > > +  ASSERT(BootMode == BOOT_ON_FLASH_UPDATE);
> > > > +
> > > > +  //
> > > > +  // If there are capsules provisioned in memory, quit.
> > > > +  // Only one capsule resource is accept, CapsuleOnRam's priority
> > > > + is higher
> > > > than CapsuleOnDisk.
> > > > +  //
> > > > +  if (CheckCapsuleFromRam(PeiServices)) {
> > > > +    DEBUG((DEBUG_ERROR, "Capsule On Memory Detected! Quit.\n"));
> > > > +    return EFI_ABORTED;
> > > > +  }
> > > > +
> > > > +  DEBUG_CODE (
> > > > +   VOID *CapsuleOnDiskModePpi;
> > > > +
> > > > +  if (!IsCapsuleOnDiskMode()){
> > > > +    return EFI_NOT_FOUND;
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Check Capsule On Disk Relocation flag. If exists, load capsule
> > > > + & create
> > > > Capsule Hob
> > > > +  //
> > > > +  Status = PeiServicesLocatePpi (
> > > > +             &gEfiPeiBootInCapsuleOnDiskModePpiGuid,
> > > > +             0,
> > > > +             NULL,
> > > > +             (VOID **)&CapsuleOnDiskModePpi
> > > > +             );
> > > > +    if (EFI_ERROR(Status)) {
> > > > +      DEBUG((DEBUG_ERROR, "Locate CapsuleOnDiskModePpi
> > error %x\n",
> > > > Status));
> > > > +      return Status;
> > > > +    }
> > > > +  );
> > > > +
> > > > +  Status = (**PeiServices).InstallPpi (PeiServices,
> > > > + &mCapsuleOnDiskPpiList);
> > >
> > >
> > > Minor one, suggest to directly use PeiServicesInstallPpi().
> > >
> > >
> > > > +  ASSERT_EFI_ERROR (Status);
> > > > +
> > > > +  FileNameSize = PcdGetSize (PcdCoDRelocationFileName);  Status =
> > > > + PcdSetPtrS (PcdRecoveryFileName, &FileNameSize, (VOID *)
> > > > PcdGetPtr(PcdCoDRelocationFileName));
> > >
> > >
> > > Buffer for 'PcdRecoveryFileName' may not be big enough to hold the
> > > content in 'PcdCoDRelocationFileName'.
> > >
> > > I think there might be a chance for the above PcdSetPtrS() call to fail.
> > >
> > >
> > > > +  ASSERT_EFI_ERROR (Status);
> > > > +
> > > > +  return Status;
> > > > +}
> > > > +
> > > > +/**
> > > > +  Loads a DXE capsule from some media into memory and updates the
> > > HOB
> > > > table
> > > > +  with the DXE firmware volume information.
> > > > +
> > > > +  @param[in]  PeiServices   General-purpose services that are available
> > to
> > > > every PEIM.
> > > > +  @param[in]  This          Indicates the EFI_PEI_RECOVERY_MODULE_PPI
> > > > instance.
> > > > +
> > > > +  @retval EFI_SUCCESS        The capsule was loaded correctly.
> > > > +  @retval EFI_DEVICE_ERROR   A device error occurred.
> > > > +  @retval EFI_NOT_FOUND      A recovery DXE capsule cannot be found.
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +LoadCapsuleOnDisk (
> > > > +  IN EFI_PEI_SERVICES                     **PeiServices,
> > > > +  IN EFI_PEI_CAPSULE_ON_DISK_PPI          *This
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS                          Status;
> > > > +  EFI_PEI_DEVICE_RECOVERY_MODULE_PPI  *DeviceRecoveryPpi;
> > > > +  UINTN                               NumberRecoveryCapsules;
> > > > +  UINTN                               Instance;
> > > > +  UINTN                               CapsuleInstance;
> > > > +  UINTN                               CapsuleSize;
> > > > +  EFI_GUID                            CapsuleType;
> > > > +  VOID                                *CapsuleBuffer;
> > > > +
> > > > +  DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Load Capsule On Disk
> > > Entry\n"));
> > > > +
> > > > +  for (Instance = 0; ; Instance++) {
> > > > +    Status = PeiServicesLocatePpi (
> > > > +               &gEfiPeiDeviceRecoveryModulePpiGuid,
> > > > +               Instance,
> > > > +               NULL,
> > > > +               (VOID **)&DeviceRecoveryPpi
> > > > +               );
> > > > +    DEBUG ((DEBUG_INFO, "LoadCapsuleOnDisk - LocateRecoveryPpi
> (%d)
> > > -
> > > >  %r\n", Instance, Status));
> > > > +    if (EFI_ERROR (Status)) {
> > > > +      if (Instance == 0) {
> > > > +        REPORT_STATUS_CODE (
> > > > +          EFI_ERROR_CODE | EFI_ERROR_MAJOR,
> > > > +          (EFI_SOFTWARE_PEI_MODULE |
> > > > EFI_SW_PEI_EC_RECOVERY_PPI_NOT_FOUND)
> > > > +          );
> > > > +      }
> > > > +      break;
> > > > +    }
> > > > +    NumberRecoveryCapsules = 0;
> > > > +    Status = DeviceRecoveryPpi->GetNumberRecoveryCapsules (
> > > > +                                  (EFI_PEI_SERVICES **)PeiServices,
> > > > +                                  DeviceRecoveryPpi,
> > > > +                                  &NumberRecoveryCapsules
> > > > +                                  );
> > > > +    DEBUG ((DEBUG_INFO, "LoadCapsuleOnDisk -
> > > > GetNumberRecoveryCapsules (%d) - %r\n", NumberRecoveryCapsules,
> > > > Status));
> > > > +    if (EFI_ERROR (Status)) {
> > > > +      continue;
> > > > +    }
> > > > +
> > > > +    for (CapsuleInstance = 1; CapsuleInstance <=
> > > > + NumberRecoveryCapsules;
> > > > CapsuleInstance++) {
> > > > +      CapsuleSize = 0;
> > > > +      Status = DeviceRecoveryPpi->GetRecoveryCapsuleInfo (
> > > > +                                    (EFI_PEI_SERVICES **)PeiServices,
> > > > +                                    DeviceRecoveryPpi,
> > > > +                                    CapsuleInstance,
> > > > +                                    &CapsuleSize,
> > > > +                                    &CapsuleType
> > > > +                                    );
> > > > +      DEBUG ((DEBUG_INFO, "LoadCapsuleOnDisk -
> > > GetRecoveryCapsuleInfo
> > > > (%d - %x) - %r\n", CapsuleInstance, CapsuleSize, Status));
> > > > +      if (EFI_ERROR (Status)) {
> > > > +        break;
> > > > +      }
> > > > +
> > > > +      //
> > > > +      // Allocate the memory so that it gets preserved into DXE.
> > > > +      // Capsule is special because it may need to populate to system
> table
> > > > +      //
> > > > +      CapsuleBuffer = AllocateRuntimePages (EFI_SIZE_TO_PAGES
> > > > (CapsuleSize));
> > > > +
> > > > +      if (CapsuleBuffer == NULL) {
> > > > +        DEBUG ((DEBUG_ERROR, "LoadCapsuleOnDisk -
> > > > + AllocateRuntimePages
> > > > fail\n"));
> > > > +        continue;
> > > > +      }
> > > > +
> > > > +      Status = DeviceRecoveryPpi->LoadRecoveryCapsule (
> > > > +                                    (EFI_PEI_SERVICES **)PeiServices,
> > > > +                                    DeviceRecoveryPpi,
> > > > +                                    CapsuleInstance,
> > > > +                                    CapsuleBuffer
> > > > +                                    );
> > > > +      DEBUG ((DEBUG_INFO, "LoadCapsuleOnDisk -
> LoadRecoveryCapsule
> > > > (%d) - %r\n", CapsuleInstance, Status));
> > > > +      if (EFI_ERROR (Status)) {
> > > > +        FreePages (CapsuleBuffer, EFI_SIZE_TO_PAGES(CapsuleSize));
> > > > +        break;
> > > > +      }
> > > > +
> > > > +      //
> > > > +      // Capsule Update Mode, Split relocated Capsule buffer into
> > > > + different
> > > > capsule vehical hobs.
> > > > +      //
> > > > +      Status = RetrieveRelocatedCapsule(CapsuleBuffer,
> > > > + CapsuleSize);
> > > > +
> > > > +      break;
> > > > +    }
> > > > +
> > > > +    if (EFI_ERROR (Status)) {
> > > > +      REPORT_STATUS_CODE (
> > > > +        EFI_ERROR_CODE | EFI_ERROR_MAJOR,
> > > > +        (EFI_SOFTWARE_PEI_MODULE |
> > > > EFI_SW_PEI_EC_NO_RECOVERY_CAPSULE)
> > > > +        );
> > > > +    }
> > > > +
> > > > +    return Status;
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Any attack against GPT, Relocation Info Variable or temp
> > > > + relocation file
> > > > will result in no Capsule HOB and return EFI_NOT_FOUND.
> > > > +  // After flow to DXE phase. since no capsule hob is detected.
> > > > + Platform will
> > > > clear Info flag and force restart.
> > > > +  // No volunerability will be exposed  //
> > > > +
> > > > +  return EFI_NOT_FOUND;
> > > > +}
> > > > diff --git
> > > >
> > >
> >
> a/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > inf
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > inf
> > > > new file mode 100644
> > > > index 0000000000..4af07440b7
> > > > --- /dev/null
> > > > +++
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > inf
> > > > @@ -0,0 +1,64 @@
> > > > +## @file
> > > > +# Load Capsule on Disk module.
> > > > +#
> > > > +# Load Capsule On Disk from Root Directory file system. Create CV
> > > > +hob # based on temporary Capsule On Disk file.
> > > > +#
> > > > +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> #
> > > > +#
> > > > +SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> > > > +
> > > > +[Defines]
> > > > +  INF_VERSION                    = 0x00010005
> > > > +  BASE_NAME                      = CapsuleOnDiskLoadPei
> > > > +  MODULE_UNI_FILE                = CapsuleOnDiskLoadPei.uni
> > > > +  FILE_GUID                      = 8ADEDF9E-2EC8-40fb-AE56-B76D90225D2D
> > > > +  MODULE_TYPE                    = PEIM
> > > > +  VERSION_STRING                 = 1.0
> > > > +  ENTRY_POINT                    = InitializeCapsuleOnDiskLoad
> > > > +
> > > > +#
> > > > +# The following information is for reference only and not required
> > > > +by the
> > > > build tools.
> > > > +#
> > > > +#  VALID_ARCHITECTURES           = IA32 X64 EBC
> > > > +#
> > > > +
> > > > +[Sources]
> > > > +  CapsuleOnDiskLoadPei.c
> > > > +
> > > > +[Packages]
> > > > +  MdePkg/MdePkg.dec
> > > > +  MdeModulePkg/MdeModulePkg.dec
> > > > +
> > > > +[LibraryClasses]
> > > > +  PeimEntryPoint
> > > > +  DebugLib
> > > > +  HobLib
> > > > +  BaseMemoryLib
> > > > +  MemoryAllocationLib
> > > > +  ReportStatusCodeLib
> > > > +
> > > > +[Ppis]
> > > > +  gEdkiiPeiCapsuleOnDiskPpiGuid           ## PRODUCES
> > > > +  gEfiPeiReadOnlyVariable2PpiGuid         ## CONSUMES
> > > > +  gEfiPeiBootInCapsuleOnDiskModePpiGuid   ##
> > SOMETIMES_CONSUMES
> > > > +  gEfiPeiDeviceRecoveryModulePpiGuid      ## CONSUMES
> > > > +  gPeiCapsulePpiGuid                      ## CONSUMES
> > >
> > >
> > > Suggest to use gEfiPeiCapsulePpiGuid here.
> > > gPeiCapsulePpiGuid is kept for compatibility before PI Version 1.4.
> > >
> > >
> > > > +
> > > > +[Guids]
> > > > +  gEfiCapsuleVendorGuid                   ## SOMETIMES_CONSUMES ##
> > > Variable
> > > > L"CodRelocationInfo"
> > > > +
> > > > +[Pcd]
> > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCoDRelocationFileName
> > > > ## CONSUMES
> > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleMax
> ##
> > > > CONSUMES
> > > > +
> > > > +[PcdEx]
> > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRecoveryFileName
> > > ##
> > > > PRODUCES
> > > > +
> > > > +[depex]
> > >
> > >
> > > Minor comment:
> > > [depex] -> [Depex]
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > > +  gEfiPeiBootInCapsuleOnD
> > > iskModePpiGuid
> > > > +
> > > > +[UserExtensions.TianoCore."ExtraFiles"]
> > > > +  CapsuleOnDiskLoadPeiExtra.uni
> > > > diff --git
> > > >
> > >
> >
> a/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > uni
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > uni
> > > > new file mode 100644
> > > > index 0000000000..c3eae6a5c2
> > > > --- /dev/null
> > > > +++
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei.
> > > > uni
> > > > @@ -0,0 +1,15 @@
> > > > +// /** @file
> > > > +// Caspule On Disk Load module.
> > > > +//
> > > > +// Load Capsule On Disk and build CV hob.
> > > > +//
> > > > +// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > > > +// // SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
> > > > +
> > > > +
> > > > +#string STR_MODULE_ABSTRACT             #language en-US "Caspule On
> > Disk
> > > > Load module."
> > > > +
> > > > +#string STR_MODULE_DESCRIPTION          #language en-US "Load
> > Capsule
> > > > On Disk and build CV hob."
> > > > diff --git
> > > >
> > >
> >
> a/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei
> > > > Extra.uni
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei
> > > > Extra.uni
> > > > new file mode 100644
> > > > index 0000000000..81034f6294
> > > > --- /dev/null
> > > > +++
> > > >
> > >
> >
> b/MdeModulePkg/Universal/CapsuleOnDiskLoadPei/CapsuleOnDiskLoadPei
> > > > Extra.uni
> > > > @@ -0,0 +1,14 @@
> > > > +// /** @file
> > > > +// CapsuleOnDiskLoadPei Localized Strings and Content // //
> > > > +Copyright
> > > > +(c) 2019, Intel Corporation. All rights reserved.<BR> // //
> > > > +SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
> > > > +
> > > > +#string STR_PROPERTIES_MODULE_NAME
> > > > +#language en-US
> > > > +"CapsuleOnDiskLoad PEI Driver"
> > > > +
> > > > +
> > > > --
> > > > 2.16.2.windows.1
> > > >
> > > >
> > > >
> >
> >
> >