Re: [PATCH 2/2] SecurityPkg Tcg2Dxe: Filter inactive digest in event2 log from PEI HOB

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

That's good idea. I like both. :)

From: Zeng, Star
Sent: Friday, November 18, 2016 9:31 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 2/2] SecurityPkg Tcg2Dxe: Filter inactive digest in event2 log from PEI HOB

On 2016/11/17 20:19, Yao, Jiewen wrote:
> Hi Star
> 1) I am a little confused on below description.
> +  @param[in,out] Buffer             Buffer to hold TPML_DIGEST_VALUES.
>
> I think the buffer is the *TPML_DIGEST_VALUES compact binary buffer*, instead of *TPML_DIGEST_VALUES*, right?
> I suggest we describe it clearly.

Oh, yes. The description was just copied from CopyDigestListToBuffer ()
of Tpm2CommandLib, do you think we also need to update the description
for CopyDigestListToBuffer ().

>
> 2) I think the FILTER is great to report ERROR if the TCG event hob producer makes mistake.
> Do you think it is worthy to add more stronger check that: All required TCG event log are reported ?

Yes, It can be done by adding a parameter to CopyDigestListBinToBuffer
() and compare if HashAlgorithmMaskCopied equals to HashAlgorithmMask to
know if all required TCG evente log are reported.

VOID *
CopyDigestListBinToBuffer (
   IN OUT VOID                       *Buffer,
   IN VOID                           *DigestListBin,
   IN UINT32                         HashAlgorithmMask,
*  OUT UINT32                        *HashAlgorithmMaskCopied*
   )

Thanks,
Star

>
>
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Thursday, November 17, 2016 5:29 PM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>> Cc: Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>;
>> Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>> Subject: [PATCH 2/2] SecurityPkg Tcg2Dxe: Filter inactive digest in event2 log
>> from PEI HOB
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>> Cc: Chao Zhang <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>> ---
>>  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c | 80
>> +++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
>> b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
>> index db8d662f80dc..d5a32307db6e 100644
>> --- a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
>> +++ b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
>> @@ -898,6 +898,56 @@ GetDigestListBinSize (
>>  }
>>
>>  /**
>> +  Copy TPML_DIGEST_VALUES compact binary into a buffer
>> +
>> +  @param[in,out] Buffer             Buffer to hold
>> TPML_DIGEST_VALUES.
>> +  @param[in]     DigestListBin      TPML_DIGEST_VALUES compact
>> binary buffer.
>> +  @param[in]     HashAlgorithmMask  HASH bits corresponding to the
>> desired digests to copy.
>> +
>> +  @return The end of buffer to hold TPML_DIGEST_VALUES.
>> +**/
>> +VOID *
>> +CopyDigestListBinToBuffer (
>> +  IN OUT VOID                       *Buffer,
>> +  IN VOID                           *DigestListBin,
>> +  IN UINT32                         HashAlgorithmMask
>> +  )
>> +{
>> +  UINTN         Index;
>> +  UINT16        DigestSize;
>> +  UINT32        Count;
>> +  TPMI_ALG_HASH HashAlg;
>> +  UINT32        DigestListCount;
>> +  UINT32        *DigestListCountPtr;
>> +
>> +  DigestListCountPtr = (UINT32 *) Buffer;
>> +  DigestListCount = 0;
>> +
>> +  Count = ReadUnaligned32 (DigestListBin);
>> +  Buffer = (UINT8 *)Buffer + sizeof(Count);
>> +  DigestListBin = (UINT8 *)DigestListBin + sizeof(Count);
>> +  for (Index = 0; Index < Count; Index++) {
>> +    HashAlg = ReadUnaligned16 (DigestListBin);
>> +    DigestListBin = (UINT8 *)DigestListBin + sizeof(HashAlg);
>> +    DigestSize = GetHashSizeFromAlgo (HashAlg);
>> +
>> +    if (IsHashAlgSupportedInHashAlgorithmMask(HashAlg,
>> HashAlgorithmMask)) {
>> +      CopyMem (Buffer, &HashAlg, sizeof(HashAlg));
>> +      Buffer = (UINT8 *)Buffer + sizeof(HashAlg);
>> +      CopyMem (Buffer, DigestListBin, DigestSize);
>> +      Buffer = (UINT8 *)Buffer + DigestSize;
>> +      DigestListCount++;
>> +    } else {
>> +      DEBUG ((EFI_D_ERROR, "WARNING: CopyDigestListBinToBuffer
>> Event log has HashAlg unsupported by PCR bank (0x%x)\n", HashAlg));
>> +    }
>> +    DigestListBin = (UINT8 *)DigestListBin + DigestSize;
>> +  }
>> +  WriteUnaligned32 (DigestListCountPtr, DigestListCount);
>> +
>> +  return Buffer;
>> +}
>> +
>> +/**
>>    Add a new entry to the Event Log.
>>
>>    @param[in]     DigestList    A list of digest.
>> @@ -1317,8 +1367,12 @@ SetupEventLog (
>>    EFI_PEI_HOB_POINTERS            GuidHob;
>>    EFI_PHYSICAL_ADDRESS            Lasa;
>>    UINTN                           Index;
>> +  VOID                            *DigestListBin;
>> +  TPML_DIGEST_VALUES              TempDigestListBin;
>>    UINT32                          DigestListBinSize;
>> +  UINT8                           *Event;
>>    UINT32                          EventSize;
>> +  UINT32                          *EventSizePtr;
>>    TCG_EfiSpecIDEventStruct        *TcgEfiSpecIdEventStruct;
>>    UINT8
>> TempBuf[sizeof(TCG_EfiSpecIDEventStruct) + sizeof(UINT32) +
>> (HASH_COUNT * sizeof(TCG_EfiSpecIdEventAlgorithmSize)) + sizeof(UINT8)];
>>    TCG_PCR_EVENT_HDR               FirstPcrEvent;
>> @@ -1497,7 +1551,8 @@ SetupEventLog (
>>        Status = EFI_SUCCESS;
>>        while (!EFI_ERROR (Status) &&
>>               (GuidHob.Raw = GetNextGuidHob
>> (mTcg2EventInfo[Index].EventGuid, GuidHob.Raw)) != NULL) {
>> -        TcgEvent    = GET_GUID_HOB_DATA (GuidHob.Guid);
>> +        TcgEvent    = AllocateCopyPool (GET_GUID_HOB_DATA_SIZE
>> (GuidHob.Guid), GET_GUID_HOB_DATA (GuidHob.Guid));
>> +        ASSERT (TcgEvent != NULL);
>>          GuidHob.Raw = GET_NEXT_HOB (GuidHob);
>>          switch (mTcg2EventInfo[Index].LogFormat) {
>>          case EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2:
>> @@ -1510,17 +1565,34 @@ SetupEventLog (
>>                       );
>>            break;
>>          case EFI_TCG2_EVENT_LOG_FORMAT_TCG_2:
>> -          DigestListBinSize = GetDigestListBinSize ((UINT8 *)TcgEvent +
>> sizeof(TCG_PCRINDEX) + sizeof(TCG_EVENTTYPE));
>> -          CopyMem (&EventSize, (UINT8 *)TcgEvent +
>> sizeof(TCG_PCRINDEX) + sizeof(TCG_EVENTTYPE) + DigestListBinSize,
>> sizeof(UINT32));
>> +          DigestListBin = (UINT8 *)TcgEvent + sizeof(TCG_PCRINDEX) +
>> sizeof(TCG_EVENTTYPE);
>> +          DigestListBinSize = GetDigestListBinSize (DigestListBin);
>> +          //
>> +          // Save event size.
>> +          //
>> +          CopyMem (&EventSize, (UINT8 *)DigestListBin +
>> DigestListBinSize, sizeof(UINT32));
>> +          Event = (UINT8 *)DigestListBin + DigestListBinSize +
>> sizeof(UINT32);
>> +          //
>> +          // Filter inactive digest in the event2 log from PEI HOB.
>> +          //
>> +          CopyMem (&TempDigestListBin, DigestListBin,
>> GetDigestListBinSize (DigestListBin));
>> +          EventSizePtr = CopyDigestListBinToBuffer (DigestListBin,
>> &TempDigestListBin, mTcgDxeData.BsCap.ActivePcrBanks);
>> +          //
>> +          // Restore event size.
>> +          //
>> +          CopyMem (EventSizePtr, &EventSize, sizeof(UINT32));
>> +          DigestListBinSize = GetDigestListBinSize (DigestListBin);
>> +
>>            Status = TcgDxeLogEvent (
>>                       mTcg2EventInfo[Index].LogFormat,
>>                       TcgEvent,
>>                       sizeof(TCG_PCRINDEX) + sizeof(TCG_EVENTTYPE)
>> + DigestListBinSize + sizeof(UINT32),
>> -                     (UINT8 *)TcgEvent + sizeof(TCG_PCRINDEX) +
>> sizeof(TCG_EVENTTYPE) + DigestListBinSize + sizeof(UINT32),
>> +                     Event,
>>                       EventSize
>>                       );
>>            break;
>>          }
>> +        FreePool (TcgEvent);
>>        }
>>      }
>>    }
>> --
>> 2.7.0.windows.1