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

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

I see. Will do that.

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Monday, January 6, 2020 1:58 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.
> 
> 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.yao@intel.com>
> > Sent: Monday, January 06, 2020 1:53 PM
> > To: Wang, Jian J <jian.j.wang@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.
> >
> > 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