I mean putting all rev0 code in 'else' not just one line. For rev105, your patch will initialize FvBlob but never use it at all. It will confuse readers sometimes.
Regards, Jian > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: Monday, January 06, 2020 1:53 PM > To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io > Cc: Zhang, Chao B <chao.b.zh...@intel.com> > Subject: RE: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. > > Hi Jian > I purposely put the line there, because I want to group all the rev 0 code > together and rev 105 code together in if statement. I don't want to move that > particular line to else statement. > > Thank you > Yao Jiewen > > > -----Original Message----- > > From: Wang, Jian J <jian.j.w...@intel.com> > > Sent: Monday, January 6, 2020 1:33 PM > > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io > > Cc: Zhang, Chao B <chao.b.zh...@intel.com> > > Subject: RE: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. > > > > Jiewen, > > > > Just one minor comment. With it addressed, > > > > Reviewed-by: Jian J Wang <jian.j.w...@intel.com> > > > > > > > -----Original Message----- > > > From: Yao, Jiewen <jiewen....@intel.com> > > > Sent: Tuesday, December 31, 2019 2:44 PM > > > To: devel@edk2.groups.io > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Zhang, Chao B > > > <chao.b.zh...@intel.com> > > > Subject: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2439 > > > > > > Use EV_EFI_PLATFORM_FIRMWARE_BLOB2 if the TCG PFP revision is >= 105. > > > Use FvName as the description for the FV. > > > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > > Cc: Chao Zhang <chao.b.zh...@intel.com> > > > Signed-off-by: Jiewen Yao <jiewen....@intel.com> > > > --- > > > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 91 ++++++++++++++++++++++++++--- > > > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 2 + > > > 2 files changed, 84 insertions(+), 9 deletions(-) > > > > > > diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > > b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > > index 1565d4e402..7d99c7906a 100644 > > > --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > > +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c > > > @@ -37,6 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > #include <Library/MemoryAllocationLib.h> > > > #include <Library/ReportStatusCodeLib.h> > > > #include <Library/ResetSystemLib.h> > > > +#include <Library/PrintLib.h> > > > > > > #define PERF_ID_TCG2_PEI 0x3080 > > > > > > @@ -78,6 +79,18 @@ EFI_PLATFORM_FIRMWARE_BLOB > > > *mMeasuredChildFvInfo; > > > UINT32 mMeasuredMaxChildFvIndex = 0; > > > UINT32 mMeasuredChildFvIndex = 0; > > > > > > +#pragma pack (1) > > > + > > > +#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX- > > > XXXXXXXXXXXX)" > > > +typedef struct { > > > + UINT8 BlobDescriptionSize; > > > + UINT8 > > > BlobDescription[sizeof(FV_HANDOFF_TABLE_DESC)]; > > > + EFI_PHYSICAL_ADDRESS BlobBase; > > > + UINT64 BlobLength; > > > +} FV_HANDOFF_TABLE_POINTERS2; > > > + > > > +#pragma pack () > > > + > > > /** > > > Measure and record the Firmware Volume Information once FvInfoPPI > install. > > > > > > @@ -447,6 +460,48 @@ MeasureCRTMVersion ( > > > ); > > > } > > > > > > +/* > > > + Get the FvName from the FV header. > > > + > > > + Causion: The FV is untrusted input. > > > + > > > + @param[in] FvBase Base address of FV image. > > > + @param[in] FvLength Length of FV image. > > > + > > > + @return FvName pointer > > > + @retval NULL FvName is NOT found > > > +*/ > > > +VOID * > > > +GetFvName ( > > > + IN EFI_PHYSICAL_ADDRESS FvBase, > > > + IN UINT64 FvLength > > > + ) > > > +{ > > > + EFI_FIRMWARE_VOLUME_HEADER *FvHeader; > > > + EFI_FIRMWARE_VOLUME_EXT_HEADER *FvExtHeader; > > > + > > > + if (FvBase >= MAX_ADDRESS) { > > > + return NULL; > > > + } > > > + if (FvLength >= MAX_ADDRESS - FvBase) { > > > + return NULL; > > > + } > > > + if (FvLength < sizeof(EFI_FIRMWARE_VOLUME_HEADER)) { > > > + return NULL; > > > + } > > > + > > > + FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvBase; > > > + if (FvHeader->ExtHeaderOffset < > sizeof(EFI_FIRMWARE_VOLUME_HEADER)) > > { > > > + return NULL; > > > + } > > > + if (FvHeader->ExtHeaderOffset + > > > sizeof(EFI_FIRMWARE_VOLUME_EXT_HEADER) > FvLength) { > > > + return NULL; > > > + } > > > + FvExtHeader = (EFI_FIRMWARE_VOLUME_EXT_HEADER *)(UINTN)(FvBase > + > > > FvHeader->ExtHeaderOffset); > > > + > > > + return &FvExtHeader->FvName; > > > +} > > > + > > > /** > > > Measure FV image. > > > Add it into the measured FV list after the FV is measured successfully. > > > @@ -469,6 +524,9 @@ MeasureFvImage ( > > > UINT32 Index; > > > EFI_STATUS Status; > > > EFI_PLATFORM_FIRMWARE_BLOB FvBlob; > > > + FV_HANDOFF_TABLE_POINTERS2 FvBlob2; > > > + VOID *EventData; > > > + VOID *FvName; > > > TCG_PCR_EVENT_HDR TcgEventHdr; > > > UINT32 Instance; > > > UINT32 Tpm2HashMask; > > > @@ -571,6 +629,21 @@ MeasureFvImage ( > > > TcgEventHdr.PCRIndex = 0; > > > TcgEventHdr.EventType = EV_EFI_PLATFORM_FIRMWARE_BLOB; > > > TcgEventHdr.EventSize = sizeof (FvBlob); > > > + EventData = &FvBlob; > > > + > > > > I'd suggest to put above code in 'else' block to make code easier to read, > > i.e. FvBlob for revision less than 105 and FvBlob2 for later ones. > > > > Regards, > > Jian > > > > > + if (PcdGet32(PcdTcgPfpMeasurementRevision) >= > > > TCG_EfiSpecIDEventStruct_SPEC_ERRATA_TPM2_REV_105) { > > > + FvBlob2.BlobDescriptionSize = sizeof(FvBlob2.BlobDescription); > > > + CopyMem (FvBlob2.BlobDescription, FV_HANDOFF_TABLE_DESC, > > > sizeof(FvBlob2.BlobDescription)); > > > + FvName = GetFvName (FvBase, FvLength); > > > + if (FvName != NULL) { > > > + AsciiSPrint ((CHAR8 *)FvBlob2.BlobDescription, > > > sizeof(FvBlob2.BlobDescription), "Fv(%g)", FvName); > > > + } > > > + FvBlob2.BlobBase = FvBlob.BlobBase; > > > + FvBlob2.BlobLength = FvBlob.BlobLength; > > > + TcgEventHdr.EventType = EV_EFI_PLATFORM_FIRMWARE_BLOB2; > > > + TcgEventHdr.EventSize = sizeof (FvBlob2); > > > + EventData = &FvBlob2; > > > + } > > > > > > if (Tpm2HashMask == 0) { > > > // > > > @@ -583,9 +656,9 @@ MeasureFvImage ( > > > ); > > > > > > if (!EFI_ERROR(Status)) { > > > - Status = LogHashEvent (&DigestList, &TcgEventHdr, (UINT8*) > > > &FvBlob); > > > - DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged > > by > > > Tcg2Pei starts at: 0x%x\n", FvBlob.BlobBase)); > > > - DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged > > by > > > Tcg2Pei has the size: 0x%x\n", FvBlob.BlobLength)); > > > + Status = LogHashEvent (&DigestList, &TcgEventHdr, EventData); > > > + DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged > > by > > > Tcg2Pei starts at: 0x%x\n", FvBase)); > > > + DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged > > by > > > Tcg2Pei has the size: 0x%x\n", FvLength)); > > > } else if (Status == EFI_DEVICE_ERROR) { > > > BuildGuidHob (&gTpmErrorHobGuid,0); > > > REPORT_STATUS_CODE ( > > > @@ -599,13 +672,13 @@ MeasureFvImage ( > > > // > > > Status = HashLogExtendEvent ( > > > 0, > > > - (UINT8*) (UINTN) FvBlob.BlobBase, > > > - (UINTN) FvBlob.BlobLength, > > > - &TcgEventHdr, > > > - (UINT8*) &FvBlob > > > + (UINT8*) (UINTN) FvBase, // HashData > > > + (UINTN) FvLength, // HashDataLen > > > + &TcgEventHdr, // EventHdr > > > + EventData // EventData > > > ); > > > - DEBUG ((DEBUG_INFO, "The FV which is measured by Tcg2Pei starts at: > > > 0x%x\n", FvBlob.BlobBase)); > > > - DEBUG ((DEBUG_INFO, "The FV which is measured by Tcg2Pei has the > > > size: > > > 0x%x\n", FvBlob.BlobLength)); > > > + DEBUG ((DEBUG_INFO, "The FV which is measured by Tcg2Pei starts at: > > > 0x%x\n", FvBase)); > > > + DEBUG ((DEBUG_INFO, "The FV which is measured by Tcg2Pei has the > size: > > > 0x%x\n", FvLength)); > > > } > > > > > > if (EFI_ERROR(Status)) { > > > diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > > > b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > > > index 30f985b6ea..3d361e8859 100644 > > > --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > > > +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > > > @@ -54,6 +54,7 @@ > > > MemoryAllocationLib > > > ReportStatusCodeLib > > > ResetSystemLib > > > + PrintLib > > > > > > [Guids] > > > gTcgEventEntryHobGuid > > > ## PRODUCES ## > > > HOB > > > @@ -74,6 +75,7 @@ > > > > > > [Pcd] > > > gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString ## > > > SOMETIMES_CONSUMES > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdTcgPfpMeasurementRevision > > ## > > > CONSUMES > > > gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## > > > CONSUMES > > > gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy ## > > > CONSUMES > > > gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy ## > > > SOMETIMES_CONSUMES > > > -- > > > 2.19.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#52876): https://edk2.groups.io/g/devel/message/52876 Mute This Topic: https://groups.io/mt/69344973/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
