From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6C93B8190B for ; Mon, 26 Dec 2016 19:35:11 -0800 (PST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP; 26 Dec 2016 19:35:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,414,1477983600"; d="scan'208,217";a="43533034" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga004.jf.intel.com with ESMTP; 26 Dec 2016 19:35:10 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 26 Dec 2016 19:35:10 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 26 Dec 2016 19:35:09 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.177]) with mapi id 14.03.0248.002; Tue, 27 Dec 2016 11:35:07 +0800 From: "Yao, Jiewen" To: "Zhang, Chao B" , "Zeng, Star" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] SecurityPkg: Tcg2Smm: TPM2 Vendor specific HID Thread-Index: AQHSX+PC3i+Yew46yE+j6Q6m57BRIqEamfSAgAADbYCAAIcfQA== Date: Tue, 27 Dec 2016 03:35:06 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A8D5842@shsmsx102.ccr.corp.intel.com> References: <1482803481-1108-1-git-send-email-chao.b.zhang@intel.com> <126f7f51-dc03-1feb-c9cb-8e28ea041be7@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH] SecurityPkg: Tcg2Smm: TPM2 Vendor specific HID X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Dec 2016 03:35:11 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Maybe still use "MSFT0101" as default value, if we fail to get Manufacturer= ID? From: Zhang, Chao B Sent: Tuesday, December 27, 2016 11:31 AM To: Zeng, Star ; edk2-devel@lists.01.org Cc: Yao, Jiewen Subject: RE: [edk2] [PATCH] SecurityPkg: Tcg2Smm: TPM2 Vendor specific HID Star: Thanks for your comments. 1. For error handling, ManufacturerID & Version failure is not consid= ered to cause fatal break. We can still use default "NNN0000" value. 2. I will update the comments for UpdateHID 3. EFI_NOT_FOUND is not expected here, but I think it is reasonable e= rror handling if user override TPM.asl. Thanks & Best regards Chao Zhang From: Zeng, Star Sent: Tuesday, December 27, 2016 11:19 AM To: Zhang, Chao B; edk2-devel@lists.01.org Cc: Yao, Jiewen; Zeng, Star Subject: Re: [edk2] [PATCH] SecurityPkg: Tcg2Smm: TPM2 Vendor specific HID Chao, Minor comments inline. On 2016/12/27 9:51, Zhang, Chao B wrote: > Update TPM2 HID using vendor ManufacturerID & FirmwareVersion1. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Chao Zhang > > --- > .../Library/Tpm2CommandLib/Tpm2Capability.c | 4 +- > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 102 +++++++++++++++= ++++++ > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.h | 10 +- > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf | 2 +- > SecurityPkg/Tcg/Tcg2Smm/Tpm.asl | 10 +- > 5 files changed, 122 insertions(+), 6 deletions(-) > > diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Capability.c b/Securi= tyPkg/Library/Tpm2CommandLib/Tpm2Capability.c > index 9aab17f..79e80fb 100644 > --- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Capability.c > +++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Capability.c > @@ -1,7 +1,7 @@ > /** @file > Implement TPM2 Capability related command. > > -Copyright (c) 2013, Intel Corporation. All rights reserved.
> +Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D License > which accompanies this distribution. The full text of the license may b= e found at > @@ -189,7 +189,7 @@ Tpm2GetCapabilityManufactureID ( > if (EFI_ERROR (Status)) { > return Status; > } > - *ManufactureId =3D SwapBytes32 (TpmCap.data.tpmProperties.tpmProperty-= >value); > + *ManufactureId =3D TpmCap.data.tpmProperties.tpmProperty->value; > > return EFI_SUCCESS; > } > diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c b/SecurityPkg/Tcg/Tcg2Smm/= Tcg2Smm.c > index d02123d..addb302 100644 > --- a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c > +++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c > @@ -303,6 +303,103 @@ UpdatePPVersion ( > } > > /** > + Patch TPM2 device HID string. The initial string tag in TPM2 ACPI tab= le is "NNN0000". > + > + @param[in, out] Table The TPM2 SSDT ACPI table. > + > + @return HID Update status. > + > +**/ > +EFI_STATUS > +UpdateHID ( > + EFI_ACPI_DESCRIPTION_HEADER *Table > + ) > +{ > + EFI_STATUS Status; > + UINT8 *DataPtr; > + CHAR8 HID[TPM_HID_ACPI_SIZE]; > + UINT32 ManufacturerID; > + UINT32 FirmwareVersion1; > + UINT32 FirmwareVersion2; > + BOOLEAN PnpHID; > + > + PnpHID =3D TRUE; > + > + // > + // Initialize HID with Default PNP string > + // > + ZeroMem(HID, TPM_HID_ACPI_SIZE); > + CopyMem(HID, TPM_HID_TAG, TPM_HID_PNP_SIZE); > + > + // > + // Get Manufacturer ID > + // > + Status =3D Tpm2GetCapabilityManufactureID(&ManufacturerID); > + if (!EFI_ERROR(Status)) { > + DEBUG((EFI_D_INFO, "TPM_PT_VENDOR_STRING_1 0x%08x\n", ManufacturerID= )); > + // > + // ManfacturerID defined in TCG Vendor ID Registry > + // may tailed with 0x00 or 0x20 > + // > + if ((ManufacturerID >> 24) =3D=3D 0x00 || ((ManufacturerID >> 24) = =3D=3D 0x20)) { > + // > + // HID containing PNP ID "NNN####" > + // NNN is uppercase letter for Vendor ID specified by manufactur= er > + // > + CopyMem(HID, &ManufacturerID, 3); > + } else { > + // > + // HID containing ACP ID "NNNN####" > + // NNNN is uppercase letter for Vendor ID specified by manufactu= rer > + // > + CopyMem(HID, &ManufacturerID, 4); > + PnpHID =3D FALSE; > + } > + } else { > + DEBUG ((EFI_D_ERROR, "Get TPM_PT_VENDOR_STRING_1 failed %x!\n", Stat= us)); Should just return failure here? > + } > + > + Status =3D Tpm2GetCapabilityFirmwareVersion(&FirmwareVersion1, &Firmwa= reVersion2); > + if (!EFI_ERROR(Status)) { > + DEBUG((EFI_D_INFO, "TPM_PT_FIRMWARE_VERSION_1 0x%x\n", FirmwareVersi= on1)); > + DEBUG((EFI_D_INFO, "TPM_PT_FIRMWARE_VERSION_2 0x%x\n", FirmwareVersi= on2)); > + // > + // #### is Firmware Version 1 > + // > + if (PnpHID) { > + AsciiSPrint(HID + 3, TPM_HID_PNP_SIZE - 3, "%02d%02d", ((FirmwareV= ersion1 & 0xFFFF0000) >> 16), (FirmwareVersion1 && 0x0000FFFF)); > + } else { > + AsciiSPrint(HID + 4, TPM_HID_ACPI_SIZE - 4, "%02d%02d", ((Firmware= Version1 & 0xFFFF0000) >> 16), (FirmwareVersion1 && 0x0000FFFF)); > + } > + > + } else { > + DEBUG ((EFI_D_ERROR, "Get TPM_PT_FIRMWARE_VERSION_X failed %x!\n", S= tatus)); Should just return failure here? > + } > + > + // > + // Patch HID in ASL code before loading the SSDT. > + // > + for (DataPtr =3D (UINT8 *)(Table + 1); > + DataPtr <=3D (UINT8 *) ((UINT8 *) Table + Table->Length - TPM_HID= _PNP_SIZE); > + DataPtr +=3D 1) { > + if (AsciiStrCmp((CHAR8 *)DataPtr, TPM_HID_TAG) =3D=3D 0) { > + if (PnpHID) { > + CopyMem(DataPtr, HID, TPM_HID_PNP_SIZE); > + } else { > + // > + // NOOP will be patched to '\0' > + // > + CopyMem(DataPtr, HID, TPM_HID_ACPI_SIZE); > + } > + DEBUG((EFI_D_INFO, "TPM2 ACPI _HID updated to %a\n", HID)); > + return Status; > + } > + } > + > + return EFI_NOT_FOUND; Since the for loop above can find out TPM_HID_TAG definitly, can the code run to here according to current code logic? > +} > + > +/** > Initialize and publish TPM items in ACPI table. > > @retval EFI_SUCCESS The TCG ACPI table is published successfully= . > @@ -336,6 +433,11 @@ PublishAcpiTable ( > ASSERT_EFI_ERROR (Status); > > // > + // Update Table version before measuring it to PCR Does the comment here need to be updated? Thanks, Star > + // > + Status =3D UpdateHID(Table); > + > + // > // Measure to PCR[0] with event EV_POST_CODE ACPI DATA > // > TpmMeasureAndLogData( > diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.h b/SecurityPkg/Tcg/Tcg2Smm/= Tcg2Smm.h > index 0b09032..18e8bfc 100644 > --- a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.h > +++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.h > @@ -35,9 +35,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EIT= HER EXPRESS OR IMPLIED. > #include > #include > #include > -#include > +#include > #include > #include > +#include > > #include > > @@ -94,4 +95,11 @@ typedef struct { > #define PHYSICAL_PRESENCE_VERSION_TAG "$PV" > #define PHYSICAL_PRESENCE_VERSION_SIZE 4 > > +// > +// PNP _HID for TPM2 device > +// > +#define TPM_HID_TAG "NNN0= 000" > +#define TPM_HID_PNP_SIZE 8 > +#define TPM_HID_ACPI_SIZE 9 > + > #endif // __TCG_SMM_H__ > diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf b/SecurityPkg/Tcg/Tcg2Sm= m/Tcg2Smm.inf > index 0de4fce..8c823d6 100644 > --- a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf > +++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf > @@ -48,7 +48,7 @@ > DebugLib > DxeServicesLib > TpmMeasurementLib > - Tpm2DeviceLib > + Tpm2CommandLib > Tcg2PhysicalPresenceLib > IoLib > > diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tpm.asl b/SecurityPkg/Tcg/Tcg2Smm/Tp= m.asl > index 2083a3e..4128684 100644 > --- a/SecurityPkg/Tcg/Tcg2Smm/Tpm.asl > +++ b/SecurityPkg/Tcg/Tcg2Smm/Tpm.asl > @@ -30,8 +30,14 @@ DefinitionBlock ( > // > // TCG2 > // > - Name (_HID, "MSFT0101") > - > + Name (_HID, "NNN0000") > + // > + // Reserve 1 more byte for ACPI HID > + // > + Noop > + > + Name (_CID, "MSFT0101") > + > // > // Readable name of this device, don't know if this way is correct= yet > // >