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

["Wang, Jian J" <jian.j.wang@intel.com>]

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
> 
> 
>