Re: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support.

["Yao, Jiewen" <jiewen.yao@intel.com>]

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.wang@intel.com>
> Sent: Monday, January 6, 2020 1:33 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@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.wang@intel.com>
> 
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Tuesday, December 31, 2019 2:44 PM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@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.wang@intel.com>
> > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > Signed-off-by: Jiewen Yao <jiewen.yao@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