[edk2-devel] [PATCH v2 0/3] Common OBB verification feature
Wang, Jian J
jian.j.wang at intel.com
Fri Jun 14 00:29:47 UTC 2019
Jiewen,
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, June 12, 2019 12:49 PM
> To: Wang, Jian J <jian.j.wang at intel.com>; devel at edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang at intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.beltran at intel.com>; Han, Harry <harry.han at 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 at edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang at intel.com>; Yao, Jiewen
> > <jiewen.yao at intel.com>; Hernandez Beltran, Jorge
> > <jorge.hernandez.beltran at intel.com>; Han, Harry <harry.han at 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 at intel.com>
> > Cc: Jiewen Yao <jiewen.yao at intel.com>
> > Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.beltran at intel.com>
> > Cc: Harry Han <harry.han at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list