Re: [edk2-devel] [PATCH v5 00/10] Secure Boot default keys

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

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-Mahmoud@arm.com>
> Sent: Saturday, July 10, 2021 4:03 AM
> To: devel@edk2.groups.io; spbrogan@outlook.com; gjb@semihalf.com
> Cc: leif@nuviainc.com; ardb+tianocore@kernel.org; Sunny Wang
> <Sunny.Wang@arm.com>; mw@semihalf.com; upstream@semihalf.com; Yao,
> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Xu, Min
> M <min.m.xu@intel.com>; lersek@redhat.com; Sami Mujawar
> <Sami.Mujawar@arm.com>; afish@apple.com; Ni, Ray <ray.ni@intel.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>; rebecca@bsdio.com;
> grehan@freebsd.org; Thomas Abraham <thomas.abraham@arm.com>; Chiu,
> Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; gaoliming@byosoft.com.cn; Dong, Eric
> <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Sun,
> Zailiang <zailiang.sun@intel.com>; Qian, Yi <yi.qian@intel.com>;
> graeme@nuviainc.com; rad@semihalf.com; pete@akeo.ie; Samer El-Haj-
> Mahmoud <Samer.El-Haj-Mahmoud@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; gjb@semihalf.com
> > Cc: leif@nuviainc.com; ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud
> > <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang
> > <Sunny.Wang@arm.com>; mw@semihalf.com; upstream@semihalf.com;
> > jiewen.yao@intel.com; jian.j.wang@intel.com; min.m.xu@intel.com;
> > lersek@redhat.com; Sami Mujawar <Sami.Mujawar@arm.com>;
> > afish@apple.com; ray.ni@intel.com; jordan.l.justen@intel.com;
> > rebecca@bsdio.com; grehan@freebsd.org; Thomas Abraham
> > <thomas.abraham@arm.com>; chasel.chiu@intel.com;
> > nathaniel.l.desimone@intel.com; gaoliming@byosoft.com.cn;
> > eric.dong@intel.com; michael.d.kinney@intel.com; zailiang.sun@intel.com;
> > yi.qian@intel.com; graeme@nuviainc.com; rad@semihalf.com;
> > pete@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.