Re: [PATCH V5 2/3] SecurityPkg: Support CcMeasurementProtocol in DxeTpm2MeasureBootLib


Min Xu
 

Hi, Sami

Please see my comments inline.

 

From: Sami Mujawar <sami.mujawar@...>
Sent: Monday, November 8, 2021 10:18 PM
To: Xu, Min M <min.m.xu@...>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@...>; Liming Gao <gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>; Yao, Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Gerd Hoffmann <kraxel@...>; nd <nd@...>
Subject: Re: [PATCH V5 2/3] SecurityPkg: Support CcMeasurementProtocol in DxeTpm2MeasureBootLib

 

Hi Min,

Thank you for the updated patch.

I have a minor suggestion marked inline as [SAMI]. Otherwise, this patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@...>

Regards,

Sami Mujawar

 

On 07/11/2021 12:35 PM, Min Xu wrote:

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
 
   //
-  Status = Tcg2Protocol->HashLogExtendEvent (
-             Tcg2Protocol,
-             0,
-             (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
-             (UINT64) EventSize,
-             Tcg2Event
-             );
-  if (!EFI_ERROR (Status)) {
-    mTcg2MeasureGptCount++;
+  if (CcProtocol != NULL) {
+    //
+    // EFI_CC_EVENT share the same data structure with EFI_TCG2_EVENT
+    // except the MrIndex and PCRIndex in Header.

[SAMI] Since we are now sharing the same data structures between TCG2 and CC, would it be better to typedef the CC data structures?

This would also avoid potential issues should any one of the data structures were to be changed in the future.
I think EFI_CC_EVENT, EFI_CC_EVENT_HEADER and EFI_CC_MR_INDEX could be type defined. Similarly, EFI_CC_EVENT_HEADER_VERSION could
also be defined to the TCG2 equivalent.

If not then we should at least add an ASSERT () to check if the size of EFI_CC_EVENT and EFI_TCG2_EVENT is not different.

[/SAMI]

 

[Min]

In the original version EFI_CC_EVENT is type defined as EFI_TCG2_EVENT. But in the definition of EFI_TCG2_EVENT / EFI_TCG2_EVENT_HEADER,  field of PCRIndex will be weird when it is used in CC Measurement. So we have to define a new data structure EFI_CC_EVENT / EFI_CC_EVENT_HEADER.

typedef struct {

  UINT32                        HeaderSize;

  UINT16                        HeaderVersion;

  TCG_PCRINDEX         PCRIndex;  ß It is PCR specific

  TCG_EVENTTYPE       EventType;

} EFI_TCG2_EVENT_HEADER;

 

I think we can add ASSERT (sizeof (EFI_CC_EVENT) == sizeof (EFI_TCG2_EVENT)).

[/Min]

Join devel@edk2.groups.io to automatically receive all group messages.