Re: [PATCH] SecurityPkg: Tcg2Smm: TPM2 Vendor specific HID

["Zeng, Star" <star.zeng@intel.com>]

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 <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>> ---
>>  .../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. <BR>
>> +Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved. <BR>
>>  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 <Library/UefiBootServicesTableLib.h>
>>  #include <Library/DxeServicesLib.h>
>>  #include <Library/TpmMeasurementLib.h>
>> -#include <Library/Tpm2DeviceLib.h>
>> +#include <Library/Tpm2CommandLib.h>
>>  #include <Library/Tcg2PhysicalPresenceLib.h>
>>  #include <Library/IoLib.h>
>> +#include <Library/PrintLib.h>
>>
>>  #include <IndustryStandard/TpmPtp.h>
>>
>> @@ -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
>>        //
>>