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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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