From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web09.141.1578290264812464718 for ; Sun, 05 Jan 2020 21:57:44 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: jian.j.wang@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jan 2020 21:57:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,401,1571727600"; d="scan'208";a="420604330" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga005.fm.intel.com with ESMTP; 05 Jan 2020 21:57:44 -0800 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 21:57:43 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 21:57:43 -0800 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.210]) by shsmsx102.ccr.corp.intel.com ([169.254.2.202]) with mapi id 14.03.0439.000; Mon, 6 Jan 2020 13:57:41 +0800 From: "Wang, Jian J" To: "Yao, Jiewen" , "devel@edk2.groups.io" CC: "Zhang, Chao B" Subject: Re: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. Thread-Topic: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. Thread-Index: AQHVv6XNz5KnHOHVHk6gwootPQYvMqfdI4CAgAAH/aCAAACagA== Date: Mon, 6 Jan 2020 05:57:40 +0000 Message-ID: References: <20191231064412.22988-1-jiewen.yao@intel.com> <20191231064412.22988-7-jiewen.yao@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F8D08C5@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F8D08C5@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYWYyNjA3YmMtOWM4Zi00ODI1LTgyMWItMTQwZGYyZTk3YzRiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiK0pEN0FxN1BPTW1PVDNBXC90QWRKTzBPcDd1Y0t5RUJTbnRQMGNLb2pPREJMdmY3MEdpK2FJcXdRNHpSWDhCRXIifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 som= etimes. Regards, Jian > -----Original Message----- > From: Yao, Jiewen > Sent: Monday, January 06, 2020 1:53 PM > To: Wang, Jian J ; devel@edk2.groups.io > Cc: Zhang, Chao B > Subject: RE: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. >=20 > Hi Jian > I purposely put the line there, because I want to group all the rev 0 cod= e > together and rev 105 code together in if statement. I don't want to move = that > particular line to else statement. >=20 > Thank you > Yao Jiewen >=20 > > -----Original Message----- > > From: Wang, Jian J > > Sent: Monday, January 6, 2020 1:33 PM > > To: Yao, Jiewen ; devel@edk2.groups.io > > Cc: Zhang, Chao B > > 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 > > > > > > > -----Original Message----- > > > From: Yao, Jiewen > > > Sent: Tuesday, December 31, 2019 2:44 PM > > > To: devel@edk2.groups.io > > > Cc: Wang, Jian J ; Zhang, Chao B > > > > > > Subject: [PATCH 6/6] SecurityPkg/Tcg2Pei: Add TCG PFP 105 support. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2439 > > > > > > Use EV_EFI_PLATFORM_FIRMWARE_BLOB2 if the TCG PFP revision is >=3D 10= 5. > > > Use FvName as the description for the FV. > > > > > > Cc: Jian J Wang > > > Cc: Chao Zhang > > > Signed-off-by: Jiewen Yao > > > --- > > > 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 > > > #include > > > #include > > > +#include > > > > > > #define PERF_ID_TCG2_PEI 0x3080 > > > > > > @@ -78,6 +79,18 @@ EFI_PLATFORM_FIRMWARE_BLOB > > > *mMeasuredChildFvInfo; > > > UINT32 mMeasuredMaxChildFvIndex =3D 0; > > > UINT32 mMeasuredChildFvIndex =3D 0; > > > > > > +#pragma pack (1) > > > + > > > +#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX- > > > XXXXXXXXXXXX)" > > > +typedef struct { > > > + UINT8 BlobDescriptionSize; > > > + UINT8 BlobDescription[sizeof(FV_HANDOF= F_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 >=3D MAX_ADDRESS) { > > > + return NULL; > > > + } > > > + if (FvLength >=3D MAX_ADDRESS - FvBase) { > > > + return NULL; > > > + } > > > + if (FvLength < sizeof(EFI_FIRMWARE_VOLUME_HEADER)) { > > > + return NULL; > > > + } > > > + > > > + FvHeader =3D (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 =3D (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 successf= ully. > > > @@ -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 =3D 0; > > > TcgEventHdr.EventType =3D EV_EFI_PLATFORM_FIRMWARE_BLOB; > > > TcgEventHdr.EventSize =3D sizeof (FvBlob); > > > + EventData =3D &FvBlob; > > > + > > > > I'd suggest to put above code in 'else' block to make code easier to re= ad, > > i.e. FvBlob for revision less than 105 and FvBlob2 for later ones. > > > > Regards, > > Jian > > > > > + if (PcdGet32(PcdTcgPfpMeasurementRevision) >=3D > > > TCG_EfiSpecIDEventStruct_SPEC_ERRATA_TPM2_REV_105) { > > > + FvBlob2.BlobDescriptionSize =3D sizeof(FvBlob2.BlobDescription); > > > + CopyMem (FvBlob2.BlobDescription, FV_HANDOFF_TABLE_DESC, > > > sizeof(FvBlob2.BlobDescription)); > > > + FvName =3D GetFvName (FvBase, FvLength); > > > + if (FvName !=3D NULL) { > > > + AsciiSPrint ((CHAR8 *)FvBlob2.BlobDescription, > > > sizeof(FvBlob2.BlobDescription), "Fv(%g)", FvName); > > > + } > > > + FvBlob2.BlobBase =3D FvBlob.BlobBase; > > > + FvBlob2.BlobLength =3D FvBlob.BlobLength; > > > + TcgEventHdr.EventType =3D EV_EFI_PLATFORM_FIRMWARE_BLOB2; > > > + TcgEventHdr.EventSize =3D sizeof (FvBlob2); > > > + EventData =3D &FvBlob2; > > > + } > > > > > > if (Tpm2HashMask =3D=3D 0) { > > > // > > > @@ -583,9 +656,9 @@ MeasureFvImage ( > > > ); > > > > > > if (!EFI_ERROR(Status)) { > > > - Status =3D LogHashEvent (&DigestList, &TcgEventHdr, (UINT8*) = &FvBlob); > > > - DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & lo= gged > > by > > > Tcg2Pei starts at: 0x%x\n", FvBlob.BlobBase)); > > > - DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & lo= gged > > by > > > Tcg2Pei has the size: 0x%x\n", FvBlob.BlobLength)); > > > + Status =3D LogHashEvent (&DigestList, &TcgEventHdr, EventData= ); > > > + DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & lo= gged > > by > > > Tcg2Pei starts at: 0x%x\n", FvBase)); > > > + DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & lo= gged > > by > > > Tcg2Pei has the size: 0x%x\n", FvLength)); > > > } else if (Status =3D=3D EFI_DEVICE_ERROR) { > > > BuildGuidHob (&gTpmErrorHobGuid,0); > > > REPORT_STATUS_CODE ( > > > @@ -599,13 +672,13 @@ MeasureFvImage ( > > > // > > > Status =3D 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