[PATCH v2 0/3] Common OBB verification feature

[PATCH v2 0/3] Common OBB verification feature

[Wang, Jian J]

>V2: fix parameter description error found by ECC

https://bugzilla.tianocore.org/show_bug.cgi?id=1617

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran@intel.com>
Cc: Harry Han <harry.han@intel.com>

Jian J Wang (3):
  SecurityPkg: add definitions for OBB verification
  SecurityPkg/FvReportPei: implement a common FV verifier and reporter
  SecurityPkg: add FvReportPei.inf in dsc for build validation

 SecurityPkg/FvReportPei/FvReportPei.c         | 418 ++++++++++++++++++
 SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
 SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
 SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
 .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
 .../Ppi/FirmwareVolumeInfoStoredHashFv.h      |  61 +++
 SecurityPkg/SecurityPkg.dec                   |   9 +
 SecurityPkg/SecurityPkg.dsc                   |   5 +
 8 files changed, 697 insertions(+)
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
 create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
 create mode 100644 SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h

-- 
2.17.1.windows.2

[PATCH v2 1/3] SecurityPkg: add definitions for OBB verification

[Wang, Jian J]

https://bugzilla.tianocore.org/show_bug.cgi?id=1617

gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid should be installed by
platform to pass FV hash information to the common FV verify/report
driver, in which the hash value will be calculated again based on the
information fed in and then verified.

The information passed in this PPI include:
  - FVs location in flash and length
  - Hash values for different boot mode

The hash value must be calculated in following way (if 3 FVs to calc):

  FV1 -> Hash1
  FV2 -> Hash2
  FV3 -> Hash3
  Hash1 + Hash2 + Hash3 -> HashAll

Only HashAll is stored in this PPI. The purposes for this algorithm
are two:

  1. To report each FV's hash to TCG driver and verify HashAll at the
     same time without the burden to calculate the hash twice;
  2. To save hash value storage due to potential hardware limitation

Different boot mode may have its own hash value so that each mode can
decide which FV will be verified. For example, for the sake of performance,
S3 may choose to skip some FVs verification and normal boot will verify
all FVs it concerns.

So in this PPI, each FV information has flag to indicate which boot mode
it will be taken into hash calculation.

And if multiple hash values passed in this PPI, each has a flag to indicate
which boot mode it's used for. Note one hash value supports more than one
boot modes if they're just the same.

PcdStatusCodeFvVerificationPass and PcdStatusCodeFvVerificationFail are
introduced to report status back to platform, and platform can choose how
to act upon verification success and failure.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 .../Ppi/FirmwareVolumeInfoStoredHashFv.h      | 61 +++++++++++++++++++
 SecurityPkg/SecurityPkg.dec                   |  9 +++
 2 files changed, 70 insertions(+)
 create mode 100644 SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h

diff --git a/SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h b/SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
new file mode 100644
index 0000000000..71d10728c5
--- /dev/null
+++ b/SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
@@ -0,0 +1,61 @@
+/** @file
+PPI to describe stored hash digest for FVs.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_H__
+#define __PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_H__
+
+#include <Ppi/FirmwareVolumeInfoPrehashedFV.h>
+
+// {7F5E4E31-81B1-47E5-9E21-1E4B5BC2F61D}
+#define EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI_GUID \
+  {0x7f5e4e31, 0x81b1, 0x47e5, {0x9e, 0x21, 0x1e, 0x4b, 0x5b, 0xc2, 0xf6, 0x1d}}
+
+//
+// Hashed FV flags.
+//
+#define HASHED_FV_FLAG_REPORT_FV_INFO_PPI     0x0000000000000001
+#define HASHED_FV_FLAG_REPORT_FV_HOB          0x0000000000000002
+#define HASHED_FV_FLAG_VERIFIED_BOOT          0x0000000000000010
+#define HASHED_FV_FLAG_MEASURED_BOOT          0x0000000000000020
+#define HASHED_FV_FLAG_SKIP_ALL               0xFFFFFFFFFFFFFF00
+#define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64 (0x100, (Mode))
+
+#define HASHED_FV_MAX_NUMBER                  10
+
+#define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1, (Mode))
+
+typedef struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI
+                EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI;
+
+typedef struct _HASHED_FV_INFO {
+  UINT32                  Base;
+  UINT32                  Length;
+  UINT64                  Flag;
+} HASHED_FV_INFO;
+
+typedef struct _FV_HASH_INFO {
+  UINT64                  HashFlag;
+  UINT16                  HashAlgoId;
+  UINT16                  HashSize;
+  UINT8                   Hash[64];
+} FV_HASH_INFO;
+
+//
+// PPI used to convey FVs and hash information of a specific platform.
+//
+struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
+  UINTN                   FvNumber;
+  HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
+  UINTN                   HashNumber;
+  FV_HASH_INFO            HashInfo[1];
+};
+
+extern EFI_GUID gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid;
+
+#endif
+
diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index 3314f1854b..f0c0581b17 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -187,6 +187,9 @@
 
   ## Include/Ppi/FirmwareVolumeInfoPrehashedFV.h
   gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid = { 0x3ce1e631, 0x7008, 0x477c, { 0xad, 0xa7, 0x5d, 0xcf, 0xc7, 0xc1, 0x49, 0x4b } }
+ 
+  ## Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
+  gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid = {0x7f5e4e31, 0x81b1, 0x47e5, { 0x9e, 0x21, 0x1e, 0x4b, 0x5b, 0xc2, 0xf6, 0x1d } }
 
 #
 # [Error.gEfiSecurityPkgTokenSpaceGuid]
@@ -257,6 +260,12 @@
   # @ValidList  0x80000003 | 0x010D0000
   gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeSubClassTpmDevice|0x010D0000|UINT32|0x00000007
 
+  ## Progress Code for FV verification result.<BR><BR>
+  #  (EFI_SOFTWARE_PEI_MODULE | EFI_SUBCLASS_SPECIFIC | XXX)
+  # @Prompt Status Code for FV verification result
+  gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A|UINT32|0x00010030
+  gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B|UINT32|0x00010031
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
   #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>
-- 
2.17.1.windows.2

[PATCH v2 2/3] SecurityPkg/FvReportPei: implement a common FV verifier and reporter

[Wang, Jian J]

>v2: correct parameter and return value description for GetHashInfo()

https://bugzilla.tianocore.org/show_bug.cgi?id=1617

This driver implements a common checker, verifier and reporter which is
independent of hardware based root-of-trust.

Usually the hardware based root-of-trust will not verify all BIOS but
part of it. For example, Boot Guard will only verify IBB segment. The IBB
needs to verify other part of BIOS, i.e. other FVs to transfer control to
from IBB. This driver plays the role in IBB to verify FVs not covered by
hardware root-of-trust to make sure integrity of the chain of trust.

To be hardware/platform independent, PPI

  gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid

is introduced for platform to pass digest information to this driver.
This PPI should include all information needed to verify required FVs in
required boot mode.

struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
  UINTN                   FvNumber;
  HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
  UINTN                   HashNumber;
  FV_HASH_INFO            HashInfo[1];
};

To avoid TOCTOU issue, all FVs to be verified will be copied to memory
before hash calculation. That also means this driver has to be run after
permanent memory has been discovered.

For a measured boot, this driver will install

  gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid

to report digest of each FV to TCG driver.

For a verified boot, this driver will verify the final hash value
(calculated from the concatenation of each FV's hash) for indicated
FVs against the hash got from platform/hardware.

If pass, it will build EFI_HOB_TYPE_FV (consumed by DXE core) and/or
install gEfiPeiFirmwareVolumeInfoPpiGuid (consumed by PEI core), and
then report status code PcdStatusCodeFvVerificationPass.

If fail, it just report status code PcdStatusCodeFvVerificationFail
and go to dead loop if status report returns.

The platform can register customized handler to process pass and fail
cases differently.

Currently, this driver only supports hash (sha256/384/512) verification
for the performance consideration.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 SecurityPkg/FvReportPei/FvReportPei.c         | 418 ++++++++++++++++++
 SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
 SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
 SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
 .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
 5 files changed, 622 insertions(+)
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
 create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
 create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni

diff --git a/SecurityPkg/FvReportPei/FvReportPei.c b/SecurityPkg/FvReportPei/FvReportPei.c
new file mode 100644
index 0000000000..07855774b4
--- /dev/null
+++ b/SecurityPkg/FvReportPei/FvReportPei.c
@@ -0,0 +1,418 @@
+/** @file
+  This driver verifies and reports OBB FVs.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "FvReportPei.h"
+
+STATIC CONST HASH_ALG_INFO mHashAlgInfo[] = {
+  {0, NULL, NULL, NULL, NULL},                    // 0000 TPM_ALG_ERROR
+  {0, NULL, NULL, NULL, NULL},                    // 0001 TPM_ALG_FIRST
+  {0, NULL, NULL, NULL, NULL},                    // 0002
+  {0, NULL, NULL, NULL, NULL},                    // 0003
+  {0, NULL, NULL, NULL, NULL},                    // 0004 TPM_ALG_SHA1
+  {0, NULL, NULL, NULL, NULL},                    // 0005
+  {0, NULL, NULL, NULL, NULL},                    // 0006 TPM_ALG_AES
+  {0, NULL, NULL, NULL, NULL},                    // 0007
+  {0, NULL, NULL, NULL, NULL},                    // 0008 TPM_ALG_KEYEDHASH
+  {0, NULL, NULL, NULL, NULL},                    // 0009
+  {0, NULL, NULL, NULL, NULL},                    // 000A
+  {SHA256_DIGEST_SIZE, Sha256Init, Sha256Update, Sha256Final, Sha256HashAll}, // 000B TPM_ALG_SHA256
+  {SHA384_DIGEST_SIZE, Sha384Init, Sha384Update, Sha384Final, Sha384HashAll}, // 000C TPM_ALG_SHA384
+  {SHA512_DIGEST_SIZE, Sha512Init, Sha512Update, Sha512Final, Sha512HashAll}, // 000D TPM_ALG_SHA512
+  {0, NULL, NULL, NULL, NULL},                    // 000E
+  {0, NULL, NULL, NULL, NULL},                    // 000F
+  {0, NULL, NULL, NULL, NULL},                    // 0010 TPM_ALG_NULL
+//{0, NULL, NULL, NULL, NULL},                    // 0011
+//{0, NULL, NULL, NULL, NULL},                    // 0012 TPM_ALG_SM3_256
+};
+
+/**
+  Install a EDKII_PEI_FIRMWARE_VOLUME_INFO_PREHASHED_FV_PPI instance so that
+  TCG driver may use to extend PCRs.
+
+  @param[in]  FvBuffer            Buffer containing the whole FV.
+  @param[in]  FvLength            Length of the FV.
+  @param[in]  HashAlgoId          Hash algorithm type id.
+  @param[in]  HashSize            Hash size.
+  @param[in]  HashValue           Hash value buffer.
+**/
+STATIC
+VOID
+InstallPreHashFvPpi (
+  IN VOID           *FvBuffer,
+  IN UINTN          FvLength,
+  IN UINT16         HashAlgoId,
+  IN UINT16         HashSize,
+  IN UINT8          *HashValue
+  )
+{
+  EFI_STATUS                                        Status;
+  EFI_PEI_PPI_DESCRIPTOR                            *FvInfoPpiDescriptor;
+  EDKII_PEI_FIRMWARE_VOLUME_INFO_PREHASHED_FV_PPI   *PreHashedFvPpi;
+  UINTN                                             PpiSize;
+  HASH_INFO                                         *HashInfo;
+
+  PpiSize = sizeof (EDKII_PEI_FIRMWARE_VOLUME_INFO_PREHASHED_FV_PPI)
+            + sizeof (sizeof (HASH_INFO))
+            + HashSize;
+
+  PreHashedFvPpi = AllocatePool (PpiSize);
+  ASSERT (PreHashedFvPpi != NULL);
+
+  PreHashedFvPpi->FvBase    = (UINT32)(UINTN)FvBuffer;
+  PreHashedFvPpi->FvLength  = (UINT32)FvLength;
+  PreHashedFvPpi->Count     = 1;
+
+  HashInfo = HASH_INFO_PTR (PreHashedFvPpi);
+  HashInfo->HashAlgoId = HashAlgoId;
+  HashInfo->HashSize = HashSize;
+  CopyMem (HASH_VALUE_PTR (HashInfo), HashValue, HashSize);
+
+  FvInfoPpiDescriptor = AllocatePool (sizeof (EFI_PEI_PPI_DESCRIPTOR));
+  ASSERT (FvInfoPpiDescriptor != NULL);
+
+  FvInfoPpiDescriptor->Guid  = &gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid;
+  FvInfoPpiDescriptor->Flags = EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
+  FvInfoPpiDescriptor->Ppi   = (VOID *) PreHashedFvPpi;
+
+  Status = PeiServicesInstallPpi (FvInfoPpiDescriptor);
+  ASSERT_EFI_ERROR (Status);
+}
+
+/**
+  Calculate and verify hash value for given FV.
+
+  @param[in]  HashInfo            Hash information of the FV.
+  @param[in]  FvInfo              Information of FV used for verification.
+  @param[in]  FvNumber            Length of the FV.
+  @param[in]  BootMode            Length of the FV.
+
+  @retval EFI_SUCCESS           The given FV is integrate.
+  @retval EFI_VOLUME_CORRUPTED  The given FV is corrupted (hash mismatch).
+  @retval EFI_UNSUPPORTED       The hash algorithm is not supported.
+  @retval EFI_INVALID_PARAMETER FvInfo is NULL or FvNumber is zero.
+**/
+STATIC
+EFI_STATUS
+VerifyHashedFv (
+  IN FV_HASH_INFO         *HashInfo,
+  IN HASHED_FV_INFO       *FvInfo,
+  IN UINTN                FvNumber,
+  IN EFI_BOOT_MODE        BootMode
+  )
+{
+  UINTN                 FvIndex;
+  CONST HASH_ALG_INFO   *AlgInfo;
+  UINT8                 *HashValue;
+  UINT8                 *FvHashValue;
+  VOID                  *FvBuffer;
+  EFI_STATUS            Status;
+
+  if (HashInfo == NULL ||
+      HashInfo->HashSize == 0 ||
+      HashInfo->HashAlgoId == TPM_ALG_NULL) {
+    DEBUG ((DEBUG_INFO, "Bypass FV hash verification\r\n"));
+    return EFI_SUCCESS;
+  }
+
+  if (FvInfo == NULL || FvNumber == 0 ) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (HashInfo->HashAlgoId >= ARRAY_SIZE (mHashAlgInfo) ||
+      mHashAlgInfo[HashInfo->HashAlgoId].HashSize != HashInfo->HashSize) {
+    DEBUG ((DEBUG_ERROR, "Unsupported or wrong hash algorithm: %04X (size=%d)\r\n",
+            HashInfo->HashAlgoId, HashInfo->HashSize));
+    return EFI_UNSUPPORTED;
+  }
+
+  AlgInfo = &mHashAlgInfo[HashInfo->HashAlgoId];
+
+  //
+  // We need a hash value for each FV as well as one for all FVs.
+  //
+  HashValue = AllocateZeroPool (AlgInfo->HashSize * (FvNumber + 1));
+  ASSERT (HashValue != NULL);
+
+  //
+  // Calcuate hash value for each FV first.
+  //
+  FvHashValue = HashValue;
+  for (FvIndex = 0; FvIndex < FvNumber; ++FvIndex) {
+    //
+    // Skip any FV not meant for verified boot and measured boot.
+    //
+    if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) == 0 &&
+        (FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT) == 0) {
+      continue;
+    }
+
+    //
+    // Skip any FV not meant for current boot mode.
+    //
+    if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_SKIP_BOOT_MODE (BootMode)) != 0) {
+      continue;
+    }
+
+    DEBUG ((
+      DEBUG_INFO,
+      "Pre-hashed[alg=%04X,size=%d,flag=%016lX] FV: 0x%08X (%X) (Flag=%016lX)\r\n",
+      HashInfo->HashAlgoId,
+      HashInfo->HashSize,
+      HashInfo->HashFlag,
+      FvInfo[FvIndex].Base,
+      FvInfo[FvIndex].Length,
+      FvInfo[FvIndex].Flag
+      ));
+
+    //
+    // Copy FV to permanent memory to avoid potential TOC/TOU.
+    //
+    FvBuffer = AllocatePages (EFI_SIZE_TO_PAGES(FvInfo[FvIndex].Length));
+    ASSERT (FvBuffer != NULL);
+    CopyMem (FvBuffer, (CONST VOID *)(UINTN)FvInfo[FvIndex].Base, FvInfo[FvIndex].Length);
+
+    if (AlgInfo->HashAll (FvBuffer, FvInfo[FvIndex].Length, FvHashValue) == FALSE) {
+      Status = EFI_ABORTED;
+      goto Done;
+    }
+
+    //
+    // Report the FV measurement.
+    //
+    if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT) != 0) {
+      InstallPreHashFvPpi (
+        FvBuffer,
+        FvInfo[FvIndex].Length,
+        HashInfo->HashAlgoId,
+        HashInfo->HashSize,
+        FvHashValue
+        );
+    }
+
+    //
+    // Don't keep the hash value of current FV if we don't need to verify it.
+    //
+    if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) != 0) {
+      FvHashValue += AlgInfo->HashSize;
+    }
+
+    //
+    // Use memory copy of the FV from now on.
+    //
+    FvInfo[FvIndex].Base = (UINT32)(UINTN)FvBuffer;
+  }
+
+  //
+  // Check final hash for all FVs.
+  //
+  if (FvHashValue == HashValue ||
+      (AlgInfo->HashAll (HashValue, FvHashValue - HashValue, FvHashValue) &&
+       CompareMem (HashInfo->Hash, FvHashValue, AlgInfo->HashSize) == 0)) {
+    Status = EFI_SUCCESS;
+  } else {
+    Status = EFI_VOLUME_CORRUPTED;
+  }
+
+Done:
+  FreePool (HashValue);
+  return Status;
+}
+
+/**
+  Report FV to PEI and/or DXE core for dispatch.
+
+  @param[in] FvInfo     Information of a FV.
+
+**/
+STATIC
+VOID
+ReportHashedFv (
+  IN HASHED_FV_INFO       *FvInfo
+  )
+{
+  CONST EFI_GUID    *FvFormat;
+
+  if ((FvInfo->Flag & HASHED_FV_FLAG_REPORT_FV_HOB) != 0) {
+    //
+    // Require DXE core to process this FV.
+    //
+    BuildFvHob (
+      (EFI_PHYSICAL_ADDRESS)(UINTN)FvInfo->Base,
+      (UINT64)FvInfo->Length
+      );
+    DEBUG ((DEBUG_INFO, "Reported FV HOB: %08X (%X)\r\n", FvInfo->Base, FvInfo->Length));
+  }
+
+  if ((FvInfo->Flag & HASHED_FV_FLAG_REPORT_FV_INFO_PPI) != 0) {
+    //
+    // Require PEI core to process this FV.
+    //
+    FvFormat = &((EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvInfo->Base)->FileSystemGuid;
+    PeiServicesInstallFvInfoPpi (
+      FvFormat,
+      (VOID *)(UINTN)FvInfo->Base,
+      FvInfo->Length,
+      NULL,
+      NULL
+      );
+    DEBUG ((DEBUG_INFO, "Reported FV PPI: %08X (%X)\r\n", FvInfo->Base, FvInfo->Length));
+  }
+}
+
+/**
+  Verify and report pre-hashed FVs.
+
+  Doing this must be at post-memory to make sure there's enough memory to hold
+  all FVs to be verified. This is necessary for mitigating TOCTOU issue.
+
+  This function will never return if the verification is failed.
+
+  @param[in] StoredHashFvPpi  Pointer to PPI containing hash information.
+  @param[in] BootMode         Current boot mode.
+
+  @retval Pointer to structure containning valid hash information for current boot mode.
+  @retval NULL if there's no hash associated with current boot mode.
+**/
+STATIC
+FV_HASH_INFO *
+GetHashInfo (
+  IN EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI  *StoredHashFvPpi,
+  IN EFI_BOOT_MODE                                      BootMode
+  )
+{
+  FV_HASH_INFO            *HashInfo;
+  UINTN                   HashIndex;
+
+  for (HashIndex = 0, HashInfo = NULL;
+       HashIndex < StoredHashFvPpi->HashNumber && HashInfo == NULL;
+       ++HashIndex) {
+
+    if ((StoredHashFvPpi->HashInfo[HashIndex].HashFlag
+         & FV_HASH_FLAG_BOOT_MODE (BootMode)) != 0) {
+      HashInfo = &StoredHashFvPpi->HashInfo[HashIndex];
+      break;
+    }
+
+  }
+
+  return HashInfo;
+}
+
+/**
+  Verify and report pre-hashed FVs.
+
+  Doing this must be at post-memory to make sure there's enough memory to hold
+  all FVs to be verified. This is necessary for mitigating TOCTOU issue.
+
+  This function will never return if the verification is failed.
+
+  @param[in] PeiServices      General purpose services available to every PEIM.
+  @param[in] BootMode         Current boot mode.
+
+  @retval EFI_SUCCESS         The function completed successfully.
+**/
+STATIC
+EFI_STATUS
+CheckStoredHashFv (
+  IN CONST EFI_PEI_SERVICES           **PeiServices,
+  IN EFI_BOOT_MODE                    BootMode
+  )
+{
+  EFI_STATUS                                            Status;
+  UINT32                                                Instance;
+  EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI     *StoredHashFvPpi;
+  FV_HASH_INFO                                          *HashInfo;
+  UINTN                                                 FvIndex;
+
+  //
+  // Check pre-hashed FV list
+  //
+  Instance        = 0;
+  StoredHashFvPpi = NULL;
+  do {
+    Status = PeiServicesLocatePpi (
+               &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
+               Instance,
+               NULL,
+               (VOID**)&StoredHashFvPpi
+               );
+    if (!EFI_ERROR(Status) && StoredHashFvPpi != NULL && StoredHashFvPpi->FvNumber > 0) {
+
+      HashInfo = GetHashInfo(StoredHashFvPpi, BootMode);
+      Status = VerifyHashedFv (HashInfo, StoredHashFvPpi->FvInfo,
+                               StoredHashFvPpi->FvNumber, BootMode);
+      if (!EFI_ERROR (Status)) {
+        //
+        // Report the FVs to PEI core and/or DXE core.
+        //
+        for (FvIndex = 0; FvIndex < StoredHashFvPpi->FvNumber; ++FvIndex) {
+          if ((StoredHashFvPpi->FvInfo[FvIndex].Flag
+               & HASHED_FV_FLAG_SKIP_BOOT_MODE (BootMode)) == 0) {
+            ReportHashedFv (&StoredHashFvPpi->FvInfo[FvIndex]);
+          }
+        }
+
+      } else {
+
+        DEBUG ((DEBUG_ERROR, "ERROR: Failed to verify OBB FVs (%r)\r\n", Status));
+
+        REPORT_STATUS_CODE_EX (
+          EFI_PROGRESS_CODE,
+          PcdGet32 (PcdStatusCodeFvVerificationFail),
+          Instance,
+          NULL,
+          &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
+          StoredHashFvPpi,
+          sizeof (*StoredHashFvPpi)
+          );
+
+        ASSERT (FALSE);
+        CpuDeadLoop ();
+
+      }
+    }
+
+    Instance++;
+
+  } while (!EFI_ERROR(Status));
+
+  REPORT_STATUS_CODE (
+    EFI_PROGRESS_CODE,
+    PcdGet32 (PcdStatusCodeFvVerificationPass)
+    );
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Main entry for FvReport PEIM.
+
+  @param[in]  FileHandle              Handle of the file being invoked.
+  @param[in]  PeiServices             Pointer to PEI Services table.
+
+  @retval EFI_SUCCESS  If all FVs reported by StoredHashFvPpi are verified.
+
+**/
+EFI_STATUS
+EFIAPI
+FvReportEntryPoint (
+  IN       EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES     **PeiServices
+  )
+{
+  EFI_STATUS           Status;
+  EFI_BOOT_MODE        BootMode;
+
+  Status = PeiServicesGetBootMode (&BootMode);
+  ASSERT_EFI_ERROR (Status);
+
+  Status = CheckStoredHashFv (PeiServices, BootMode);
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
+}
diff --git a/SecurityPkg/FvReportPei/FvReportPei.h b/SecurityPkg/FvReportPei/FvReportPei.h
new file mode 100644
index 0000000000..fb7d205f73
--- /dev/null
+++ b/SecurityPkg/FvReportPei/FvReportPei.h
@@ -0,0 +1,121 @@
+/** @file
+  Definitions for OBB FVs verification.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __FV_REPORT_PEI_H__
+#define __FV_REPORT_PEI_H__
+
+#include <PiPei.h>
+
+#include <IndustryStandard/Tpm20.h>
+
+#include <Ppi/FirmwareVolumeInfoStoredHashFv.h>
+
+#include <Library/PeiServicesLib.h>
+#include <Library/PcdLib.h>
+#include <Library/HobLib.h>
+#include <Library/DebugLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/BaseCryptLib.h>
+#include <Library/ReportStatusCodeLib.h>
+
+#define HASH_INFO_PTR(PreHashedFvPpi)  \
+  (HASH_INFO *)((UINT8 *)(PreHashedFvPpi) + sizeof (EDKII_PEI_FIRMWARE_VOLUME_INFO_PREHASHED_FV_PPI))
+
+#define HASH_VALUE_PTR(HashInfo)   \
+  (VOID *)((UINT8 *)(HashInfo) + sizeof (HASH_INFO))
+
+/**
+  Computes the message digest of a input data buffer.
+
+  This function performs message digest of a given data buffer, and places
+  the digest value into the specified memory.
+
+  If this interface is not supported, then return FALSE.
+
+  @param[in]   Data        Pointer to the buffer containing the data to be hashed.
+  @param[in]   DataSize    Size of Data buffer in bytes.
+  @param[out]  HashValue   Pointer to a buffer that receives digest value.
+
+  @retval TRUE   The digest computation succeeded.
+  @retval FALSE  The digest computation failed.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI *HASH_ALL_METHOD) (
+  IN   CONST VOID  *Data,
+  IN   UINTN       DataSize,
+  OUT  UINT8       *HashValue
+  );
+
+/**
+  Initializes user-supplied memory as hash context for subsequent use.
+
+  @param[out]  HashContext  Pointer to hash context being initialized.
+
+  @retval TRUE   Hash context initialization succeeded.
+  @retval FALSE  Hash context initialization failed.
+  @retval FALSE  This interface is not supported.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI *HASH_INIT_METHOD) (
+  OUT  VOID  *HashContext
+  );
+
+/**
+  Digests the input data and updates hash context.
+
+  @param[in, out]  HashContext  Pointer to the hash context.
+  @param[in]       Data         Pointer to the buffer containing the data to be hashed.
+  @param[in]       DataSize     Size of Data buffer in bytes.
+
+  @retval TRUE   Hash data digest succeeded.
+  @retval FALSE  Hash data digest failed.
+  @retval FALSE  This interface is not supported.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI *HASH_UPDATE_METHOD) (
+  IN OUT  VOID        *HashContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  );
+
+/**
+  Completes computation of the hash digest value.
+
+  @param[in, out]  HashContext  Pointer to the hash context.
+  @param[out]      HashValue    Pointer to a buffer that receives the hash digest
+                                value.
+
+  @retval TRUE   Hash digest computation succeeded.
+  @retval FALSE  Hash digest computation failed.
+  @retval FALSE  This interface is not supported.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI *HASH_FINAL_METHOD) (
+  IN OUT  VOID   *HashContext,
+  OUT     UINT8  *HashValue
+  );
+
+typedef struct {
+  UINTN               HashSize;
+  HASH_INIT_METHOD    HashInit;
+  HASH_UPDATE_METHOD  HashUpdate;
+  HASH_FINAL_METHOD   HashFinal;
+  HASH_ALL_METHOD     HashAll;
+} HASH_ALG_INFO;
+
+#endif //__FV_REPORT_PEI_H__
+
diff --git a/SecurityPkg/FvReportPei/FvReportPei.inf b/SecurityPkg/FvReportPei/FvReportPei.inf
new file mode 100644
index 0000000000..2f1188509b
--- /dev/null
+++ b/SecurityPkg/FvReportPei/FvReportPei.inf
@@ -0,0 +1,57 @@
+## @file
+# FV Report/Verify PEI Driver.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = FvReportPei
+  MODULE_UNI_FILE                = FvReportPei.uni
+  FILE_GUID                      = 72405B40-38DA-4ABA-9283-CA8321C23E63
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = FvReportEntryPoint
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64
+#
+
+[Sources]
+  FvReportPei.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  CryptoPkg/CryptoPkg.dec
+  SecurityPkg/SecurityPkg.dec
+
+[LibraryClasses]
+  PeimEntryPoint
+  PeiServicesLib
+  BaseLib
+  DebugLib
+  BaseMemoryLib
+  PcdLib
+  HobLib
+  MemoryAllocationLib
+  BaseCryptLib
+  ReportStatusCodeLib
+
+[Ppis]
+  gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid   ## PRODUCES
+  gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid  ## CONSUMES
+
+[Pcd]
+  gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass
+  gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail
+
+[Depex]
+  gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid AND gEfiPeiMemoryDiscoveredPpiGuid
+
+[UserExtensions.TianoCore."ExtraFiles"]
+  FvReportPeiPeiExtra.uni
diff --git a/SecurityPkg/FvReportPei/FvReportPei.uni b/SecurityPkg/FvReportPei/FvReportPei.uni
new file mode 100644
index 0000000000..bad43403c4
--- /dev/null
+++ b/SecurityPkg/FvReportPei/FvReportPei.uni
@@ -0,0 +1,14 @@
+// /** @file
+// FV Verify/Report PEI Driver.
+//
+// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// **/
+
+
+#string STR_MODULE_ABSTRACT             #language en-US "This module verifies and reports FVs."
+
+#string STR_MODULE_DESCRIPTION          #language en-US "This module verifies FVs' digest passed through gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid, "
+                                                        "and installs gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid, gEfiPeiFirmwareVolumeInfoPpiGuid and/or FV HOB if passed."
+
diff --git a/SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni b/SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
new file mode 100644
index 0000000000..6214bbdaa9
--- /dev/null
+++ b/SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
@@ -0,0 +1,12 @@
+// /** @file
+// FV Verify/Report PEI Driver.
+//
+// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// **/
+
+#string STR_PROPERTIES_MODULE_NAME
+#language en-US "FV Verify/Report PEI Driver"
+
+
-- 
2.17.1.windows.2

[PATCH v2 3/3] SecurityPkg: add FvReportPei.inf in dsc for build validation

[Wang, Jian J]

https://bugzilla.tianocore.org/show_bug.cgi?id=1617

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 SecurityPkg/SecurityPkg.dsc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/SecurityPkg/SecurityPkg.dsc b/SecurityPkg/SecurityPkg.dsc
index a2ee0528f0..4451bd1271 100644
--- a/SecurityPkg/SecurityPkg.dsc
+++ b/SecurityPkg/SecurityPkg.dsc
@@ -287,6 +287,11 @@
   SecurityPkg/HddPassword/HddPasswordDxe.inf
   SecurityPkg/HddPassword/HddPasswordPei.inf
 
+  #
+  # Common FV checker/verifier/reporter
+  #
+  SecurityPkg/FvReportPei/FvReportPei.inf
+
 [BuildOptions]
    MSFT:*_*_IA32_DLINK_FLAGS = /ALIGN:256
   INTEL:*_*_IA32_DLINK_FLAGS = /ALIGN:256
-- 
2.17.1.windows.2

Re: [PATCH v2 0/3] Common OBB verification feature

[Yao, Jiewen]

Thanks Jian. Some comment below:

0) Please add what unit test has been done.

1) Can we use UINT64 for Base and Length?
typedef struct _HASHED_FV_INFO {
  UINT32                  Base;
  UINT32                  Length;
  UINT64                  Flag;
} HASHED_FV_INFO;

2) Can we remove the hard code HASHED_FV_MAX_NUMBER and use more flexible way?
#define HASHED_FV_MAX_NUMBER                  10
struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
  UINTN                   FvNumber;
  HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
  UINTN                   HashNumber;
  FV_HASH_INFO            HashInfo[1];
};

3) can we use better way to organize the table? It is weird to have so many zero. Why not just use TPM_ALG_xxx as the first field and search?
STATIC CONST HASH_ALG_INFO mHashAlgInfo[] = {
  {0, NULL, NULL, NULL, NULL},                    // 0000 TPM_ALG_ERROR
  {0, NULL, NULL, NULL, NULL},                    // 0001 TPM_ALG_FIRST
  {0, NULL, NULL, NULL, NULL},                    // 0002
  {0, NULL, NULL, NULL, NULL},                    // 0003
  {0, NULL, NULL, NULL, NULL},                    // 0004 TPM_ALG_SHA1
  {0, NULL, NULL, NULL, NULL},                    // 0005
  {0, NULL, NULL, NULL, NULL},                    // 0006 TPM_ALG_AES
  {0, NULL, NULL, NULL, NULL},                    // 0007
  {0, NULL, NULL, NULL, NULL},                    // 0008 TPM_ALG_KEYEDHASH
  {0, NULL, NULL, NULL, NULL},                    // 0009
  {0, NULL, NULL, NULL, NULL},                    // 000A
  {SHA256_DIGEST_SIZE, Sha256Init, Sha256Update, Sha256Final, Sha256HashAll}, // 000B TPM_ALG_SHA256
  {SHA384_DIGEST_SIZE, Sha384Init, Sha384Update, Sha384Final, Sha384HashAll}, // 000C TPM_ALG_SHA384
  {SHA512_DIGEST_SIZE, Sha512Init, Sha512Update, Sha512Final, Sha512HashAll}, // 000D TPM_ALG_SHA512
  {0, NULL, NULL, NULL, NULL},                    // 000E
  {0, NULL, NULL, NULL, NULL},                    // 000F
  {0, NULL, NULL, NULL, NULL},                    // 0010 TPM_ALG_NULL
//{0, NULL, NULL, NULL, NULL},                    // 0011
//{0, NULL, NULL, NULL, NULL},                    // 0012 TPM_ALG_SM3_256
};

4) Why not just add one bit say: skip in S3 ? Why need such complexity?
#define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64 (0x100, (Mode))
#define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1, (Mode))

I am not sure how that works. Is boot mode bit start from BIT0 or BIT8 ? I am confused.

    if ((StoredHashFvPpi->HashInfo[HashIndex].HashFlag
         & FV_HASH_FLAG_BOOT_MODE (BootMode)) != 0) {
      HashInfo = &StoredHashFvPpi->HashInfo[HashIndex];
      break;
    }

5) Why the producer want skip both verified boot and measured boot? Is that legal or illegal? If it is illegal, I prefer use ASSER() to tell people.
    if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) == 0 &&
        (FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT) == 0) {
      continue;
    }

6) I recommend to add one debug message to tell people this is skipped.
    //
    // Skip any FV not meant for current boot mode.
    //
    if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_SKIP_BOOT_MODE (BootMode)) != 0) {
      continue;
    }

7) Would you please clarify why and when a platform need report multiple StartedHashFv ?
  do {
    Status = PeiServicesLocatePpi (
               &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
               Instance,
               NULL,
               (VOID**)&StoredHashFvPpi
               );
    if (!EFI_ERROR(Status) && StoredHashFvPpi != NULL && StoredHashFvPpi->FvNumber > 0) {

It will be better, if you can those description in StoredHashFvPpi.h file

8) Same code above, would you please clarify if it is legal or illegal that StoredHashFvPpi->FvNumber == 0 ?
If it is illegal, I prefer use ASSERT()

Thank you
Yao Jiewen


> -----Original Message-----
> From: Wang, Jian J
> Sent: Tuesday, June 11, 2019 2:36 AM
> To: devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> Subject: [PATCH v2 0/3] Common OBB verification feature
> 
> >V2: fix parameter description error found by ECC
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1617
> 
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> 
> Jian J Wang (3):
>   SecurityPkg: add definitions for OBB verification
>   SecurityPkg/FvReportPei: implement a common FV verifier and reporter
>   SecurityPkg: add FvReportPei.inf in dsc for build validation
> 
>  SecurityPkg/FvReportPei/FvReportPei.c         | 418
> ++++++++++++++++++
>  SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
>  SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
>  SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
>  .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
>  .../Ppi/FirmwareVolumeInfoStoredHashFv.h      |  61 +++
>  SecurityPkg/SecurityPkg.dec                   |   9 +
>  SecurityPkg/SecurityPkg.dsc                   |   5 +
>  8 files changed, 697 insertions(+)
>  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
>  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
>  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
>  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
>  create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
>  create mode 100644
> SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
> 
> --
> 2.17.1.windows.2

Re: [PATCH v2 0/3] Common OBB verification feature

[Wang, Jian J]

Jiewen,

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, June 12, 2019 12:49 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> 
> Thanks Jian. Some comment below:
> 
> 0) Please add what unit test has been done.
> 
> 1) Can we use UINT64 for Base and Length?
> typedef struct _HASHED_FV_INFO {
>   UINT32                  Base;
>   UINT32                  Length;
>   UINT64                  Flag;
> } HASHED_FV_INFO;
> 

Yes, we can. But is it necessary? Isn't the flash address always below 4G?

> 2) Can we remove the hard code HASHED_FV_MAX_NUMBER and use more
> flexible way?
> #define HASHED_FV_MAX_NUMBER                  10
> struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
>   UINTN                   FvNumber;
>   HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
>   UINTN                   HashNumber;
>   FV_HASH_INFO            HashInfo[1];
> };
> 

Yes. I thought we need more than one hash value here. I went through the whole
logic here. Maybe one hash value is enough (no need to pass the hash value not
meant for current boot mode). So we can put the FvInfo at the end of structure
and remove the hard-coded fv number.

> 3) can we use better way to organize the table? It is weird to have so many zero.
> Why not just use TPM_ALG_xxx as the first field and search?
> STATIC CONST HASH_ALG_INFO mHashAlgInfo[] = {
>   {0, NULL, NULL, NULL, NULL},                    // 0000 TPM_ALG_ERROR
>   {0, NULL, NULL, NULL, NULL},                    // 0001 TPM_ALG_FIRST
>   {0, NULL, NULL, NULL, NULL},                    // 0002
>   {0, NULL, NULL, NULL, NULL},                    // 0003
>   {0, NULL, NULL, NULL, NULL},                    // 0004 TPM_ALG_SHA1
>   {0, NULL, NULL, NULL, NULL},                    // 0005
>   {0, NULL, NULL, NULL, NULL},                    // 0006 TPM_ALG_AES
>   {0, NULL, NULL, NULL, NULL},                    // 0007
>   {0, NULL, NULL, NULL, NULL},                    // 0008 TPM_ALG_KEYEDHASH
>   {0, NULL, NULL, NULL, NULL},                    // 0009
>   {0, NULL, NULL, NULL, NULL},                    // 000A
>   {SHA256_DIGEST_SIZE, Sha256Init, Sha256Update, Sha256Final,
> Sha256HashAll}, // 000B TPM_ALG_SHA256
>   {SHA384_DIGEST_SIZE, Sha384Init, Sha384Update, Sha384Final,
> Sha384HashAll}, // 000C TPM_ALG_SHA384
>   {SHA512_DIGEST_SIZE, Sha512Init, Sha512Update, Sha512Final,
> Sha512HashAll}, // 000D TPM_ALG_SHA512
>   {0, NULL, NULL, NULL, NULL},                    // 000E
>   {0, NULL, NULL, NULL, NULL},                    // 000F
>   {0, NULL, NULL, NULL, NULL},                    // 0010 TPM_ALG_NULL
> //{0, NULL, NULL, NULL, NULL},                    // 0011
> //{0, NULL, NULL, NULL, NULL},                    // 0012 TPM_ALG_SM3_256
> };
> 

I prefer the code directly index the algorithm info/methods as array. It
makes code quite simpler.

> 4) Why not just add one bit say: skip in S3 ? Why need such complexity?
> #define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64 (0x100,
> (Mode))
> #define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1, (Mode))
> 
> I am not sure how that works. Is boot mode bit start from BIT0 or BIT8 ? I am
> confused.
> 
>     if ((StoredHashFvPpi->HashInfo[HashIndex].HashFlag
>          & FV_HASH_FLAG_BOOT_MODE (BootMode)) != 0) {
>       HashInfo = &StoredHashFvPpi->HashInfo[HashIndex];
>       break;
>     }
> 

Boot mode is just a const number less than 64. So 64 bits can hold all different
boot mode. Using this way is just to keep the flexibility to avoid code change if
we want to support more boot modes besides S3. But if there's never such
possibility at all, you're right that one bit is enough.

> 5) Why the producer want skip both verified boot and measured boot? Is that
> legal or illegal? If it is illegal, I prefer use ASSER() to tell people.
>     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) == 0 &&
>         (FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT) == 0) {
>       continue;
>     }

Suppose there's a use case, most likely for developers, which need to disable
security feature temporarily. The BIOS still need to boot. The developers don't
need to remove this driver in order to do it. I think it's legacl.

> 
> 6) I recommend to add one debug message to tell people this is skipped.
>     //
>     // Skip any FV not meant for current boot mode.
>     //
>     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_SKIP_BOOT_MODE
> (BootMode)) != 0) {
>       continue;
>     }
> 

Right. I'll add one.

> 7) Would you please clarify why and when a platform need report multiple
> StartedHashFv ?
>   do {
>     Status = PeiServicesLocatePpi (
>                &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
>                Instance,
>                NULL,
>                (VOID**)&StoredHashFvPpi
>                );
>     if (!EFI_ERROR(Status) && StoredHashFvPpi != NULL && StoredHashFvPpi-
> >FvNumber > 0) {
> 
> It will be better, if you can those description in StoredHashFvPpi.h file
> 

I don't know if there's such necessity. It's just trying to keep a certain of flexibility.

> 8) Same code above, would you please clarify if it is legal or illegal that
> StoredHashFvPpi->FvNumber == 0 ?
> If it is illegal, I prefer use ASSERT()
> 

Let's call it illegal in case of skipping.

Regards,
Jian

> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Tuesday, June 11, 2019 2:36 AM
> > To: devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Hernandez Beltran, Jorge
> > <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> > Subject: [PATCH v2 0/3] Common OBB verification feature
> >
> > >V2: fix parameter description error found by ECC
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1617
> >
> > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran@intel.com>
> > Cc: Harry Han <harry.han@intel.com>
> >
> > Jian J Wang (3):
> >   SecurityPkg: add definitions for OBB verification
> >   SecurityPkg/FvReportPei: implement a common FV verifier and reporter
> >   SecurityPkg: add FvReportPei.inf in dsc for build validation
> >
> >  SecurityPkg/FvReportPei/FvReportPei.c         | 418
> > ++++++++++++++++++
> >  SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
> >  SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
> >  SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
> >  .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
> >  .../Ppi/FirmwareVolumeInfoStoredHashFv.h      |  61 +++
> >  SecurityPkg/SecurityPkg.dec                   |   9 +
> >  SecurityPkg/SecurityPkg.dsc                   |   5 +
> >  8 files changed, 697 insertions(+)
> >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
> >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
> >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
> >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
> >  create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
> >  create mode 100644
> > SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
> >
> > --
> > 2.17.1.windows.2

Re: [PATCH v2 0/3] Common OBB verification feature

[Yao, Jiewen]

Thanks.
Comment below:

> -----Original Message-----
> From: Wang, Jian J
> Sent: Friday, June 14, 2019 8:30 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> 
> Jiewen,
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Wednesday, June 12, 2019 12:49 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> > <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> > Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> >
> > Thanks Jian. Some comment below:
> >
> > 0) Please add what unit test has been done.
> >
> > 1) Can we use UINT64 for Base and Length?
> > typedef struct _HASHED_FV_INFO {
> >   UINT32                  Base;
> >   UINT32                  Length;
> >   UINT64                  Flag;
> > } HASHED_FV_INFO;
> >
> 
> Yes, we can. But is it necessary? Isn't the flash address always below 4G?
[Jiewen] We have other PCD use UINT64 for flash address.
Also, it might happen in emulation environment.

> 
> > 2) Can we remove the hard code HASHED_FV_MAX_NUMBER and use
> more
> > flexible way?
> > #define HASHED_FV_MAX_NUMBER                  10
> > struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
> >   UINTN                   FvNumber;
> >   HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
> >   UINTN                   HashNumber;
> >   FV_HASH_INFO            HashInfo[1];
> > };
> >
> 
> Yes. I thought we need more than one hash value here. I went through the
> whole
> logic here. Maybe one hash value is enough (no need to pass the hash value
> not
> meant for current boot mode). So we can put the FvInfo at the end of
> structure
> and remove the hard-coded fv number.
[Jiewen] May I know how you support multiple hash algorithms?


> 
> > 3) can we use better way to organize the table? It is weird to have so many
> zero.
> > Why not just use TPM_ALG_xxx as the first field and search?
> > STATIC CONST HASH_ALG_INFO mHashAlgInfo[] = {
> >   {0, NULL, NULL, NULL, NULL},                    // 0000
> TPM_ALG_ERROR
> >   {0, NULL, NULL, NULL, NULL},                    // 0001
> TPM_ALG_FIRST
> >   {0, NULL, NULL, NULL, NULL},                    // 0002
> >   {0, NULL, NULL, NULL, NULL},                    // 0003
> >   {0, NULL, NULL, NULL, NULL},                    // 0004
> TPM_ALG_SHA1
> >   {0, NULL, NULL, NULL, NULL},                    // 0005
> >   {0, NULL, NULL, NULL, NULL},                    // 0006
> TPM_ALG_AES
> >   {0, NULL, NULL, NULL, NULL},                    // 0007
> >   {0, NULL, NULL, NULL, NULL},                    // 0008
> TPM_ALG_KEYEDHASH
> >   {0, NULL, NULL, NULL, NULL},                    // 0009
> >   {0, NULL, NULL, NULL, NULL},                    // 000A
> >   {SHA256_DIGEST_SIZE, Sha256Init, Sha256Update, Sha256Final,
> > Sha256HashAll}, // 000B TPM_ALG_SHA256
> >   {SHA384_DIGEST_SIZE, Sha384Init, Sha384Update, Sha384Final,
> > Sha384HashAll}, // 000C TPM_ALG_SHA384
> >   {SHA512_DIGEST_SIZE, Sha512Init, Sha512Update, Sha512Final,
> > Sha512HashAll}, // 000D TPM_ALG_SHA512
> >   {0, NULL, NULL, NULL, NULL},                    // 000E
> >   {0, NULL, NULL, NULL, NULL},                    // 000F
> >   {0, NULL, NULL, NULL, NULL},                    // 0010
> TPM_ALG_NULL
> > //{0, NULL, NULL, NULL, NULL},                    // 0011
> > //{0, NULL, NULL, NULL, NULL},                    // 0012
> TPM_ALG_SM3_256
> > };
> >
> 
> I prefer the code directly index the algorithm info/methods as array. It
> makes code quite simpler.
[Jiewen] What happen if a new algo ID is assigned with a very big number?
Then you need many zero entry. I don't think it is necessary.
I prefer to use direct compare instead of index. Index can be used when the number is architecture defined.
Here we just need 4 entries, but totally 18 entries present.

> 
> > 4) Why not just add one bit say: skip in S3 ? Why need such complexity?
> > #define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64
> (0x100,
> > (Mode))
> > #define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1,
> (Mode))
> >
> > I am not sure how that works. Is boot mode bit start from BIT0 or BIT8 ? I
> am
> > confused.
> >
> >     if ((StoredHashFvPpi->HashInfo[HashIndex].HashFlag
> >          & FV_HASH_FLAG_BOOT_MODE (BootMode)) != 0) {
> >       HashInfo = &StoredHashFvPpi->HashInfo[HashIndex];
> >       break;
> >     }
> >
> 
> Boot mode is just a const number less than 64. So 64 bits can hold all
> different
> boot mode. Using this way is just to keep the flexibility to avoid code change
> if
> we want to support more boot modes besides S3. But if there's never such
> possibility at all, you're right that one bit is enough.
[Jiewen] But you already defined lowest 4 bits. I don't know the usage of below 2 MACRO.
Why one skip 8 bit, and other does not? Too confusing.

#define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64 (0x100, (Mode))
#define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1, (Mode))


> 
> > 5) Why the producer want skip both verified boot and measured boot? Is
> that
> > legal or illegal? If it is illegal, I prefer use ASSER() to tell people.
> >     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) == 0
> &&
> >         (FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT)
> == 0) {
> >       continue;
> >     }
> 
> Suppose there's a use case, most likely for developers, which need to disable
> security feature temporarily. The BIOS still need to boot. The developers
> don't
> need to remove this driver in order to do it. I think it's legal.

[Jiewen] I disagree. I believe it is illegal for production.
If we need disable both, this driver should NOT be included. It saves flash size.

> 
> >
> > 6) I recommend to add one debug message to tell people this is skipped.
> >     //
> >     // Skip any FV not meant for current boot mode.
> >     //
> >     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_SKIP_BOOT_MODE
> > (BootMode)) != 0) {
> >       continue;
> >     }
> >
> 
> Right. I'll add one.
[Jiewen] Thank you.

> 
> > 7) Would you please clarify why and when a platform need report multiple
> > StartedHashFv ?
> >   do {
> >     Status = PeiServicesLocatePpi (
> >                &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
> >                Instance,
> >                NULL,
> >                (VOID**)&StoredHashFvPpi
> >                );
> >     if (!EFI_ERROR(Status) && StoredHashFvPpi != NULL &&
> StoredHashFvPpi-
> > >FvNumber > 0) {
> >
> > It will be better, if you can those description in StoredHashFvPpi.h file
> >
> 
> I don't know if there's such necessity. It's just trying to keep a certain of
> flexibility.
[Jiewen] I prefer NOT. If we cannot find usage, please don't add such feature.
It adds the complexity of code, and adds the validation effort.

No matter you choose single PPI or multiple PPI, please describe this supported case in PPI.

> 
> > 8) Same code above, would you please clarify if it is legal or illegal that
> > StoredHashFvPpi->FvNumber == 0 ?
> > If it is illegal, I prefer use ASSERT()
> >
> 
> Let's call it illegal in case of skipping.
[Jiewen] Thanks. Please add ASSERT.

> 
> Regards,
> Jian
> 
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Tuesday, June 11, 2019 2:36 AM
> > > To: devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen
> > > <jiewen.yao@intel.com>; Hernandez Beltran, Jorge
> > > <jorge.hernandez.beltran@intel.com>; Han, Harry
> <harry.han@intel.com>
> > > Subject: [PATCH v2 0/3] Common OBB verification feature
> > >
> > > >V2: fix parameter description error found by ECC
> > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=1617
> > >
> > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran@intel.com>
> > > Cc: Harry Han <harry.han@intel.com>
> > >
> > > Jian J Wang (3):
> > >   SecurityPkg: add definitions for OBB verification
> > >   SecurityPkg/FvReportPei: implement a common FV verifier and
> reporter
> > >   SecurityPkg: add FvReportPei.inf in dsc for build validation
> > >
> > >  SecurityPkg/FvReportPei/FvReportPei.c         | 418
> > > ++++++++++++++++++
> > >  SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
> > >  SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
> > >  SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
> > >  .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
> > >  .../Ppi/FirmwareVolumeInfoStoredHashFv.h      |  61 +++
> > >  SecurityPkg/SecurityPkg.dec                   |   9 +
> > >  SecurityPkg/SecurityPkg.dsc                   |   5 +
> > >  8 files changed, 697 insertions(+)
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
> > >  create mode 100644
> > > SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
> > >
> > > --
> > > 2.17.1.windows.2

Re: [PATCH v2 0/3] Common OBB verification feature

[Wang, Jian J]

Jiewen,

See my comments below.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, June 14, 2019 6:41 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> 
> Thanks.
> Comment below:
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Friday, June 14, 2019 8:30 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> > <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> > Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> >
> > Jiewen,
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Wednesday, June 12, 2019 12:49 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> > > <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> > > Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> > >
> > > Thanks Jian. Some comment below:
> > >
> > > 0) Please add what unit test has been done.
> > >
> > > 1) Can we use UINT64 for Base and Length?
> > > typedef struct _HASHED_FV_INFO {
> > >   UINT32                  Base;
> > >   UINT32                  Length;
> > >   UINT64                  Flag;
> > > } HASHED_FV_INFO;
> > >
> >
> > Yes, we can. But is it necessary? Isn't the flash address always below 4G?
> [Jiewen] We have other PCD use UINT64 for flash address.
> Also, it might happen in emulation environment.
> 

If so, I'll change it.

> >
> > > 2) Can we remove the hard code HASHED_FV_MAX_NUMBER and use
> > more
> > > flexible way?
> > > #define HASHED_FV_MAX_NUMBER                  10
> > > struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
> > >   UINTN                   FvNumber;
> > >   HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
> > >   UINTN                   HashNumber;
> > >   FV_HASH_INFO            HashInfo[1];
> > > };
> > >
> >
> > Yes. I thought we need more than one hash value here. I went through the
> > whole
> > logic here. Maybe one hash value is enough (no need to pass the hash value
> > not
> > meant for current boot mode). So we can put the FvInfo at the end of
> > structure
> > and remove the hard-coded fv number.
> [Jiewen] May I know how you support multiple hash algorithms?
> 

Do you mean supporting multiple hash algo in the same build/boot? I didn't see such
requirement. Please clarify if this is required.

> 
> >
> > > 3) can we use better way to organize the table? It is weird to have so many
> > zero.
> > > Why not just use TPM_ALG_xxx as the first field and search?
> > > STATIC CONST HASH_ALG_INFO mHashAlgInfo[] = {
> > >   {0, NULL, NULL, NULL, NULL},                    // 0000
> > TPM_ALG_ERROR
> > >   {0, NULL, NULL, NULL, NULL},                    // 0001
> > TPM_ALG_FIRST
> > >   {0, NULL, NULL, NULL, NULL},                    // 0002
> > >   {0, NULL, NULL, NULL, NULL},                    // 0003
> > >   {0, NULL, NULL, NULL, NULL},                    // 0004
> > TPM_ALG_SHA1
> > >   {0, NULL, NULL, NULL, NULL},                    // 0005
> > >   {0, NULL, NULL, NULL, NULL},                    // 0006
> > TPM_ALG_AES
> > >   {0, NULL, NULL, NULL, NULL},                    // 0007
> > >   {0, NULL, NULL, NULL, NULL},                    // 0008
> > TPM_ALG_KEYEDHASH
> > >   {0, NULL, NULL, NULL, NULL},                    // 0009
> > >   {0, NULL, NULL, NULL, NULL},                    // 000A
> > >   {SHA256_DIGEST_SIZE, Sha256Init, Sha256Update, Sha256Final,
> > > Sha256HashAll}, // 000B TPM_ALG_SHA256
> > >   {SHA384_DIGEST_SIZE, Sha384Init, Sha384Update, Sha384Final,
> > > Sha384HashAll}, // 000C TPM_ALG_SHA384
> > >   {SHA512_DIGEST_SIZE, Sha512Init, Sha512Update, Sha512Final,
> > > Sha512HashAll}, // 000D TPM_ALG_SHA512
> > >   {0, NULL, NULL, NULL, NULL},                    // 000E
> > >   {0, NULL, NULL, NULL, NULL},                    // 000F
> > >   {0, NULL, NULL, NULL, NULL},                    // 0010
> > TPM_ALG_NULL
> > > //{0, NULL, NULL, NULL, NULL},                    // 0011
> > > //{0, NULL, NULL, NULL, NULL},                    // 0012
> > TPM_ALG_SM3_256
> > > };
> > >
> >
> > I prefer the code directly index the algorithm info/methods as array. It
> > makes code quite simpler.
> [Jiewen] What happen if a new algo ID is assigned with a very big number?
> Then you need many zero entry. I don't think it is necessary.
> I prefer to use direct compare instead of index. Index can be used when the
> number is architecture defined.
> Here we just need 4 entries, but totally 18 entries present.
> 
I don't have strong opinion. Let's update code with your way.

> >
> > > 4) Why not just add one bit say: skip in S3 ? Why need such complexity?
> > > #define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64
> > (0x100,
> > > (Mode))
> > > #define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1,
> > (Mode))
> > >
> > > I am not sure how that works. Is boot mode bit start from BIT0 or BIT8 ? I
> > am
> > > confused.
> > >
> > >     if ((StoredHashFvPpi->HashInfo[HashIndex].HashFlag
> > >          & FV_HASH_FLAG_BOOT_MODE (BootMode)) != 0) {
> > >       HashInfo = &StoredHashFvPpi->HashInfo[HashIndex];
> > >       break;
> > >     }
> > >
> >
> > Boot mode is just a const number less than 64. So 64 bits can hold all
> > different
> > boot mode. Using this way is just to keep the flexibility to avoid code change
> > if
> > we want to support more boot modes besides S3. But if there's never such
> > possibility at all, you're right that one bit is enough.
> [Jiewen] But you already defined lowest 4 bits. I don't know the usage of below
> 2 MACRO.
> Why one skip 8 bit, and other does not? Too confusing.
> 
> #define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64 (0x100,
> (Mode))
> #define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1, (Mode))
> 

They're different flags, one for FV and one for hash value.

Lower 8 bit is left room for FV flags of report method, HOB, FV_INFO_PPI or
potential others.

Hash flags have no such needs.

> 
> >
> > > 5) Why the producer want skip both verified boot and measured boot? Is
> > that
> > > legal or illegal? If it is illegal, I prefer use ASSER() to tell people.
> > >     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) == 0
> > &&
> > >         (FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT)
> > == 0) {
> > >       continue;
> > >     }
> >
> > Suppose there's a use case, most likely for developers, which need to disable
> > security feature temporarily. The BIOS still need to boot. The developers
> > don't
> > need to remove this driver in order to do it. I think it's legal.
> 
> [Jiewen] I disagree. I believe it is illegal for production.
> If we need disable both, this driver should NOT be included. It saves flash size.
> 

No strong opinion. Let's add ASSERT(). But if it's real illegal case, I think there should
be a dead loop here besides ASSERT(), right (the same as below ASSERT)?

> >
> > >
> > > 6) I recommend to add one debug message to tell people this is skipped.
> > >     //
> > >     // Skip any FV not meant for current boot mode.
> > >     //
> > >     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_SKIP_BOOT_MODE
> > > (BootMode)) != 0) {
> > >       continue;
> > >     }
> > >
> >
> > Right. I'll add one.
> [Jiewen] Thank you.
> 
> >
> > > 7) Would you please clarify why and when a platform need report multiple
> > > StartedHashFv ?
> > >   do {
> > >     Status = PeiServicesLocatePpi (
> > >                &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
> > >                Instance,
> > >                NULL,
> > >                (VOID**)&StoredHashFvPpi
> > >                );
> > >     if (!EFI_ERROR(Status) && StoredHashFvPpi != NULL &&
> > StoredHashFvPpi-
> > > >FvNumber > 0) {
> > >
> > > It will be better, if you can those description in StoredHashFvPpi.h file
> > >
> >
> > I don't know if there's such necessity. It's just trying to keep a certain of
> > flexibility.
> [Jiewen] I prefer NOT. If we cannot find usage, please don't add such feature.
> It adds the complexity of code, and adds the validation effort.
> 
> No matter you choose single PPI or multiple PPI, please describe this supported
> case in PPI.
> 
Agree. I'll update the code and PPI description.

> >
> > > 8) Same code above, would you please clarify if it is legal or illegal that
> > > StoredHashFvPpi->FvNumber == 0 ?
> > > If it is illegal, I prefer use ASSERT()
> > >
> >
> > Let's call it illegal in case of skipping.
> [Jiewen] Thanks. Please add ASSERT.
> 
> >
> > Regards,
> > Jian
> >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J
> > > > Sent: Tuesday, June 11, 2019 2:36 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen
> > > > <jiewen.yao@intel.com>; Hernandez Beltran, Jorge
> > > > <jorge.hernandez.beltran@intel.com>; Han, Harry
> > <harry.han@intel.com>
> > > > Subject: [PATCH v2 0/3] Common OBB verification feature
> > > >
> > > > >V2: fix parameter description error found by ECC
> > > >
> > > > https://bugzilla.tianocore.org/show_bug.cgi?id=1617
> > > >
> > > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran@intel.com>
> > > > Cc: Harry Han <harry.han@intel.com>
> > > >
> > > > Jian J Wang (3):
> > > >   SecurityPkg: add definitions for OBB verification
> > > >   SecurityPkg/FvReportPei: implement a common FV verifier and
> > reporter
> > > >   SecurityPkg: add FvReportPei.inf in dsc for build validation
> > > >
> > > >  SecurityPkg/FvReportPei/FvReportPei.c         | 418
> > > > ++++++++++++++++++
> > > >  SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
> > > >  SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
> > > >  SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
> > > >  .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
> > > >  .../Ppi/FirmwareVolumeInfoStoredHashFv.h      |  61 +++
> > > >  SecurityPkg/SecurityPkg.dec                   |   9 +
> > > >  SecurityPkg/SecurityPkg.dsc                   |   5 +
> > > >  8 files changed, 697 insertions(+)
> > > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
> > > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
> > > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
> > > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
> > > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
> > > >  create mode 100644
> > > > SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
> > > >
> > > > --
> > > > 2.17.1.windows.2

Re: [edk2-devel] [PATCH v2 0/3] Common OBB verification feature

[Wang, Jian J]

Forgot to mention test:
1. Unit test and security test based on HBFA test framework
2. System test on three real platforms

Regards,
Jian


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Wang,
> Jian J
> Sent: Friday, June 14, 2019 8:30 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 0/3] Common OBB verification feature
> 
> Jiewen,
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Wednesday, June 12, 2019 12:49 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Hernandez Beltran, Jorge
> > <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> > Subject: RE: [PATCH v2 0/3] Common OBB verification feature
> >
> > Thanks Jian. Some comment below:
> >
> > 0) Please add what unit test has been done.
> >
> > 1) Can we use UINT64 for Base and Length?
> > typedef struct _HASHED_FV_INFO {
> >   UINT32                  Base;
> >   UINT32                  Length;
> >   UINT64                  Flag;
> > } HASHED_FV_INFO;
> >
> 
> Yes, we can. But is it necessary? Isn't the flash address always below 4G?
> 
> > 2) Can we remove the hard code HASHED_FV_MAX_NUMBER and use more
> > flexible way?
> > #define HASHED_FV_MAX_NUMBER                  10
> > struct _EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI {
> >   UINTN                   FvNumber;
> >   HASHED_FV_INFO          FvInfo[HASHED_FV_MAX_NUMBER];
> >   UINTN                   HashNumber;
> >   FV_HASH_INFO            HashInfo[1];
> > };
> >
> 
> Yes. I thought we need more than one hash value here. I went through the
> whole
> logic here. Maybe one hash value is enough (no need to pass the hash value not
> meant for current boot mode). So we can put the FvInfo at the end of structure
> and remove the hard-coded fv number.
> 
> > 3) can we use better way to organize the table? It is weird to have so many
> zero.
> > Why not just use TPM_ALG_xxx as the first field and search?
> > STATIC CONST HASH_ALG_INFO mHashAlgInfo[] = {
> >   {0, NULL, NULL, NULL, NULL},                    // 0000 TPM_ALG_ERROR
> >   {0, NULL, NULL, NULL, NULL},                    // 0001 TPM_ALG_FIRST
> >   {0, NULL, NULL, NULL, NULL},                    // 0002
> >   {0, NULL, NULL, NULL, NULL},                    // 0003
> >   {0, NULL, NULL, NULL, NULL},                    // 0004 TPM_ALG_SHA1
> >   {0, NULL, NULL, NULL, NULL},                    // 0005
> >   {0, NULL, NULL, NULL, NULL},                    // 0006 TPM_ALG_AES
> >   {0, NULL, NULL, NULL, NULL},                    // 0007
> >   {0, NULL, NULL, NULL, NULL},                    // 0008 TPM_ALG_KEYEDHASH
> >   {0, NULL, NULL, NULL, NULL},                    // 0009
> >   {0, NULL, NULL, NULL, NULL},                    // 000A
> >   {SHA256_DIGEST_SIZE, Sha256Init, Sha256Update, Sha256Final,
> > Sha256HashAll}, // 000B TPM_ALG_SHA256
> >   {SHA384_DIGEST_SIZE, Sha384Init, Sha384Update, Sha384Final,
> > Sha384HashAll}, // 000C TPM_ALG_SHA384
> >   {SHA512_DIGEST_SIZE, Sha512Init, Sha512Update, Sha512Final,
> > Sha512HashAll}, // 000D TPM_ALG_SHA512
> >   {0, NULL, NULL, NULL, NULL},                    // 000E
> >   {0, NULL, NULL, NULL, NULL},                    // 000F
> >   {0, NULL, NULL, NULL, NULL},                    // 0010 TPM_ALG_NULL
> > //{0, NULL, NULL, NULL, NULL},                    // 0011
> > //{0, NULL, NULL, NULL, NULL},                    // 0012 TPM_ALG_SM3_256
> > };
> >
> 
> I prefer the code directly index the algorithm info/methods as array. It
> makes code quite simpler.
> 
> > 4) Why not just add one bit say: skip in S3 ? Why need such complexity?
> > #define HASHED_FV_FLAG_SKIP_BOOT_MODE(Mode)   LShiftU64 (0x100,
> > (Mode))
> > #define FV_HASH_FLAG_BOOT_MODE(Mode)          LShiftU64 (1, (Mode))
> >
> > I am not sure how that works. Is boot mode bit start from BIT0 or BIT8 ? I am
> > confused.
> >
> >     if ((StoredHashFvPpi->HashInfo[HashIndex].HashFlag
> >          & FV_HASH_FLAG_BOOT_MODE (BootMode)) != 0) {
> >       HashInfo = &StoredHashFvPpi->HashInfo[HashIndex];
> >       break;
> >     }
> >
> 
> Boot mode is just a const number less than 64. So 64 bits can hold all different
> boot mode. Using this way is just to keep the flexibility to avoid code change if
> we want to support more boot modes besides S3. But if there's never such
> possibility at all, you're right that one bit is enough.
> 
> > 5) Why the producer want skip both verified boot and measured boot? Is that
> > legal or illegal? If it is illegal, I prefer use ASSER() to tell people.
> >     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_VERIFIED_BOOT) == 0 &&
> >         (FvInfo[FvIndex].Flag & HASHED_FV_FLAG_MEASURED_BOOT) == 0) {
> >       continue;
> >     }
> 
> Suppose there's a use case, most likely for developers, which need to disable
> security feature temporarily. The BIOS still need to boot. The developers don't
> need to remove this driver in order to do it. I think it's legacl.
> 
> >
> > 6) I recommend to add one debug message to tell people this is skipped.
> >     //
> >     // Skip any FV not meant for current boot mode.
> >     //
> >     if ((FvInfo[FvIndex].Flag & HASHED_FV_FLAG_SKIP_BOOT_MODE
> > (BootMode)) != 0) {
> >       continue;
> >     }
> >
> 
> Right. I'll add one.
> 
> > 7) Would you please clarify why and when a platform need report multiple
> > StartedHashFv ?
> >   do {
> >     Status = PeiServicesLocatePpi (
> >                &gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid,
> >                Instance,
> >                NULL,
> >                (VOID**)&StoredHashFvPpi
> >                );
> >     if (!EFI_ERROR(Status) && StoredHashFvPpi != NULL && StoredHashFvPpi-
> > >FvNumber > 0) {
> >
> > It will be better, if you can those description in StoredHashFvPpi.h file
> >
> 
> I don't know if there's such necessity. It's just trying to keep a certain of
> flexibility.
> 
> > 8) Same code above, would you please clarify if it is legal or illegal that
> > StoredHashFvPpi->FvNumber == 0 ?
> > If it is illegal, I prefer use ASSERT()
> >
> 
> Let's call it illegal in case of skipping.
> 
> Regards,
> Jian
> 
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Tuesday, June 11, 2019 2:36 AM
> > > To: devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen
> > > <jiewen.yao@intel.com>; Hernandez Beltran, Jorge
> > > <jorge.hernandez.beltran@intel.com>; Han, Harry <harry.han@intel.com>
> > > Subject: [PATCH v2 0/3] Common OBB verification feature
> > >
> > > >V2: fix parameter description error found by ECC
> > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=1617
> > >
> > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran@intel.com>
> > > Cc: Harry Han <harry.han@intel.com>
> > >
> > > Jian J Wang (3):
> > >   SecurityPkg: add definitions for OBB verification
> > >   SecurityPkg/FvReportPei: implement a common FV verifier and reporter
> > >   SecurityPkg: add FvReportPei.inf in dsc for build validation
> > >
> > >  SecurityPkg/FvReportPei/FvReportPei.c         | 418
> > > ++++++++++++++++++
> > >  SecurityPkg/FvReportPei/FvReportPei.h         | 121 +++++
> > >  SecurityPkg/FvReportPei/FvReportPei.inf       |  57 +++
> > >  SecurityPkg/FvReportPei/FvReportPei.uni       |  14 +
> > >  .../FvReportPei/FvReportPeiPeiExtra.uni       |  12 +
> > >  .../Ppi/FirmwareVolumeInfoStoredHashFv.h      |  61 +++
> > >  SecurityPkg/SecurityPkg.dec                   |   9 +
> > >  SecurityPkg/SecurityPkg.dsc                   |   5 +
> > >  8 files changed, 697 insertions(+)
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.c
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.h
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.inf
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPei.uni
> > >  create mode 100644 SecurityPkg/FvReportPei/FvReportPeiPeiExtra.uni
> > >  create mode 100644
> > > SecurityPkg/Include/Ppi/FirmwareVolumeInfoStoredHashFv.h
> > >
> > > --
> > > 2.17.1.windows.2
> 
> 
>