Re: [edk2-devel] [PATCH v2 3/9] SecurityPkg/DxeTpmMeasurementLib: Add new API.

["Wang, Jian J" <jian.j.wang@intel.com>]

Qi,

Comments below.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Qi Zhang
> Sent: Thursday, August 06, 2020 8:34 AM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Zhang, Qi1 <qi1.zhang@intel.com>
> Subject: [edk2-devel] [PATCH v2 3/9] SecurityPkg/DxeTpmMeasurementLib: Add
> new API.
> 
> From: Jiewen Yao <jiewen.yao@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2376
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Qi Zhang <qi1.zhang@intel.com>
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  .../DxeTpmMeasurementLib.inf                  |   6 +-
>  .../DxeTpmMeasurementLib/EventLogRecord.c     | 218 ++++++++++++++++++
>  2 files changed, 223 insertions(+), 1 deletion(-)
>  create mode 100644
> SecurityPkg/Library/DxeTpmMeasurementLib/EventLogRecord.c
> 
> diff --git
> a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> index 7d41bc41f9..39448f8ee8 100644
> --- a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> +++
> b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> @@ -4,7 +4,7 @@
>  #  This library provides TpmMeasureAndLogData() to measure and log data, and
> 
>  #  extend the measurement result into a specific PCR.
> 
>  #
> 
> -# Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +# Copyright (c) 2012 - 2020, Intel Corporation. All rights reserved.<BR>
> 
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #
> 
>  ##
> 
> @@ -26,6 +26,7 @@
> 
> 
>  [Sources]
> 
>    DxeTpmMeasurementLib.c
> 
> +  EventLogRecord.c
> 
> 
> 
>  [Packages]
> 
>    MdePkg/MdePkg.dec
> 
> @@ -42,3 +43,6 @@
>  [Protocols]
> 
>    gEfiTcgProtocolGuid           ## SOMETIMES_CONSUMES
> 
>    gEfiTcg2ProtocolGuid          ## SOMETIMES_CONSUMES
> 
> +
> 
> +[Pcd]
> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdTcgPfpMeasurementRevision          ##
> CONSUMES
> 
> diff --git a/SecurityPkg/Library/DxeTpmMeasurementLib/EventLogRecord.c
> b/SecurityPkg/Library/DxeTpmMeasurementLib/EventLogRecord.c
> new file mode 100644
> index 0000000000..7b3726e44b
> --- /dev/null
> +++ b/SecurityPkg/Library/DxeTpmMeasurementLib/EventLogRecord.c
> @@ -0,0 +1,218 @@
> +/** @file
> 
> +  This library is used by other modules to measure data to TPM.
> 
> +
> 
> +Copyright (c) 2020, Intel Corporation. All rights reserved. <BR>
> 
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> +
> 
> +**/
> 
> +
> 
> +#include <PiDxe.h>
> 
> +
> 
> +#include <Library/BaseMemoryLib.h>
> 
> +#include <Library/DebugLib.h>
> 
> +#include <Library/ReportStatusCodeLib.h>
> 
> +#include <Library/HobLib.h>
> 
> +#include <Library/PcdLib.h>
> 
> +#include <Library/PrintLib.h>
> 
> +#include <Library/TpmMeasurementLib.h>
> 
> +
> 
> +#include <IndustryStandard/UefiTcgPlatform.h>
> 
> +
> 
> +#pragma pack (1)
> 
> +
> 
> +#define PLATFORM_FIRMWARE_BLOB_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX-
> XXXXXXXXXXXX)"
> 
> +typedef struct {
> 
> +  UINT8                             BlobDescriptionSize;
> 
> +  UINT8
> BlobDescription[sizeof(PLATFORM_FIRMWARE_BLOB_DESC)];
> 
> +  EFI_PHYSICAL_ADDRESS              BlobBase;
> 
> +  UINT64                            BlobLength;
> 
> +} PLATFORM_FIRMWARE_BLOB2_STRUCT;
> 
> +
> 
> +#define HANDOFF_TABLE_POINTER_DESC  "1234567890ABCDEF"
> 
> +typedef struct {
> 
> +  UINT8                             TableDescriptionSize;
> 
> +  UINT8
> TableDescription[sizeof(HANDOFF_TABLE_POINTER_DESC)];
> 
> +  UINT64                            NumberOfTables;
> 
> +  EFI_CONFIGURATION_TABLE           TableEntry[1];
> 
> +} HANDOFF_TABLE_POINTERS2_STRUCT;
> 
> +
> 
> +#pragma pack ()
> 
> +
> 
> +/**
> 
> +  Get the FvName from the FV header.
> 
> +
> 
> +  Causion: The FV is untrusted input.
> 
> +
> 
> +  @param[in]  FvBase            Base address of FV image.
> 
> +  @param[in]  FvLength          Length of FV image.
> 
> +
> 
> +  @return FvName pointer
> 
> +  @retval NULL   FvName is NOT found
> 
> +**/
> 
> +VOID *
> 
> +TpmMeasurementGetFvName (
> 
> +  IN EFI_PHYSICAL_ADDRESS           FvBase,
> 
> +  IN UINT64                         FvLength
> 
> +  )
> 
> +{
> 
> +  EFI_FIRMWARE_VOLUME_HEADER      *FvHeader;
> 
> +  EFI_FIRMWARE_VOLUME_EXT_HEADER  *FvExtHeader;
> 
> +
> 
> +  if (FvBase >= MAX_ADDRESS) {
> 
> +    return NULL;
> 
> +  }
> 
> +  if (FvLength >= MAX_ADDRESS - FvBase) {
> 
> +    return NULL;
> 
> +  }
> 
> +  if (FvLength < sizeof(EFI_FIRMWARE_VOLUME_HEADER)) {
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
> +  FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvBase;
> 
> +  if (FvHeader->Signature != EFI_FVH_SIGNATURE) {
> 
> +    return NULL;
> 
> +  }
> 
> +  if (FvHeader->ExtHeaderOffset < sizeof(EFI_FIRMWARE_VOLUME_HEADER)) {
> 
> +    return NULL;
> 
> +  }
> 
> +  if (FvHeader->ExtHeaderOffset +
> sizeof(EFI_FIRMWARE_VOLUME_EXT_HEADER) > FvLength) {
> 
> +    return NULL;
> 
> +  }
> 
> +  FvExtHeader = (EFI_FIRMWARE_VOLUME_EXT_HEADER *)(UINTN)(FvBase +
> FvHeader->ExtHeaderOffset);
> 
> +
> 
> +  return &FvExtHeader->FvName;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Mesure a FirmwareBlob.

'Mesure' -> 'Measure'

> 
> +
> 
> +  @param[in]  PcrIndex                PcrIndex of the measurment.

'measurment' -> 'measurement'

> 
> +  @param[in]  Descrption              Description for this FirmwareBlob.

'Descrption' -> 'Description'

> 
> +  @param[in]  FirmwareBlobBase        Base address of this FirmwareBlob.
> 
> +  @param[in]  FirmwareBlobLength      Size in bytes of this FirmwareBlob.
> 
> +
> 
> +  @retval EFI_SUCCESS           Operation completed successfully.
> 
> +  @retval EFI_UNSUPPORTED       TPM device not available.
> 
> +  @retval EFI_OUT_OF_RESOURCES  Out of memory.
> 
> +  @retval EFI_DEVICE_ERROR      The operation was unsuccessful.
> 
> +*/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +MeasureFirmwareBlob (
> 
> +  IN UINT32                         PcrIndex,
> 
> +  IN CHAR8                          *Description OPTIONAL,
> 
> +  IN EFI_PHYSICAL_ADDRESS           FirmwareBlobBase,
> 
> +  IN UINT64                         FirmwareBlobLength
> 
> +  )
> 
> +{
> 
> +  EFI_PLATFORM_FIRMWARE_BLOB        FvBlob;
> 
> +  PLATFORM_FIRMWARE_BLOB2_STRUCT    FvBlob2;
> 
> +  VOID                              *FvName;
> 
> +  UINT32                            EventType;
> 
> +  VOID                              *EventLog;
> 
> +  UINT32                            EventLogSize;
> 
> +  EFI_STATUS                        Status;
> 
> +
> 
> +  FvName = TpmMeasurementGetFvName (FirmwareBlobBase,
> FirmwareBlobLength);
> 
> +
> 
> +  if (((Description != NULL) || (FvName != NULL)) &&
> 
> +      (PcdGet32(PcdTcgPfpMeasurementRevision) >=
> TCG_EfiSpecIDEventStruct_SPEC_ERRATA_TPM2_REV_105)) {
> 
> +    ZeroMem (&FvBlob2, sizeof(FvBlob2));

It looks that clear the data structure is not necessary. Code below
will fill all fields in it. According to description of AsciiSPrint(), it also
produces NULL-terminated string. I see no reason to clear it in
advance.

> 
> +    if (Description != NULL) {
> 
> +      AsciiSPrint((CHAR8*)FvBlob2.BlobDescription,
> sizeof(FvBlob2.BlobDescription), "%a", Description);
> 
> +    } else {
> 
> +      AsciiSPrint((CHAR8*)FvBlob2.BlobDescription,
> sizeof(FvBlob2.BlobDescription), "Fv(%g)", FvName);
> 
> +    }
> 
> +
> 
> +    FvBlob2.BlobDescriptionSize = sizeof(FvBlob2.BlobDescription);
> 
> +    FvBlob2.BlobBase = FirmwareBlobBase;
> 
> +    FvBlob2.BlobLength = FirmwareBlobLength;
> 
> +
> 
> +    EventType = EV_EFI_PLATFORM_FIRMWARE_BLOB2;
> 
> +    EventLog = &FvBlob2;
> 
> +    EventLogSize = sizeof(FvBlob2);
> 
> +  } else {
> 
> +    FvBlob.BlobBase = FirmwareBlobBase;
> 
> +    FvBlob.BlobLength = FirmwareBlobLength;
> 
> +
> 
> +    EventType = EV_EFI_PLATFORM_FIRMWARE_BLOB;
> 
> +    EventLog = &FvBlob;
> 
> +    EventLogSize = sizeof(FvBlob);
> 
> +  }
> 
> +
> 
> +  Status = TpmMeasureAndLogData (
> 
> +             PcrIndex,
> 
> +             EventType,
> 
> +             EventLog,
> 
> +             EventLogSize,
> 
> +             (VOID*)(UINTN)FirmwareBlobBase,
> 
> +             FirmwareBlobLength
> 
> +             );
> 
> +
> 
> +  return Status;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Mesure a HandoffTable.

'Mesure' -> 'Measure'

> 
> +
> 
> +  @param[in]  PcrIndex                PcrIndex of the measurment.

'measurment' -> 'measurement'

> 
> +  @param[in]  Descrption              Description for this HandoffTable.

'Descrption' -> 'Description'

> 
> +  @param[in]  TableGuid               GUID of this HandoffTable.
> 
> +  @param[in]  TableAddress            Base address of this HandoffTable.
> 
> +  @param[in]  TableLength             Size in bytes of this HandoffTable.
> 
> +
> 
> +  @retval EFI_SUCCESS           Operation completed successfully.
> 
> +  @retval EFI_UNSUPPORTED       TPM device not available.
> 
> +  @retval EFI_OUT_OF_RESOURCES  Out of memory.
> 
> +  @retval EFI_DEVICE_ERROR      The operation was unsuccessful.
> 
> +*/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +MeasureHandoffTable (
> 
> +  IN UINT32                         PcrIndex,
> 
> +  IN CHAR8                          *Description OPTIONAL,
> 
> +  IN EFI_GUID                       *TableGuid,
> 
> +  IN VOID                           *TableAddress,
> 
> +  IN UINTN                          TableLength
> 
> +  )
> 
> +{
> 
> +  EFI_HANDOFF_TABLE_POINTERS        HandoffTables;
> 
> +  HANDOFF_TABLE_POINTERS2_STRUCT    HandoffTables2;
> 
> +  UINT32                            EventType;
> 
> +  VOID                              *EventLog;
> 
> +  UINT32                            EventLogSize;
> 
> +  EFI_STATUS                        Status;
> 
> +
> 
> +  if ((Description != NULL) &&
> 
> +      (PcdGet32(PcdTcgPfpMeasurementRevision) >=
> TCG_EfiSpecIDEventStruct_SPEC_ERRATA_TPM2_REV_105)) {
> 
> +    ZeroMem (&HandoffTables2, sizeof(HandoffTables2));

The same as before. I see no reason to clear the data in advance.

> 
> +    AsciiSPrint((CHAR8*)HandoffTables2.TableDescription,
> sizeof(HandoffTables2.TableDescription), "%a", Description);
> 
> +
> 
> +    HandoffTables2.TableDescriptionSize =
> sizeof(HandoffTables2.TableDescription);
> 
> +    HandoffTables2.NumberOfTables = 1;
> 
> +    CopyGuid (&(HandoffTables2.TableEntry[0].VendorGuid), TableGuid);
> 
> +    HandoffTables2.TableEntry[0].VendorTable = TableAddress;
> 
> +
> 
> +    EventType = EV_EFI_HANDOFF_TABLES2;
> 
> +    EventLog = &HandoffTables2;
> 
> +    EventLogSize = sizeof(HandoffTables2);
> 
> +  } else {
> 
> +    HandoffTables.NumberOfTables = 1;
> 
> +    CopyGuid (&(HandoffTables.TableEntry[0].VendorGuid), TableGuid);
> 
> +    HandoffTables.TableEntry[0].VendorTable = TableAddress;
> 
> +
> 
> +    EventType = EV_EFI_HANDOFF_TABLES;
> 
> +    EventLog = &HandoffTables;
> 
> +    EventLogSize = sizeof(HandoffTables);
> 
> +  }
> 
> +
> 
> +  Status = TpmMeasureAndLogData (
> 
> +             PcrIndex,
> 
> +             EventType,
> 
> +             EventLog,
> 
> +             EventLogSize,
> 
> +             TableAddress,
> 
> +             TableLength
> 
> +             );
> 
> +  return Status;
> 
> +}
> 
> --
> 2.26.2.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#63754): https://edk2.groups.io/g/devel/message/63754
> Mute This Topic: https://groups.io/mt/76019584/1768734
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jian.j.wang@intel.com]
> -=-=-=-=-=-=