From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 58BAF81907 for ; Mon, 26 Dec 2016 19:19:13 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 26 Dec 2016 19:19:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,414,1477983600"; d="scan'208";a="23074245" Received: from shzintpr01.sh.intel.com (HELO [10.7.209.21]) ([10.239.4.80]) by orsmga002.jf.intel.com with ESMTP; 26 Dec 2016 19:19:11 -0800 To: "Zhang, Chao B" , edk2-devel@lists.01.org References: <1482803481-1108-1-git-send-email-chao.b.zhang@intel.com> Cc: jiewen.yao@intel.com, star.zeng@intel.com From: "Zeng, Star" Message-ID: <126f7f51-dc03-1feb-c9cb-8e28ea041be7@intel.com> Date: Tue, 27 Dec 2016 11:18:41 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1482803481-1108-1-git-send-email-chao.b.zhang@intel.com> 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:19:13 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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/SecurityPkg/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 BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -189,7 +189,7 @@ Tpm2GetCapabilityManufactureID ( > if (EFI_ERROR (Status)) { > return Status; > } > - *ManufactureId = SwapBytes32 (TpmCap.data.tpmProperties.tpmProperty->value); > + *ManufactureId = 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 table 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 = 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 = 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) == 0x00 || ((ManufacturerID >> 24) == 0x20)) { > + // > + // HID containing PNP ID "NNN####" > + // NNN is uppercase letter for Vendor ID specified by manufacturer > + // > + CopyMem(HID, &ManufacturerID, 3); > + } else { > + // > + // HID containing ACP ID "NNNN####" > + // NNNN is uppercase letter for Vendor ID specified by manufacturer > + // > + CopyMem(HID, &ManufacturerID, 4); > + PnpHID = FALSE; > + } > + } else { > + DEBUG ((EFI_D_ERROR, "Get TPM_PT_VENDOR_STRING_1 failed %x!\n", Status)); Should just return failure here? > + } > + > + Status = Tpm2GetCapabilityFirmwareVersion(&FirmwareVersion1, &FirmwareVersion2); > + if (!EFI_ERROR(Status)) { > + DEBUG((EFI_D_INFO, "TPM_PT_FIRMWARE_VERSION_1 0x%x\n", FirmwareVersion1)); > + DEBUG((EFI_D_INFO, "TPM_PT_FIRMWARE_VERSION_2 0x%x\n", FirmwareVersion2)); > + // > + // #### is Firmware Version 1 > + // > + if (PnpHID) { > + AsciiSPrint(HID + 3, TPM_HID_PNP_SIZE - 3, "%02d%02d", ((FirmwareVersion1 & 0xFFFF0000) >> 16), (FirmwareVersion1 && 0x0000FFFF)); > + } else { > + AsciiSPrint(HID + 4, TPM_HID_ACPI_SIZE - 4, "%02d%02d", ((FirmwareVersion1 & 0xFFFF0000) >> 16), (FirmwareVersion1 && 0x0000FFFF)); > + } > + > + } else { > + DEBUG ((EFI_D_ERROR, "Get TPM_PT_FIRMWARE_VERSION_X failed %x!\n", Status)); Should just return failure here? > + } > + > + // > + // Patch HID in ASL code before loading the SSDT. > + // > + for (DataPtr = (UINT8 *)(Table + 1); > + DataPtr <= (UINT8 *) ((UINT8 *) Table + Table->Length - TPM_HID_PNP_SIZE); > + DataPtr += 1) { > + if (AsciiStrCmp((CHAR8 *)DataPtr, TPM_HID_TAG) == 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 = 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, EITHER 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 "NNN0000" > +#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/Tcg2Smm/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/Tpm.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 > // >