Hi, Sami Please see my comments inline. From: Sami Mujawar <sami.muja...@arm.com> Sent: Monday, November 8, 2021 10:18 PM To: Xu, Min M <min.m...@intel.com>; devel@edk2.groups.io Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; nd <n...@arm.com> 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.muja...@arm.com><mailto:sami.muja...@arm.com> 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] -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#83465): https://edk2.groups.io/g/devel/message/83465 Mute This Topic: https://groups.io/mt/86881261/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
