Jiewen,
See my comments below.
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, June 14, 2019 6:41 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Hernandez Beltran, Jorge
> <jorge.hernandez.belt...@intel.com>; Han, Harry <harry....@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....@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Hernandez Beltran, Jorge
> > <jorge.hernandez.belt...@intel.com>; Han, Harry <harry....@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.w...@intel.com>; devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Hernandez Beltran, Jorge
> > > <jorge.hernandez.belt...@intel.com>; Han, Harry <harry....@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.zh...@intel.com>; Yao, Jiewen
> > > > <jiewen....@intel.com>; Hernandez Beltran, Jorge
> > > > <jorge.hernandez.belt...@intel.com>; Han, Harry
> > <harry....@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.zh...@intel.com>
> > > > Cc: Jiewen Yao <jiewen....@intel.com>
> > > > Cc: "Hernandez Beltran, Jorge" <jorge.hernandez.belt...@intel.com>
> > > > Cc: Harry Han <harry....@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 (#42443): https://edk2.groups.io/g/devel/message/42443
Mute This Topic: https://groups.io/mt/32007715/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
