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

["Wang, Jian J" <jian.j.wang@intel.com>]

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