[edk2-devel] [PATCH v5 00/10] Secure Boot default keys
Yao, Jiewen
jiewen.yao at intel.com
Tue Jul 13 07:54:49 UTC 2021
Appreciate your help on that.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Grzegorz
> Bernacki
> Sent: Tuesday, July 13, 2021 3:48 PM
> To: Yao, Jiewen <jiewen.yao at intel.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>;
> devel at edk2.groups.io; spbrogan at outlook.com; leif at nuviainc.com;
> ardb+tianocore at kernel.org; Sunny Wang <Sunny.Wang at arm.com>;
> mw at semihalf.com; upstream at semihalf.com; Wang, Jian J
> <jian.j.wang at intel.com>; Xu, Min M <min.m.xu at intel.com>;
> lersek at redhat.com; Sami Mujawar <Sami.Mujawar at arm.com>;
> afish at apple.com; Ni, Ray <ray.ni at intel.com>; Justen, Jordan L
> <jordan.l.justen at intel.com>; rebecca at bsdio.com; grehan at freebsd.org;
> Thomas Abraham <thomas.abraham at arm.com>; Chiu, Chasel
> <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; gaoliming at byosoft.com.cn; Dong, Eric
> <eric.dong at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>; Sun,
> Zailiang <zailiang.sun at intel.com>; Qian, Yi <yi.qian at intel.com>;
> graeme at nuviainc.com; rad at semihalf.com; pete at 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.yao at 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-Mahmoud at arm.com>
> > > Sent: Saturday, July 10, 2021 4:03 AM
> > > To: devel at edk2.groups.io; spbrogan at outlook.com; gjb at semihalf.com
> > > Cc: leif at nuviainc.com; ardb+tianocore at kernel.org; Sunny Wang
> > > <Sunny.Wang at arm.com>; mw at semihalf.com; upstream at semihalf.com;
> Yao,
> > > Jiewen <jiewen.yao at intel.com>; Wang, Jian J <jian.j.wang at intel.com>; Xu,
> Min
> > > M <min.m.xu at intel.com>; lersek at redhat.com; Sami Mujawar
> > > <Sami.Mujawar at arm.com>; afish at apple.com; Ni, Ray <ray.ni at intel.com>;
> > > Justen, Jordan L <jordan.l.justen at intel.com>; rebecca at bsdio.com;
> > > grehan at freebsd.org; Thomas Abraham <thomas.abraham at arm.com>; Chiu,
> > > Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> > > <nathaniel.l.desimone at intel.com>; gaoliming at byosoft.com.cn; Dong, Eric
> > > <eric.dong at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>;
> Sun,
> > > Zailiang <zailiang.sun at intel.com>; Qian, Yi <yi.qian at intel.com>;
> > > graeme at nuviainc.com; rad at semihalf.com; pete at akeo.ie; Samer El-Haj-
> > > Mahmoud <Samer.El-Haj-Mahmoud at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Sean
> via
> > > > groups.io
> > > > Sent: Friday, July 9, 2021 2:23 PM
> > > > To: devel at edk2.groups.io; gjb at semihalf.com
> > > > Cc: leif at nuviainc.com; ardb+tianocore at kernel.org; Samer El-Haj-
> Mahmoud
> > > > <Samer.El-Haj-Mahmoud at arm.com>; Sunny Wang
> > > > <Sunny.Wang at arm.com>; mw at semihalf.com; upstream at semihalf.com;
> > > > jiewen.yao at intel.com; jian.j.wang at intel.com; min.m.xu at intel.com;
> > > > lersek at redhat.com; Sami Mujawar <Sami.Mujawar at arm.com>;
> > > > afish at apple.com; ray.ni at intel.com; jordan.l.justen at intel.com;
> > > > rebecca at bsdio.com; grehan at freebsd.org; Thomas Abraham
> > > > <thomas.abraham at arm.com>; chasel.chiu at intel.com;
> > > > nathaniel.l.desimone at intel.com; gaoliming at byosoft.com.cn;
> > > > eric.dong at intel.com; michael.d.kinney at intel.com;
> zailiang.sun at intel.com;
> > > > yi.qian at intel.com; graeme at nuviainc.com; rad at semihalf.com;
> > > > pete at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list