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 (#52875): https://edk2.groups.io/g/devel/message/52875 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] -=-=-=-=-=-=-=-=-=-=-=-
