Appreciate your help on that. Thank you Yao Jiewen
> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Grzegorz > Bernacki > Sent: Tuesday, July 13, 2021 3:48 PM > To: Yao, Jiewen <jiewen....@intel.com> > Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; > devel@edk2.groups.io; spbro...@outlook.com; l...@nuviainc.com; > ardb+tianoc...@kernel.org; Sunny Wang <sunny.w...@arm.com>; > m...@semihalf.com; upstr...@semihalf.com; Wang, Jian J > <jian.j.w...@intel.com>; Xu, Min M <min.m...@intel.com>; > ler...@redhat.com; Sami Mujawar <sami.muja...@arm.com>; > af...@apple.com; Ni, Ray <ray...@intel.com>; Justen, Jordan L > <jordan.l.jus...@intel.com>; rebe...@bsdio.com; gre...@freebsd.org; > Thomas Abraham <thomas.abra...@arm.com>; Chiu, Chasel > <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; gaolim...@byosoft.com.cn; Dong, Eric > <eric.d...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; Sun, > Zailiang <zailiang....@intel.com>; Qian, Yi <yi.q...@intel.com>; > gra...@nuviainc.com; r...@semihalf.com; p...@akeo.ie > Subject: Re: [edk2-devel] [PATCH v5 00/10] Secure Boot default keys > > Hi Jiewen, > > I think it is a good idea to split it this way. Please let me prepare > next version of the patch. > thanks, > greg > > pon., 12 lip 2021 o 14:02 Yao, Jiewen <jiewen....@intel.com> napisał(a): > > > > I think Sean's feedback is a great idea to split the platform part from > > core part. > > > > Even a simple split would be helpful. How about below: > > 1) SecureBootVariableLib: SetSecureBootMode(), GetSetupMode(), > CreateTimeBasedPayload(), DeleteDb(), DeleteDbx(), DeleteDbt(), DeleteKEK(), > DeletePlatformKey() > > > > 2) SecureBootVariableProvisionLib: EnrollDbFromDefault(), > EnrollDbxFromDefault(), EnrollDbtFromDefault(), EnrollKEKFromDefault(), > EnrollPKFromDefault(), SecureBootInitPKDefault(), SecureBootInitKEKDefault(), > SecureBootInitdbDefault(), SecureBootInitdbtDefault(), > SecureBootInitdbxDefault(), > > > > Other minor feedback, the name of SecureBootInitdbDefault() should be > SecureBootInitDbDefault() - capital D in Db. > > > > > > > > > > Thank you > > Yao Jiewen > > > > > > > -----Original Message----- > > > From: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com> > > > Sent: Saturday, July 10, 2021 4:03 AM > > > To: devel@edk2.groups.io; spbro...@outlook.com; g...@semihalf.com > > > Cc: l...@nuviainc.com; ardb+tianoc...@kernel.org; Sunny Wang > > > <sunny.w...@arm.com>; m...@semihalf.com; upstr...@semihalf.com; > Yao, > > > Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Xu, > Min > > > M <min.m...@intel.com>; ler...@redhat.com; Sami Mujawar > > > <sami.muja...@arm.com>; af...@apple.com; Ni, Ray <ray...@intel.com>; > > > Justen, Jordan L <jordan.l.jus...@intel.com>; rebe...@bsdio.com; > > > gre...@freebsd.org; Thomas Abraham <thomas.abra...@arm.com>; Chiu, > > > Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L > > > <nathaniel.l.desim...@intel.com>; gaolim...@byosoft.com.cn; Dong, Eric > > > <eric.d...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > Sun, > > > Zailiang <zailiang....@intel.com>; Qian, Yi <yi.q...@intel.com>; > > > gra...@nuviainc.com; r...@semihalf.com; p...@akeo.ie; Samer El-Haj- > > > Mahmoud <samer.el-haj-mahm...@arm.com> > > > Subject: RE: [edk2-devel] [PATCH v5 00/10] Secure Boot default keys > > > > > > Sean, > > > > > > Thanks for the feedback. As you say, this is a design concern in > > > SecurityPkg > > > today, and the improvement you are suggesting is welcomed, especially for > > > systems that rely on EDK2 (and lack a commercial FW solution). Considering > that > > > this patch series is at v5, and has accumulated enough reviews since > > > RFC/v1 > was > > > sent to the list in April/May, is it possible to proceed with the current > > > revision, > > > and consider the feedback you suggested in future improvements to > SecurityPkg? > > > > > > Thanks, > > > --Samer > > > > > > > > > > -----Original Message----- > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean > via > > > > groups.io > > > > Sent: Friday, July 9, 2021 2:23 PM > > > > To: devel@edk2.groups.io; g...@semihalf.com > > > > Cc: l...@nuviainc.com; ardb+tianoc...@kernel.org; Samer El-Haj- > Mahmoud > > > > <samer.el-haj-mahm...@arm.com>; Sunny Wang > > > > <sunny.w...@arm.com>; m...@semihalf.com; upstr...@semihalf.com; > > > > jiewen....@intel.com; jian.j.w...@intel.com; min.m...@intel.com; > > > > ler...@redhat.com; Sami Mujawar <sami.muja...@arm.com>; > > > > af...@apple.com; ray...@intel.com; jordan.l.jus...@intel.com; > > > > rebe...@bsdio.com; gre...@freebsd.org; Thomas Abraham > > > > <thomas.abra...@arm.com>; chasel.c...@intel.com; > > > > nathaniel.l.desim...@intel.com; gaolim...@byosoft.com.cn; > > > > eric.d...@intel.com; michael.d.kin...@intel.com; > zailiang....@intel.com; > > > > yi.q...@intel.com; gra...@nuviainc.com; r...@semihalf.com; > > > > p...@akeo.ie > > > > Subject: Re: [edk2-devel] [PATCH v5 00/10] Secure Boot default keys > > > > > > > > Grzegorz, > > > > > > > > It is a little late to the party to provide broad feedback (given you > > > > are on v5) but i'll do it anyway and if anything resonates maybe you can > > > > make a few changes. > > > > > > > > > > > > This patchset (for modules/libraries in SecurityPkg) does not resolve a > > > > major issue within the SecurityPkg design today. Not that it has to, > > > > but when creating new abstractions/APIs it would be ideal this problem. > > > > The SecurityPkg modules and libraries today mix platform > > > > policy/assumptions with generic data manipulation and specification > > > > defined behavior. > > > > > > > > For example the new SecureBootVariableLib. This library contains > > > > functions that load default keys from flash (platform), delete the SB > > > > databases (platform policy), as well as helper functions for creating > > > > variable auth payloads, sig lists, etc (spec defined data manipulation). > > > > If this library was refactored into two libraries (a pure data > > > > manipulation library and platform lib) it would significantly improve > > > > the usefulness of this library (to me and i suspect many other consumers > > > > of edk2). > > > > > > > > 1. Reduce the number of forks or instances other consumers would need. > > > > Other consumers of edk2 could use the data manipulation lib without > > > > taking on the burden of the platform config stuff that may or may not > > > > apply to their platform. Other consumers might also then help maintain > > > > this library because they would be using it in their platform. > > > > > > > > 2. A data manipulation library could be easily unit tested using the > > > > host based unit test framework. This would provide significantly higher > > > > confidence in code and changes and most likely reduce quality issues. > > > > > > > > 3. A platform lib would make clear the platform requirements for using > > > > the modules and applications and allow platform maintainers to focus on > > > > this API and dependencies. > > > > > > > > Anyway, given how long and tedious the edk2 contribution process is and > > > > that you already have most of the SecurityPkg RBs I can understand if > > > > this unwelcome feedback. > > > > > > > > Thanks > > > > Sean > > > > > > > > > > > > > > > > > > > > On 7/1/2021 2:17 AM, Grzegorz Bernacki wrote: > > > > > This patchset adds support for initialization of default > > > > > Secure Boot variables based on keys content embedded in > > > > > flash binary. This feature is active only if Secure Boot > > > > > is enabled and DEFAULT_KEY is defined. The patchset > > > > > consist also application to enroll keys from default > > > > > variables and secure boot menu change to allow user > > > > > to reset key content to default values. > > > > > Discussion on design can be found at: > > > > > https://edk2.groups.io/g/rfc/topic/82139806#600 > > > > > > > > > > Built with: > > > > > GCC > > > > > - RISC-V (U500, U540) [requires fixes in dsc to build] > > > > > - Intel (Vlv2TbltDevicePkg (X64/IA32), Quark, MinPlatformPkg, > > > > > EmulatorPkg (X64), Bhyve, OvmfPkg (X64/IA32)) > > > > > - ARM (Sgi75,SbsaQemu,DeveloperBox, RPi3/RPi4) > > > > > > > > > > RISC-V, Quark, Vlv2TbltDevicePkg, Bhyve requires additional fixes to > > > > > be > > > > built, > > > > > will be post on edk2 maillist later > > > > > > > > > > VS2019 > > > > > - Intel (OvmfPkgX64) > > > > > > > > > > Test with: > > > > > GCC5/RPi4 > > > > > VS2019/OvmfX64 (requires changes to enable feature) > > > > > > > > > > Tests: > > > > > 1. Try to enroll key in incorrect format. > > > > > 2. Enroll with only PKDefault keys specified. > > > > > 3. Enroll with all keys specified. > > > > > 4. Enroll when keys are enrolled. > > > > > 5. Reset keys values. > > > > > 6. Running signed & unsigned app after enrollment. > > > > > > > > > > Changes since v1: > > > > > - change names: > > > > > SecBootVariableLib => SecureBootVariableLib > > > > > SecBootDefaultKeysDxe => SecureBootDefaultKeysDxe > > > > > SecEnrollDefaultKeysApp => EnrollFromDefaultKeysApp > > > > > - change name of function CheckSetupMode to GetSetupMode > > > > > - remove ShellPkg dependecy from EnrollFromDefaultKeysApp > > > > > - rebase to master > > > > > > > > > > Changes since v2: > > > > > - fix coding style for functions headers in SecureBootVariableLib.h > > > > > - add header to SecureBootDefaultKeys.fdf.inc > > > > > - remove empty line spaces in SecureBootDefaultKeysDxe files > > > > > - revert FAIL macro in EnrollFromDefaultKeysApp > > > > > - remove functions duplicates and add SecureBootVariableLib > > > > > to platforms which used it > > > > > > > > > > Changes since v3: > > > > > - move SecureBootDefaultKeys.fdf.inc to ArmPlatformPkg > > > > > - leave duplicate of CreateTimeBasedPayload in PlatformVarCleanupLib > > > > > - fix typo in guid description > > > > > > > > > > Changes since v4: > > > > > - reorder patches to make it bisectable > > > > > - split commits related to more than one platform > > > > > - move edk2-platform commits to separate patchset > > > > > > > > > > Grzegorz Bernacki (10): > > > > > SecurityPkg: Create library for setting Secure Boot variables. > > > > > ArmVirtPkg: add SecureBootVariableLib class resolution > > > > > OvmfPkg: add SecureBootVariableLib class resolution > > > > > EmulatorPkg: add SecureBootVariableLib class resolution > > > > > SecurityPkg: Remove duplicated functions from SecureBootConfigDxe. > > > > > ArmPlatformPkg: Create include file for default key content. > > > > > SecurityPkg: Add SecureBootDefaultKeysDxe driver > > > > > SecurityPkg: Add EnrollFromDefaultKeys application. > > > > > SecurityPkg: Add new modules to Security package. > > > > > SecurityPkg: Add option to reset secure boot keys. > > > > > > > > > > SecurityPkg/SecurityPkg.dec > > > > > | 14 + > > > > > ArmVirtPkg/ArmVirt.dsc.inc > > > > > | 1 + > > > > > EmulatorPkg/EmulatorPkg.dsc > > > > > | 1 + > > > > > OvmfPkg/Bhyve/BhyveX64.dsc > > > > > | 1 + > > > > > OvmfPkg/OvmfPkgIa32.dsc > > > > > | 1 + > > > > > OvmfPkg/OvmfPkgIa32X64.dsc > > > > > | 1 + > > > > > OvmfPkg/OvmfPkgX64.dsc > > > > > | 1 + > > > > > SecurityPkg/SecurityPkg.dsc > > > > > | 4 + > > > > > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.inf > > > > | 47 + > > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf > > > > | 79 ++ > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > > > > gDxe.inf | 2 + > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoot > > > > DefaultKeysDxe.inf | 45 + > > > > > SecurityPkg/Include/Library/SecureBootVariableLib.h > | > > > > 251 +++++ > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > > > > gNvData.h | 2 + > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > > > > g.vfr | 6 + > > > > > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c > > > > | 109 +++ > > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c > > > > | 980 ++++++++++++++++++++ > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > > > > gImpl.c | 343 ++++--- > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoot > > > > DefaultKeysDxe.c | 68 ++ > > > > > ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc > | > > > > 70 ++ > > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni > > > > | 16 + > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > > > > gStrings.uni | 4 + > > > > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoot > > > > DefaultKeysDxe.uni | 16 + > > > > > 23 files changed, 1874 insertions(+), 188 deletions(-) > > > > > create mode 100644 > > > > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.inf > > > > > create mode 100644 > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf > > > > > create mode 100644 > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoot > > > > DefaultKeysDxe.inf > > > > > create mode 100644 > SecurityPkg/Include/Library/SecureBootVariableLib.h > > > > > create mode 100644 > > > > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c > > > > > create mode 100644 > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c > > > > > create mode 100644 > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoot > > > > DefaultKeysDxe.c > > > > > create mode 100644 ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc > > > > > create mode 100644 > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni > > > > > create mode 100644 > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoot > > > > DefaultKeysDxe.uni > > > > > > > > > > > > > > > > > > > > > > > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > > confidential and may also be privileged. If you are not the intended > > > recipient, > > > please notify the sender immediately and do not disclose the contents to > > > any > > > other person, use it for any purpose, or store or copy the information in > > > any > > > medium. Thank you. > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77742): https://edk2.groups.io/g/devel/message/77742 Mute This Topic: https://groups.io/mt/83912187/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
