[edk2-devel] [PATCH v1 0/9] Add the VariablePolicy feature
Nate DeSimone
nathaniel.l.desimone at intel.com
Tue Apr 14 00:41:24 UTC 2020
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 at edk2.groups.io on behalf of Michael Kubacki" <devel at edk2.groups.io on behalf of michael.kubacki at outlook.com> wrote:
From: Michael Kubacki <michael.kubacki at 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.yao at intel.com>
Cc: Chao Zhang <chao.b.zhang at intel.com>
Cc: Jian J Wang <jian.j.wang at intel.com>
Cc: Hao A Wu <hao.a.wu at intel.com>
Cc: Liming Gao <liming.gao at intel.com>
Signed-off-by: Bret Barkelew <brbarkel at microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki at 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/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