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
>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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42122): https://edk2.groups.io/g/devel/message/42122 Mute This Topic: https://groups.io/mt/32007715/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42401): https://edk2.groups.io/g/devel/message/42401
Mute This Topic: https://groups.io/mt/32007715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42433): https://edk2.groups.io/g/devel/message/42433
Mute This Topic: https://groups.io/mt/32007715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42443): https://edk2.groups.io/g/devel/message/42443
Mute This Topic: https://groups.io/mt/32007715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.