From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (NAM10-BN7-obe.outbound.protection.outlook.com [40.92.40.24]) by mx.groups.io with SMTP id smtpd.web12.14952.1625854965631098450 for ; Fri, 09 Jul 2021 11:22:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=PLLBVFET; spf=pass (domain: outlook.com, ip: 40.92.40.24, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=brzV1GwunjNjtHDGzIxZviSA8/y0EZH0/xnhMbQBC/VVg/do4Bgo9q39X4nzoqqMJSsU3wfdware9f40V5XAsqDEu++t7AlHmKxUR72EJT+14D60WrGUh8ZTfBwc3ghHImcFifLk6RZtZfcN/LLChUNiSr2h5K7vpi51p7I+sjGHhqamsImE/NHxvkJnyhPmMoKslE8HDZanfqfXIKx7zCtxO2mNzGYUWNShBuDgphIlDR28Cl63+/67qgSH75TsNWOhS1bklrWHOSfHX0/qgfpT57C2N10XBKXYisa8qm9XrU53D8VmgxZojTtaRiD9tGtxepNVIDSIsddFhQ7QOQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=EpbrLJScMA0GAG58cZgVztvMgCR0IcSBRgNHhsp+4Ls=; b=hB+LJeGQDbDzrvY3bX2VD783PJTfkPlDz4w9dAvhvmIXS6MIoXpHg5SRmZEGr5ZRmvVSmmp8Y0cYFCXubK0iLN/strIL2K+z6GwFiYKGFdG90ltXTYX5WiV993iVknZfddl8oXMyUpE8UlvLRAKaGYYydvNnHTIq9oeZe+tegz70pQdKBh5RLN2HLXyoxL82OoPY+u9XFEcnNDODlumNt5Q3ARpL/+TlOPKd1e2oxalja+8TfwHEeVHHvuHsCSExjRUsm5l5Sxp/8VEsf2Ov0sFSfhqY55Oj4QG2VkoDLZO/lEfkzVgDKSVazQxz8GtK2JsVAvSTpQ2VpOXKwNHZww== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=EpbrLJScMA0GAG58cZgVztvMgCR0IcSBRgNHhsp+4Ls=; b=PLLBVFETyzf9GR2fYuVi2gqkp9VkNdaBB5Vof3Wqt47AJhhENORMyiLd1fg/s4TlfiHdglXVPTSiOQs3RgSOl6iJNIWcfwXqxs/uxHzQMIdIL3FkTMduVs/flyehv8Bf7AonAQrEh9y8Fhcp4wPBXGQ1UD59FI5TJu5xF3CbMvnW7yZ4cUClR1PE+wQD05TRIHcV3LHtZDzldsza6unaKRULDn/4SgZFOEVNNV9nEyklyaCIKP7evjyDpsrhDGDKE/bqcZrtKjqMrNpDdbzlzH0xp8HwL+DTCM1EazIxpDDKjlCPmhpKdBb1GoiNeON+402Hg51vNSRkxYun85jhvQ== Received: from DM6NAM10FT008.eop-nam10.prod.protection.outlook.com (2a01:111:e400:7e86::42) by DM6NAM10HT123.eop-nam10.prod.protection.outlook.com (2a01:111:e400:7e86::109) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.20; Fri, 9 Jul 2021 18:22:44 +0000 Received: from BY3PR19MB4900.namprd19.prod.outlook.com (2a01:111:e400:7e86::42) by DM6NAM10FT008.mail.protection.outlook.com (2a01:111:e400:7e86::290) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.20 via Frontend Transport; Fri, 9 Jul 2021 18:22:44 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:A507CACF666F72064E998E37113D49E54D8681B5C06FC57274B29033CECDC390;UpperCasedChecksum:F3F72BF38954B5997840F6BDAE9801F183FEF8FE6325FE12105352145E4219FB;SizeAsReceived:9248;Count:48 Received: from BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::ace7:1da0:cc6e:c52]) by BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::ace7:1da0:cc6e:c52%6]) with mapi id 15.20.4308.023; Fri, 9 Jul 2021 18:22:44 +0000 Subject: Re: [edk2-devel] [PATCH v5 00/10] Secure Boot default keys To: devel@edk2.groups.io, gjb@semihalf.com Cc: leif@nuviainc.com, ardb+tianocore@kernel.org, Samer.El-Haj-Mahmoud@arm.com, 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@arm.com, afish@apple.com, ray.ni@intel.com, jordan.l.justen@intel.com, rebecca@bsdio.com, grehan@freebsd.org, 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 References: <20210701091758.1057485-1-gjb@semihalf.com> From: "Sean" Message-ID: Date: Fri, 9 Jul 2021 11:22:41 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 In-Reply-To: <20210701091758.1057485-1-gjb@semihalf.com> X-TMN: [NUbwea+i3kZxtYyIKbFGVHcE3U/iznrC] X-ClientProxiedBy: MW4PR04CA0311.namprd04.prod.outlook.com (2603:10b6:303:82::16) To BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.2.78] (50.47.113.221) by MW4PR04CA0311.namprd04.prod.outlook.com (2603:10b6:303:82::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.19 via Frontend Transport; Fri, 9 Jul 2021 18:22:42 +0000 X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 48 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: 8666d283-da3a-4096-4217-08d943068b7b X-MS-TrafficTypeDiagnostic: DM6NAM10HT123: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: PFA49LVIlv9v6ZvmDZ9ruiac+/2+WKBTm8joYrebcUKeaNqWothoBHRd+23kRucUCAOL/vXuTLwpgMJLYU6MUCVLXsnxV4MAhRJZBio9EC1ldXfzhVGRpdVS2PvsffltrfGFLHKdZRa9MvYRJBi90lMgkHRt2S7HPU7uTvsZKJnkqenBRofp1RcGiEGFyq7YOYiiQDvPOx5R/CT0T+tzsy3jax68L5kr7pIC2sFv3UBm+BPXxKLNb2v2n9yfjWmGmRvAeUAvaNXDsxKltNGiJoF0Dl5ylMB9NZI4DsBSXbaPTu/4iwioN5v2W9lEZGzOd79AT8hEaLengXuSikwhCcNBe8drM/Ektewx74Q6ZuLKJdRGz/CZ6DPc6oM09d9LxXWCEgLB56qjkL1XHuAs5G+Ca5vWT4RcTUgY64+0tykwil9nX8cdUPSV6H79hO+EXRTHDY60yfshVLyRpUyWcM3p+ubgSUo8OxS1qrzWi1s= X-MS-Exchange-AntiSpam-MessageData: OxwW+4u8cDpFwmYoKxJsxNfb19VDoWqBmS5ncO0tMcNeJbk0jJ+AX+DSWZ/g3mtYcZGp2Q3gCo5xGq3RMAueeDVNVmRMcSaiv8vPE1zgOlGwmL9llDdaMpmUlifJxJwHPBGsUrQGVwnh/zUCMnWYlg== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8666d283-da3a-4096-4217-08d943068b7b X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jul 2021 18:22:44.1837 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-AuthSource: DM6NAM10FT008.eop-nam10.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: Internet X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6NAM10HT123 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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/SecureBootConfigDxe.inf | 2 + > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.inf | 45 + > SecurityPkg/Include/Library/SecureBootVariableLib.h | 251 +++++ > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigNvData.h | 2 + > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig.vfr | 6 + > SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c | 109 +++ > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c | 980 ++++++++++++++++++++ > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 343 ++++--- > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c | 68 ++ > ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc | 70 ++ > SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni | 16 + > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigStrings.uni | 4 + > SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.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/SecureBootDefaultKeysDxe.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/SecureBootDefaultKeysDxe.c > create mode 100644 ArmPlatformPkg/SecureBootDefaultKeys.fdf.inc > create mode 100644 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.uni > create mode 100644 SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.uni >