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.zh...@intel.com>; Yao, Jiewen
> <jiewen....@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.belt...@intel.com>; Han, Harry <harry....@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.zh...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.belt...@intel.com>
> Cc: Harry Han <harry....@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42252): https://edk2.groups.io/g/devel/message/42252
Mute This Topic: https://groups.io/mt/32007715/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
