From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f172.google.com (mail-qk1-f172.google.com [209.85.222.172]) by mx.groups.io with SMTP id smtpd.web11.1460.1626162481021215536 for ; Tue, 13 Jul 2021 00:48:01 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@semihalf-com.20150623.gappssmtp.com header.s=20150623 header.b=PhSaG/dX; spf=none, err=SPF record not found (domain: semihalf.com, ip: 209.85.222.172, mailfrom: gjb@semihalf.com) Received: by mail-qk1-f172.google.com with SMTP id m3so6893551qkm.10 for ; Tue, 13 Jul 2021 00:48:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=bXwbUb+6smMLQUfDSARmMvaR8DK2R7ciaL+aTek78MY=; b=PhSaG/dXpuzEIR43DBF+/tjUcf6hlrLQ220LcHFk6XuuV+MFaSQo/uZuPtiPkVUFdH lfOyKg9rAiZ/d8gfKEDP4a6Ig78wW1bWKkZOi5g/qIUX8Ytp2WOsjtvjv9PyLwYoolEi +CAkbVm0Iv962PPQZcvBBa0Y3n+EyEwuIDEmq6U8gCmPj8Byf0goFUPHmTeY6RnIFlAN T+qZa2ZKZ38KIBreNQlt0nk4NcG+FP3QFKQVIKVkFR5Kj8M3xF1ypKR6QCPPxhR70xlV I09NJLk+SWdqiCvF/Qw0/GuWkG3QBs+fukhAxZkmNnc/HIXcoMh/Y3gmER53sv+byg3L yR5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=bXwbUb+6smMLQUfDSARmMvaR8DK2R7ciaL+aTek78MY=; b=KH8qA7uVIzM4r4FjVj+a4Cxa3n5QkuSxAjQ6fdFt37+hswJdELkeu9m02MU5wnEKGN 5oTOK5q5HafVsUFZFwcmDsnnsa5SXJpvY5xfRSSu22WyGoX0NDDjN2p0/3B8sMZ7fj9x t4sWuRJR98/UmkLbt37nJc3lWbkbkFJjvlbxfKT2nURGre1dHFHjEH5AbHDkG+RgB0od KP+y2Uxxt1jxf5y7hjW+qQ50cQjxNbE3MuEYAAnib2+ntJjXF8DR5uoXrUeiuZ/Vnqd5 deksuCuV9mmFkTnNrPBwLAdV5wRKIjcChhufqrL2K6JH8JC/sNWJ8BDSjVHq1qc5Vluq 5n/A== X-Gm-Message-State: AOAM5302dF/E55GNyuSK9fZzc5WYnL5Rm48yonbiASHV3ESLFEbOR+bv 0BbAkSTjm1dZfuYqhOzdCcuCQg23PUWdZYwgwFvWig== X-Google-Smtp-Source: ABdhPJzH36XzdGjBaEXo++dmG+Ds2WZbutW6uMSNm+7kESWtzfxVvWGsL86SkX5hMkPMRiUF+MZAQH+zJZi2i52lJWg= X-Received: by 2002:a37:a110:: with SMTP id k16mr2040751qke.464.1626162480148; Tue, 13 Jul 2021 00:48:00 -0700 (PDT) MIME-Version: 1.0 References: <20210701091758.1057485-1-gjb@semihalf.com> In-Reply-To: From: "Grzegorz Bernacki" Date: Tue, 13 Jul 2021 09:47:49 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH v5 00/10] Secure Boot default keys To: "Yao, Jiewen" Cc: Samer El-Haj-Mahmoud , "devel@edk2.groups.io" , "spbrogan@outlook.com" , "leif@nuviainc.com" , "ardb+tianocore@kernel.org" , Sunny Wang , "mw@semihalf.com" , "upstream@semihalf.com" , "Wang, Jian J" , "Xu, Min M" , "lersek@redhat.com" , Sami Mujawar , "afish@apple.com" , "Ni, Ray" , "Justen, Jordan L" , "rebecca@bsdio.com" , "grehan@freebsd.org" , Thomas Abraham , "Chiu, Chasel" , "Desimone, Nathaniel L" , "gaoliming@byosoft.com.cn" , "Dong, Eric" , "Kinney, Michael D" , "Sun, Zailiang" , "Qian, Yi" , "graeme@nuviainc.com" , "rad@semihalf.com" , "pete@akeo.ie" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 napisa=C5=82(= 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(), CreateTim= eBasedPayload(), DeleteDb(), DeleteDbx(), DeleteDbt(), DeleteKEK(), DeleteP= latformKey() > > 2) SecureBootVariableProvisionLib: EnrollDbFromDefault(), EnrollDbxFromD= efault(), EnrollDbtFromDefault(), EnrollKEKFromDefault(), EnrollPKFromDefau= lt(), SecureBootInitPKDefault(), SecureBootInitKEKDefault(), SecureBootInit= dbDefault(), SecureBootInitdbtDefault(), SecureBootInitdbxDefault(), > > Other minor feedback, the name of SecureBootInitdbDefault() should be Se= cureBootInitDbDefault() - capital D in Db. > > > > > Thank you > Yao Jiewen > > > > -----Original Message----- > > From: Samer El-Haj-Mahmoud > > 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 > > ; mw@semihalf.com; upstream@semihalf.com; Yao, > > Jiewen ; Wang, Jian J ; X= u, Min > > M ; lersek@redhat.com; Sami Mujawar > > ; afish@apple.com; Ni, Ray ; > > Justen, Jordan L ; rebecca@bsdio.com; > > grehan@freebsd.org; Thomas Abraham ; Chiu, > > Chasel ; Desimone, Nathaniel L > > ; gaoliming@byosoft.com.cn; Dong, Eric > > ; Kinney, Michael D ;= Sun, > > Zailiang ; Qian, Yi ; > > graeme@nuviainc.com; rad@semihalf.com; pete@akeo.ie; Samer El-Haj- > > Mahmoud > > 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 Secur= ityPkg > > today, and the improvement you are suggesting is welcomed, especially = for > > systems that rely on EDK2 (and lack a commercial FW solution). Conside= ring that > > this patch series is at v5, and has accumulated enough reviews since R= FC/v1 was > > sent to the list in April/May, is it possible to proceed with the curr= ent revision, > > and consider the feedback you suggested in future improvements to Secu= rityPkg? > > > > Thanks, > > --Samer > > > > > > > -----Original Message----- > > > From: 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-Mahmo= ud > > > ; Sunny Wang > > > ; 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 ; > > > afish@apple.com; ray.ni@intel.com; jordan.l.justen@intel.com; > > > rebecca@bsdio.com; grehan@freebsd.org; Thomas Abraham > > > ; 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 yo= u > > > 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 resolv= e a > > > major issue within the SecurityPkg design today. Not that it has to= , > > > but when creating new abstractions/APIs it would be ideal this probl= em. > > > 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 S= B > > > databases (platform policy), as well as helper functions for creatin= g > > > variable auth payloads, sig lists, etc (spec defined data manipulati= on). > > > If this library was refactored into two libraries (a pure data > > > manipulation library and platform lib) it would significantly improv= e > > > the usefulness of this library (to me and i suspect many other consu= mers > > > of edk2). > > > > > > 1. Reduce the number of forks or instances other consumers would nee= d. > > > Other consumers of edk2 could use the data manipulation lib without > > > taking on the burden of the platform config stuff that may or may no= t > > > apply to their platform. Other consumers might also then help maint= ain > > > 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 hi= gher > > > confidence in code and changes and most likely reduce quality issues= . > > > > > > 3. A platform lib would make clear the platform requirements for usi= ng > > > 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 i= f > > > 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 =3D> SecureBootVariableLib > > > > SecBootDefaultKeysDxe =3D> SecureBootDefaultKeysDxe > > > > SecEnrollDefaultKeysApp =3D> 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 PlatformVarCleanupL= ib > > > > - 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 SecureBootConfigD= xe. > > > > 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.in= f > > > | 47 + > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.= inf > > > | 79 ++ > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf= i > > > gDxe.inf | 2 + > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoo= t > > > DefaultKeysDxe.inf | 45 + > > > > SecurityPkg/Include/Library/SecureBootVariableLib.h = | > > > 251 +++++ > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf= i > > > gNvData.h | 2 + > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf= i > > > g.vfr | 6 + > > > > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c > > > | 109 +++ > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.= c > > > | 980 ++++++++++++++++++++ > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf= i > > > gImpl.c | 343 ++++--- > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoo= t > > > DefaultKeysDxe.c | 68 ++ > > > > ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc = | > > > 70 ++ > > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.= uni > > > | 16 + > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf= i > > > gStrings.uni | 4 + > > > > > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoo= t > > > 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/SecureBoo= t > > > DefaultKeysDxe.inf > > > > create mode 100644 SecurityPkg/Include/Library/SecureBootVariabl= eLib.h > > > > create mode 100644 > > > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c > > > > create mode 100644 > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c > > > > create mode 100644 > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoo= t > > > DefaultKeysDxe.c > > > > create mode 100644 ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc > > > > create mode 100644 > > > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni > > > > create mode 100644 > > > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBoo= t > > > DefaultKeysDxe.uni > > > > > > > > > > > > >=20 > > > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended r= ecipient, > > 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.