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

["Oliver Smith-Denny" <osd@smith-denny.com>]

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