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

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

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