Re: [edk2-devel] [Patch 2/2] SecurityPkg/FvReportPei: Use FirmwareVolumeShadowPpi

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

I agree with your comment.  Can you please enter a new BZ to address it?

Thanks,

Mike

> -----Original Message-----
> From: Oliver Smith-Denny <osd@smith-denny.com>
> Sent: Wednesday, March 22, 2023 9:05 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Patel, Umang <umang.patel@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [Patch 2/2] SecurityPkg/FvReportPei: Use FirmwareVolumeShadowPpi
> 
> One comment below, thanks!
> 
> On 3/21/2023 7:06 PM, Michael D Kinney wrote:
> > From: Umang Patel <umang.patel@intel.com>
> >
> > If FirmwareVolumeShadow PPI is available, then use it to
> > shadow FVs to memory.  Otherwise fallback to CopyMem().
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Signed-off-by: Patel Umang <umang.patel@intel.com>
> > ---
> >   SecurityPkg/FvReportPei/FvReportPei.c   | 37 ++++++++++++++++++++-----
> >   SecurityPkg/FvReportPei/FvReportPei.h   |  1 +
> >   SecurityPkg/FvReportPei/FvReportPei.inf |  1 +
> >   3 files changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/SecurityPkg/FvReportPei/FvReportPei.c b/SecurityPkg/FvReportPei/FvReportPei.c
> > index 846605cda1e4..6288dde16b2a 100644
> > --- a/SecurityPkg/FvReportPei/FvReportPei.c
> > +++ b/SecurityPkg/FvReportPei/FvReportPei.c
> > @@ -114,12 +114,13 @@ VerifyHashedFv (
> >     IN EFI_BOOT_MODE   BootMode
> >     )
> >   {
> > -  UINTN                FvIndex;
> > -  CONST HASH_ALG_INFO  *AlgInfo;
> > -  UINT8                *HashValue;
> > -  UINT8                *FvHashValue;
> > -  VOID                 *FvBuffer;
> > -  EFI_STATUS           Status;
> > +  UINTN                                 FvIndex;
> > +  CONST HASH_ALG_INFO                   *AlgInfo;
> > +  UINT8                                 *HashValue;
> > +  UINT8                                 *FvHashValue;
> > +  VOID                                  *FvBuffer;
> > +  EDKII_PEI_FIRMWARE_VOLUME_SHADOW_PPI  *FvShadowPpi;
> > +  EFI_STATUS                            Status;
> >
> >     if ((HashInfo == NULL) ||
> >         (HashInfo->HashSize == 0) ||
> > @@ -191,8 +192,30 @@ VerifyHashedFv (
> >       // Copy FV to permanent memory to avoid potential TOC/TOU.
> >       //
> >       FvBuffer = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)FvInfo[FvIndex].Length));
> > +
> >       ASSERT (FvBuffer != NULL);
> 
> While we are here, should we make this more robust (and amenable to
> static analysis) and add error handling if FvBuffer is NULL, not just
> assert?
> 
> > -    CopyMem (FvBuffer, (CONST VOID *)(UINTN)FvInfo[FvIndex].Base, (UINTN)FvInfo[FvIndex].Length);
> > +    Status = PeiServicesLocatePpi (
> > +               &gEdkiiPeiFirmwareVolumeShadowPpiGuid,
> > +               0,
> > +               NULL,
> > +               (VOID **)&FvShadowPpi
> > +               );
> > +
> > +    if (!EFI_ERROR (Status)) {
> > +      Status = FvShadowPpi->FirmwareVolumeShadow (
> > +                              (EFI_PHYSICAL_ADDRESS)FvInfo[FvIndex].Base,
> > +                              FvBuffer,
> > +                              (UINTN)FvInfo[FvIndex].Length
> > +                              );
> > +    }
> > +
> > +    if (EFI_ERROR (Status)) {
> > +      CopyMem (
> > +        FvBuffer,
> > +        (CONST VOID *)(UINTN)FvInfo[FvIndex].Base,
> > +        (UINTN)FvInfo[FvIndex].Length
> > +        );
> > +    }
> >
> >       if (!AlgInfo->HashAll (FvBuffer, (UINTN)FvInfo[FvIndex].Length, FvHashValue)) {
> >         Status = EFI_ABORTED;
> > diff --git a/SecurityPkg/FvReportPei/FvReportPei.h b/SecurityPkg/FvReportPei/FvReportPei.h
> > index 92504a3c51e1..07ffb2f5768c 100644
> > --- a/SecurityPkg/FvReportPei/FvReportPei.h
> > +++ b/SecurityPkg/FvReportPei/FvReportPei.h
> > @@ -14,6 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >   #include <IndustryStandard/Tpm20.h>
> >
> >   #include <Ppi/FirmwareVolumeInfoStoredHashFv.h>
> > +#include <Ppi/FirmwareVolumeShadowPpi.h>
> >
> >   #include <Library/PeiServicesLib.h>
> >   #include <Library/PcdLib.h>
> > diff --git a/SecurityPkg/FvReportPei/FvReportPei.inf b/SecurityPkg/FvReportPei/FvReportPei.inf
> > index 408406889765..4246fb75ebaa 100644
> > --- a/SecurityPkg/FvReportPei/FvReportPei.inf
> > +++ b/SecurityPkg/FvReportPei/FvReportPei.inf
> > @@ -46,6 +46,7 @@ [LibraryClasses]
> >   [Ppis]
> >     gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid   ## PRODUCES
> >     gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid  ## CONSUMES
> > +  gEdkiiPeiFirmwareVolumeShadowPpiGuid            ## CONSUMES
> >
> >   [Pcd]
> >     gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass