Hi Michael and Bret, First of all, Great Work!
One thing I noticed is that this change will require 2 new LibraryClasses to be added to every platform .dsc file. However, these changes have not been made to OvmfPkg, EmulatorPkg, RaspberryPi, or any of the MinPlatform *OpenBoardPkgs in edk2-platforms. When one adds changes that require platform updates like this, it is generally expected that the change author provide those updates at the same time for the platforms that are in TianoCore project. For an example of this, see Pankaj's patch series from a month ago: https://edk2.groups.io/g/devel/message/54678 Also, looking at your code, I highly doubt you tested this with the Runtime DXE version of UEFI variable services. There are a non-zero number of platforms that use the Runtime DXE version of UEFI variable services, so this patch series will need to be updated and tested in Runtime DXE mode. Also, looking at your code, I highly doubt you tested with standalone MM either. We are trying to move from the legacy DXE+SMM to pure SMM/MM only drivers as much as possible as the mixing of DXE + SMM in a single driver has been a historical source of bugs/security vulnerabilities since you can only safely use DXE/UEFI services in the entry point. Also, there are some platforms now that only work with standalone MM, for example a lot of ARM designs have moved to using standalone MM + ARM trust zone to implement UEFI secure boot authenticated variables. I also have the following comments: PATCH #2: 1) VariablePolicyLib is defined as a BASE library, but I highly doubt this will actually work as a proper BASE library. For example, you have a lot of mutable global variables at the top of VariablePolicyLib.c, but nothing in the implementation I see handles the EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE EFI event so this code won't work with Runtime DXE. Runtime DXE support is required for this code since some platforms use the Runtime DXE version of UEFI variable services. PATCH #4: 1) Commented out code in VarCheckPolicyLibMmiHandler(): // VAR_CHECK_POLICY_COMM_DUMP_PARAMS *DumpParams; // Not yet implemented. Please don't check in commented out code. 2) VarCheckPolicyLib.inf - You define this LibraryClass as supporting the following module types: DXE_RUNTIME_DRIVER DXE_SMM_DRIVER. This is a problem because you do not support the standalone MM drivers, only the legacy DXE+SMM driver type. While most platforms work OK with the legacy DXE+SMM drivers, standalone MM is used by several platform designs, ARM trust zone for example. Please update. Thanks, Nate On 4/10/20, 11:36 AM, "devel@edk2.groups.io on behalf of Michael Kubacki" <devel@edk2.groups.io on behalf of michael.kuba...@outlook.com> wrote: From: Michael Kubacki <michael.kuba...@microsoft.com> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2522 The 9 patches in this series add the VariablePolicy feature to the core, deprecate Edk2VarLock (while adding a compatibility layer to reduce code churn), and integrate the VariablePolicy libraries and protocols into Variable Services. Since the integration requires multiple changes, including adding libraries, a protocol, an SMI communication handler, and VariableServices integration, the patches are broken up by individual library additions and then a final integration. Security-sensitive changes like bypassing Authenticated Variable enforcement are also broken out into individual patches so that attention can be called directly to them. The discussion of the feature can be found in multiple places throughout the last year on the RFC channel, staging branches, and in devel. Most recently, this subject was discussed in this thread: https://edk2.groups.io/g/devel/message/53712 (the code branches shared in that discussion are now out of date, but the whitepapers and discussion are relevant). On a separate note, shallow threading might not work on this patch series due to changes made by the SMTP server. Please bear with me while I am investigating if this can be changed. Cc: Jiewen Yao <jiewen....@intel.com> Cc: Chao Zhang <chao.b.zh...@intel.com> Cc: Jian J Wang <jian.j.w...@intel.com> Cc: Hao A Wu <hao.a...@intel.com> Cc: Liming Gao <liming....@intel.com> Signed-off-by: Bret Barkelew <brbar...@microsoft.com> Signed-off-by: Michael Kubacki <michael.kuba...@microsoft.com> Bret Barkelew (9): MdeModulePkg: Define the VariablePolicy protocol interface MdeModulePkg: Define the VariablePolicyLib MdeModulePkg: Define the VariablePolicyHelperLib MdeModulePkg: Define the VarCheckPolicyLib and SMM interface MdeModulePkg: Connect VariablePolicy business logic to VariableServices MdeModulePkg: Allow VariablePolicy state to delete protected variables SecurityPkg: Allow VariablePolicy state to delete authenticated variables MdeModulePkg: Change TCG MOR variables to use VariablePolicy MdeModulePkg: Drop VarLock from RuntimeDxe variable driver MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c | 211 ++ MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c | 396 ++++ MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c | 773 +++++++ MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c | 2285 ++++++++++++++++++++ MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 52 +- MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 60 +- MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c | 49 +- MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 51 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequstToLock.c | 71 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 445 ++++ MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 15 + SecurityPkg/Library/AuthVariableLib/AuthService.c | 22 +- MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h | 43 + MdeModulePkg/Include/Library/VariablePolicyHelperLib.h | 164 ++ MdeModulePkg/Include/Library/VariablePolicyLib.h | 206 ++ MdeModulePkg/Include/Protocol/VariablePolicy.h | 156 ++ MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf | 44 + MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni | 12 + MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf | 36 + MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.uni | 12 + MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf | 38 + MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni | 12 + MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf | 41 + MdeModulePkg/MdeModulePkg.dec | 17 +- MdeModulePkg/MdeModulePkg.dsc | 7 + MdeModulePkg/Test/MdeModulePkgHostTest.dsc | 8 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 5 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 4 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 8 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 4 + SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 2 + 31 files changed, 5172 insertions(+), 77 deletions(-) create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequstToLock.c create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c create mode 100644 MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h create mode 100644 MdeModulePkg/Include/Library/VariablePolicyHelperLib.h create mode 100644 MdeModulePkg/Include/Library/VariablePolicyLib.h create mode 100644 MdeModulePkg/Include/Protocol/VariablePolicy.h create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.uni create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf -- 2.16.3.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57286): https://edk2.groups.io/g/devel/message/57286 Mute This Topic: https://groups.io/mt/72928256/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
