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

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

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>
> ---
>  .../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
>        //
>