From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 EBC36818A8 for ; Mon, 26 Dec 2016 20:39:30 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 26 Dec 2016 20:39:30 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,414,1477983600"; d="scan'208";a="1076560269" Received: from shzintpr01.sh.intel.com (HELO [10.7.209.18]) ([10.239.4.80]) by orsmga001.jf.intel.com with ESMTP; 26 Dec 2016 20:39:29 -0800 To: "Zhang, Chao B" , "edk2-devel@lists.01.org" References: <1482803481-1108-1-git-send-email-chao.b.zhang@intel.com> <126f7f51-dc03-1feb-c9cb-8e28ea041be7@intel.com> Cc: "Yao, Jiewen" , star.zeng@intel.com From: "Zeng, Star" Message-ID: <628e1c11-1814-1d7a-744d-90abb5340446@intel.com> Date: Tue, 27 Dec 2016 12:38:59 +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: 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 04:39:31 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 2016/12/27 11:30, Zhang, Chao B wrote: > Star: > Thanks for your comments. > > 1. For error handling, ManufacturerID & Version failure is not considered to cause fatal break. We can still use default "NNN0000" value. I mean why still to update the default _HID value in TPM.asl if getting ManufacturerID or Version is failed? > > 2. I will update the comments for UpdateHID > > 3. EFI_NOT_FOUND is not expected here, but I think it is reasonable error handling if user override TPM.asl. If user override TPM.asl, for example the default _HID value in TPM.asl, shouldn't the value of TPM_HID_TAG be updated to match the default _HID value in TPM.asl? If not, the UpdateHID() function couldn't work as the for loop couldn't find the tag. If yes, how does the code run to "return EFI_NOT_FOUND"? Anyway, why does the UpdateHID() function need return status? Thanks, Star > > > > > 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/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 >> // >>