I agree with your comment. Can you please enter a new BZ to address it?
Thanks,
Mike
> -----Original Message-----
> From: Oliver Smith-Denny <o...@smith-denny.com>
> Sent: Wednesday, March 22, 2023 9:05 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Patel, Umang <umang.pa...@intel.com>; Yao, Jiewen <jiewen....@intel.com>;
> Wang, Jian J <jian.j.w...@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.pa...@intel.com>
> >
> > If FirmwareVolumeShadow PPI is available, then use it to
> > shadow FVs to memory. Otherwise fallback to CopyMem().
> >
> > Cc: Jiewen Yao <jiewen....@intel.com>
> > Cc: Jian J Wang <jian.j.w...@intel.com>
> > Signed-off-by: Patel Umang <umang.pa...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101970): https://edk2.groups.io/g/devel/message/101970
Mute This Topic: https://groups.io/mt/97770069/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
