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

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

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