From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web09.2649.1652755738054961234 for ; Mon, 16 May 2022 19:48:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=W/AsopLi; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.195.228.134]) by linux.microsoft.com (Postfix) with ESMTPSA id 30AEB20F3D15; Mon, 16 May 2022 19:48:57 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 30AEB20F3D15 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1652755737; bh=6KGvf6cA4mkWXnEIUo2Pis2L0dWceIc5cQCLdT8dgJI=; h=Date:Subject:To:References:From:In-Reply-To:From; b=W/AsopLihkfURIhNb0FBJz4LSiaKo/lJiWlH7OO9GNVS2npzVJJGAHOGZt/FQP1Px 6LTQT2rPi2acW6L/e3Z0A8qUDOhBvZMYG18Zxk4Pni5Dhi2tsg0OqQR1OJkyOdUsEb QMhxzyoHFpB1jV6Jf2/ET1lXcEV35H3B+8sfmJHk= Message-ID: <672bd35e-b376-7d7d-dc0c-9b0dc23a9b3c@linux.microsoft.com> Date: Mon, 16 May 2022 22:48:56 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [edk2-devel] [Patch v2 00/28] UEFI variable protection To: devel@edk2.groups.io, judah.vang@intel.com References: <20220429180430.3292-1-judah.vang@intel.com> From: "Michael Kubacki" In-Reply-To: <20220429180430.3292-1-judah.vang@intel.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi Judah, Do you have reference information for the following? 1. Overall boot time impact for a sample variable store? - In particular: - Initial HMAC calculation/verification time. - Non-volatile write impact time to caluclate new store HMAC value and update MetaDataHmacVar. - Variable reclaim before and after time. 2. Overall non-volatile store size overhead impact with AES-CBC encrypted variables? I understand these will vary based on system properties like SPI flash parameters, cryptographic processor details, etc. I'm trying to get an idea of the impact from sample data or averages on a particular system configuration. Also to learn whether the native encryption instruction (AES-NI) was used and if that could provide any benefit given the potential number of encryption/decryption operations introduced. For the code design, I feel the ProtectedVariableLib interface is a bit too coupled against internal implementation details of the variable driver. I generally understand why the code is split out to wrap operations around the new functionality and it follows the AuthVarLib pattern but changing the library or driver will continue to require large changes across both like this due to the coupling. Small things I noticed: 1. VariableKeyLib.inf should not be "BASE", it directly depends on PEI services 2. Typo "varabile" in some files 3. Does ProtectedVariableLibNull actually need to depend on BaseMemoryLib? Thanks, Michael On 4/29/2022 2:04 PM, Judah Vang wrote: > For a more detail description of the UEFI variable protected feature you can > view the Readme.md located at the following location: > https://github.com/judahvang/edk2/tree/rpmc-update > > > Judah Vang (28): > MdeModulePkg: Add new GUID for Variable Store Info > SecurityPkg: Add new GUIDs for > MdeModulePkg: Update AUTH_VARIABLE_INFO struct > MdeModulePkg: Add reference to new Ppi Guid > MdeModulePkg: Add new ProtectedVariable GUIDs > MdeModulePkg: Add new include files > MdeModulePkg: Add Null ProtectedVariable Library > MdeModulePkg: Add new Variable functionality > MdeModulePkg: Add support for Protected Variables > SecurityPkg: Add new KeyService types and defines > SecurityPkg: Update RPMC APIs with index > SecurityPkg: Add new variable types and functions > SecurityPkg: Fix GetVariableKey API > SecurityPkg: Add null encryption variable libs > SecurityPkg: Add VariableKey library function > SecurityPkg: Add EncryptionVariable lib with AES > SecurityPkg: Add Protected Variable Services > MdeModulePkg: Reference Null ProtectedVariableLib > SecurityPkg: Add references to new *.inf files > ArmVirtPkg: Add reference to ProtectedVariableNull > UefiPayloadPkg: Add ProtectedVariable reference > EmulatorPkg: Add ProtectedVariable reference > OvmfPkg: Add ProtectedVariable reference > OvmfPkg: Add ProtectedVariableLib reference > OvmfPkg: Add ProtectedVariableLib reference > OvmfPkg: Add ProtectedVariableLib reference > OvmfPkg: Add ProtectedVariable reference > CryptoPkg: Enable cypto HMAC KDF library > > MdeModulePkg/MdeModulePkg.dec | 13 +- > SecurityPkg/SecurityPkg.dec | 43 +- > ArmVirtPkg/ArmVirtQemu.dsc | 3 +- > EmulatorPkg/EmulatorPkg.dsc | 3 +- > MdeModulePkg/MdeModulePkg.dsc | 4 +- > OvmfPkg/AmdSev/AmdSevX64.dsc | 3 +- > OvmfPkg/Bhyve/BhyveX64.dsc | 3 +- > OvmfPkg/CloudHv/CloudHvX64.dsc | 1 + > OvmfPkg/Microvm/MicrovmX64.dsc | 3 +- > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/OvmfXen.dsc | 3 +- > SecurityPkg/SecurityPkg.dsc | 13 +- > UefiPayloadPkg/UefiPayloadPkg.dsc | 2 + > CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 2 +- > MdeModulePkg/Library/ProtectedVariableLibNull/ProtectedVariableLibNull.inf | 34 + > MdeModulePkg/Universal/Variable/Pei/VariablePei.inf | 10 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 3 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 4 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 3 +- > SecurityPkg/Library/EncryptionVariableLib/EncryptionVariableLib.inf | 43 + > SecurityPkg/Library/EncryptionVariableLibNull/EncryptionVariableLibNull.inf | 38 + > SecurityPkg/Library/ProtectedVariableLib/DxeProtectedVariableLib.inf | 64 + > SecurityPkg/Library/ProtectedVariableLib/PeiProtectedVariableLib.inf | 68 + > SecurityPkg/Library/ProtectedVariableLib/SmmProtectedVariableLib.inf | 67 + > SecurityPkg/Library/ProtectedVariableLib/SmmRuntimeProtectedVariableLib.inf | 62 + > SecurityPkg/Library/VariableKeyLib/VariableKeyLib.inf | 36 + > MdeModulePkg/Include/Guid/ProtectedVariable.h | 22 + > MdeModulePkg/Include/Library/AuthVariableLib.h | 4 +- > MdeModulePkg/Include/Library/EncryptionVariableLib.h | 165 ++ > MdeModulePkg/Include/Library/ProtectedVariableLib.h | 700 +++++++ > MdeModulePkg/Universal/Variable/Pei/Variable.h | 80 +- > MdeModulePkg/Universal/Variable/Pei/VariableParsing.h | 309 +++ > MdeModulePkg/Universal/Variable/Pei/VariableStore.h | 116 ++ > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 126 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h | 91 +- > MdePkg/Include/Ppi/ReadOnlyVariable2.h | 4 +- > SecurityPkg/Include/Library/RpmcLib.h | 15 +- > SecurityPkg/Include/Library/VariableKeyLib.h | 37 +- > SecurityPkg/Include/Ppi/KeyServicePpi.h | 57 + > SecurityPkg/Library/EncryptionVariableLib/EncryptionVariable.h | 49 + > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableInternal.h | 611 ++++++ > MdeModulePkg/Library/ProtectedVariableLibNull/ProtectedVariable.c | 449 ++++ > MdeModulePkg/Universal/Variable/Pei/Variable.c | 886 ++------ > MdeModulePkg/Universal/Variable/Pei/VariableParsing.c | 941 +++++++++ > MdeModulePkg/Universal/Variable/Pei/VariableStore.c | 305 +++ > MdeModulePkg/Universal/Variable/RuntimeDxe/Reclaim.c | 349 +++- > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2139 +++++++++++--------- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 26 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c | 167 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c | 194 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 320 ++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c | 2 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 39 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 41 +- > SecurityPkg/Library/EncryptionVariableLib/EncryptionVariable.c | 728 +++++++ > SecurityPkg/Library/EncryptionVariableLibNull/EncryptionVariable.c | 107 + > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableCommon.c | 2095 +++++++++++++++++++ > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableDxe.c | 163 ++ > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariablePei.c | 1331 ++++++++++++ > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableSmm.c | 209 ++ > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableSmmDxeCommon.c | 975 +++++++++ > SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableSmmRuntime.c | 233 +++ > SecurityPkg/Library/RpmcLibNull/RpmcLibNull.c | 8 +- > SecurityPkg/Library/VariableKeyLib/VariableKeyLib.c | 59 + > SecurityPkg/Library/VariableKeyLibNull/VariableKeyLibNull.c | 6 +- > SecurityPkg/Library/EncryptionVariableLibNull/EncryptionVariableLibNull.uni | 16 + > 69 files changed, 12845 insertions(+), 1863 deletions(-) > create mode 100644 MdeModulePkg/Library/ProtectedVariableLibNull/ProtectedVariableLibNull.inf > create mode 100644 SecurityPkg/Library/EncryptionVariableLib/EncryptionVariableLib.inf > create mode 100644 SecurityPkg/Library/EncryptionVariableLibNull/EncryptionVariableLibNull.inf > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/DxeProtectedVariableLib.inf > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/PeiProtectedVariableLib.inf > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/SmmProtectedVariableLib.inf > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/SmmRuntimeProtectedVariableLib.inf > create mode 100644 SecurityPkg/Library/VariableKeyLib/VariableKeyLib.inf > create mode 100644 MdeModulePkg/Include/Guid/ProtectedVariable.h > create mode 100644 MdeModulePkg/Include/Library/EncryptionVariableLib.h > create mode 100644 MdeModulePkg/Include/Library/ProtectedVariableLib.h > create mode 100644 MdeModulePkg/Universal/Variable/Pei/VariableParsing.h > create mode 100644 MdeModulePkg/Universal/Variable/Pei/VariableStore.h > create mode 100644 SecurityPkg/Include/Ppi/KeyServicePpi.h > create mode 100644 SecurityPkg/Library/EncryptionVariableLib/EncryptionVariable.h > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableInternal.h > create mode 100644 MdeModulePkg/Library/ProtectedVariableLibNull/ProtectedVariable.c > create mode 100644 MdeModulePkg/Universal/Variable/Pei/VariableParsing.c > create mode 100644 MdeModulePkg/Universal/Variable/Pei/VariableStore.c > create mode 100644 SecurityPkg/Library/EncryptionVariableLib/EncryptionVariable.c > create mode 100644 SecurityPkg/Library/EncryptionVariableLibNull/EncryptionVariable.c > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableCommon.c > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableDxe.c > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariablePei.c > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableSmm.c > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableSmmDxeCommon.c > create mode 100644 SecurityPkg/Library/ProtectedVariableLib/ProtectedVariableSmmRuntime.c > create mode 100644 SecurityPkg/Library/VariableKeyLib/VariableKeyLib.c > create mode 100644 SecurityPkg/Library/EncryptionVariableLibNull/EncryptionVariableLibNull.uni >