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

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

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