* [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
@ 2017-10-03 21:28 Laszlo Ersek
2017-10-03 21:28 ` [PATCH 1/6] MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new header Laszlo Ersek
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
Repo: https://github.com/lersek/edk2.git
Branch: mor_lock_init_at_end_of_dxe
This patch set fixes the issue reported in the following items:
* Inconsistent MOR control variables exposed by OVMF, breaks Windows
Device Guard
https://bugzilla.redhat.com/show_bug.cgi?id=1496170
* VariableSmm MorLockInit(): create MORLock only if / after MOR exists
https://bugzilla.tianocore.org/show_bug.cgi?id=727
Patches #1 through #3 are cleanups.
Patch #4 is a small helper patch for patch #5.
Patch #5 is the actual fix, following Jiewen's suggestions from the
edk2-devel thread
* [edk2] multiple levels of support for MOR / MORLock
https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
Debian versions) that create the MOR variable even if the platform
doesn't offer it up-front. This patch also follows Jiewen's suggestion
from the same edk2-devel thread.
(
BTW, at Paolo's recommendation, I've now reported this kernel issue for
Fedora, under
* incorrect downstream-only Platform Reset Attack Mitigation patch in
the F24-F26 kernels
https://bugzilla.redhat.com/show_bug.cgi?id=1498159
)
I've checked this set for basic regressions, using OVMF, normal boot and
S3 suspend/resume:
* Q35, SMM, IA32:
- Fedora 25 -- verified patch #6 specifically
* i440fx, no SMM, X64:
- Fedora 24
* Q35, SMM, IA32X64:
- Fedora 26 -- verified patch #6 specifically
- Windows 7
- Windows 8.1
- Windows 10
- Windows Server 2008 R2
- Windows Server 2012 R2
I didn't / couldn't test this set in the following two environments:
- on platforms where TcgMor.inf is included in the firmware, and the MOR
variable exists genuinely,
- in the nested virt setup where Ladi reported the Device Guard
breakage. (If I understand correctly, ATM this requires additional
host kernel (KVM) patches.)
Test results / feedback from those envs would be appreciated.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Thanks,
Laszlo
Laszlo Ersek (6):
MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
header
MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
header
MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
hook
MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
req
MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
EndOfDxe
MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
variable
MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 89 ++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 45 +++--
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 173 ++++++++++++++++++--
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 51 ------
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 4 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 16 +-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 1 +
12 files changed, 294 insertions(+), 94 deletions(-)
create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/6] MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new header
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
@ 2017-10-03 21:28 ` Laszlo Ersek
2017-10-03 21:28 ` [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header Laszlo Ersek
` (6 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
If the platform supports SMM, a gRT->SetVariable() call at boot time
results in the following call tree to SecureBootHook():
RuntimeServiceSetVariable() [VariableSmmRuntimeDxe.c, unprivileged]
SmmVariableHandler() [VariableSmm.c, PRIVILEGED]
VariableServiceSetVariable() [Variable.c, PRIVILEGED]
SecureBootHook() [VariableSmm.c, PRIVILEGED]
//
// do nothing
//
SecureBootHook() [Measurement.c, unprivileged]
//
// measure variable if it
// is related to SB policy
//
And if the platform does not support SMM:
VariableServiceSetVariable() [Variable.c, unprivileged]
SecureBootHook() [Measurement.c, unprivileged]
//
// measure variable if it
// is related to SB policy
//
In other words, the measurement always happens outside of SMM.
Because there are two implementations of the SecureBootHook() API, one
that is called from SMM and does nothing, and another that is called
outside of SMM and measures variables, the function declaration should be
in a header file. This way the compiler can enforce that the function
declaration and all function definitions match.
"Variable.h" is used for "including common header files, defining internal
structures and functions used by Variable modules". Technically, we could
declare SecureBootHook() in "Variable.h". However, "Measurement.c" and
"VariableSmmRuntimeDxe.c" themselves do not include "Variable.h", and that
is likely intentional -- "Variable.h" exposes so much of the privileged
variable implementation that it is likely excluded from these C source
files on purpose.
Therefore introduce a new header file called "PrivilegePolymorphic.h".
"Variable.h" includes this header (so that all C source files that have
been allowed to see the variable internals learn about the new
SecureBootHook() declaration immediately). In "Measurement.c" and
"VariableSmmRuntimeDxe.c", include *only* the new header.
This change cleans up commit fa0737a839d0 ("MdeModulePkg Variable: Merge
from Auth Variable driver in SecurityPkg", 2015-07-01).
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 38 ++++++++++++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 2 ++
MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 2 ++
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 14 --------
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 16 ++-------
8 files changed, 47 insertions(+), 28 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
index bc24a251c894..e840fc9bff40 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
@@ -41,6 +41,7 @@ [Sources]
Variable.c
VariableDxe.c
Variable.h
+ PrivilegePolymorphic.h
Measurement.c
TcgMorLockDxe.c
VarCheck.c
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
index ccfb6fc740c1..404164366579 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
@@ -51,6 +51,7 @@ [Sources]
VariableSmm.c
VarCheck.c
Variable.h
+ PrivilegePolymorphic.h
VariableExLib.c
TcgMorLockSmm.c
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
index 9975f5ae1d6e..bd73f7ac29f2 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
@@ -42,6 +42,7 @@ [Defines]
[Sources]
VariableSmmRuntimeDxe.c
+ PrivilegePolymorphic.h
Measurement.c
[Packages]
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
new file mode 100644
index 000000000000..0aa0d4f48f10
--- /dev/null
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
@@ -0,0 +1,38 @@
+/** @file
+ Polymorphic functions that are called from both the privileged driver (i.e.,
+ the DXE_SMM variable module) and the non-privileged drivers (i.e., one or
+ both of the DXE_RUNTIME variable modules).
+
+ Each of these functions has two implementations, appropriate for privileged
+ vs. non-privileged driver code.
+
+ Copyright (c) 2017, Red Hat, Inc.<BR>
+ Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+#ifndef _PRIVILEGE_POLYMORPHIC_H_
+#define _PRIVILEGE_POLYMORPHIC_H_
+
+#include <Uefi/UefiBaseType.h>
+
+/**
+ SecureBoot Hook for auth variable update.
+
+ @param[in] VariableName Name of Variable to be found.
+ @param[in] VendorGuid Variable vendor GUID.
+**/
+VOID
+EFIAPI
+SecureBootHook (
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
+ );
+
+#endif
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
index 8b1b1332b3da..ec9b9849ec09 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
@@ -44,6 +44,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Guid/FaultTolerantWrite.h>
#include <Guid/VarErrorFlag.h>
+#include "PrivilegePolymorphic.h"
+
#define EFI_VARIABLE_ATTRIBUTES_MASK (EFI_VARIABLE_NON_VOLATILE | \
EFI_VARIABLE_BOOTSERVICE_ACCESS | \
EFI_VARIABLE_RUNTIME_ACCESS | \
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
index a8ed51495e2a..6acc167224ba 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
@@ -24,6 +24,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/BaseLib.h>
#include <Library/TpmMeasurementLib.h>
+#include "PrivilegePolymorphic.h"
+
typedef struct {
CHAR16 *VariableName;
EFI_GUID *VendorGuid;
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 71a6fd209364..28e4ac8f3819 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -97,20 +97,6 @@ AUTH_VAR_LIB_CONTEXT_IN mAuthContextIn = {
AUTH_VAR_LIB_CONTEXT_OUT mAuthContextOut;
-/**
-
- SecureBoot Hook for auth variable update.
-
- @param[in] VariableName Name of Variable to be found.
- @param[in] VendorGuid Variable vendor GUID.
-**/
-VOID
-EFIAPI
-SecureBootHook (
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid
- );
-
/**
Initialization for MOR Lock Control.
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
index e209d54755ef..85d655dc19ff 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -44,6 +44,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Guid/EventGroup.h>
#include <Guid/SmmVariableCommon.h>
+#include "PrivilegePolymorphic.h"
+
EFI_HANDLE mHandle = NULL;
EFI_SMM_VARIABLE_PROTOCOL *mSmmVariable = NULL;
EFI_EVENT mVirtualAddressChangeEvent = NULL;
@@ -56,20 +58,6 @@ EFI_LOCK mVariableServicesLock;
EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock;
EDKII_VAR_CHECK_PROTOCOL mVarCheck;
-/**
- SecureBoot Hook for SetVariable.
-
- @param[in] VariableName Name of Variable to be found.
- @param[in] VendorGuid Variable vendor GUID.
-
-**/
-VOID
-EFIAPI
-SecureBootHook (
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid
- );
-
/**
Some Secure Boot Policy Variable may update following other variable changes(SecureBoot follows PK change, etc).
Record their initial State when variable write service is ready.
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
2017-10-03 21:28 ` [PATCH 1/6] MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new header Laszlo Ersek
@ 2017-10-03 21:28 ` Laszlo Ersek
2017-10-09 6:55 ` Zeng, Star
2017-10-03 21:28 ` [PATCH 3/6] MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe() hook Laszlo Ersek
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
The MorLockInit() and SetVariableCheckHandlerMor() functions have separate
implementations for VariableRuntimeDxe (= unprivileged, unified
DXE_RUNTIME driver) and VariableSmm (= privileged, DXE_SMM back-end of the
split variable driver).
Move their declarations from "Variable.c" to "PrivilegePolymorphic.h", so
that the compiler enforce that the declarations and the definitions match.
(All C source files with the call sites and the function definitions
already include "PrivilegePolymorphic.h" via "Variable.h".)
At the same time:
- replace two typos in the MorLockInit() description:
- replace "EFI_SUCEESS" with "EFI_SUCCESS",
- replace "MOR Lock Control" with "MOR Control Lock";
- in the SetVariableCheckHandlerMor() description:
- replace @param with @param[in],
- rewrap the comment to 80 columns.
This change cleans up commit 2f6aa774fe38 ("MdeModulePkg: Add MorLock to
variable driver.", 2016-01-19).
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 41 ++++++++++++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 30 +++++++-------
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 30 +++++++-------
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 37 ------------------
4 files changed, 75 insertions(+), 63 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
index 0aa0d4f48f10..1118f4b52e49 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
@@ -35,4 +35,45 @@ SecureBootHook (
IN EFI_GUID *VendorGuid
);
+/**
+ Initialization for MOR Control Lock.
+
+ @retval EFI_SUCCESS MorLock initialization success.
+ @return Others Some error occurs.
+**/
+EFI_STATUS
+MorLockInit (
+ VOID
+ );
+
+/**
+ This service is an MOR/MorLock checker handler for the SetVariable().
+
+ @param[in] VariableName the name of the vendor's variable, as a
+ Null-Terminated Unicode String
+ @param[in] VendorGuid Unify identifier for vendor.
+ @param[in] Attributes Point to memory location to return the attributes of
+ variable. If the point is NULL, the parameter would
+ be ignored.
+ @param[in] DataSize The size in bytes of Data-Buffer.
+ @param[in] Data Point to the content of the variable.
+
+ @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
+ driver can store the variable data.
+ @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
+ attributes is not allowed for MOR variable.
+ @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
+ @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
+ function. Variable driver can just return
+ EFI_SUCCESS.
+**/
+EFI_STATUS
+SetVariableCheckHandlerMor (
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid,
+ IN UINT32 Attributes,
+ IN UINTN DataSize,
+ IN VOID *Data
+ );
+
#endif
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
index c32eb3b1ac4b..ab3e5d416cd4 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
@@ -28,19 +28,23 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock;
/**
This service is an MOR/MorLock checker handler for the SetVariable().
- @param VariableName the name of the vendor's variable, as a
- Null-Terminated Unicode String
- @param VendorGuid Unify identifier for vendor.
- @param Attributes Point to memory location to return the attributes of variable. If the point
- is NULL, the parameter would be ignored.
- @param DataSize The size in bytes of Data-Buffer.
- @param Data Point to the content of the variable.
+ @param[in] VariableName the name of the vendor's variable, as a
+ Null-Terminated Unicode String
+ @param[in] VendorGuid Unify identifier for vendor.
+ @param[in] Attributes Point to memory location to return the attributes of
+ variable. If the point is NULL, the parameter would
+ be ignored.
+ @param[in] DataSize The size in bytes of Data-Buffer.
+ @param[in] Data Point to the content of the variable.
- @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
- @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
+ @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
+ driver can store the variable data.
+ @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
+ attributes is not allowed for MOR variable.
@retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
- @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
- Variable driver can just return EFI_SUCCESS.
+ @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
+ function. Variable driver can just return
+ EFI_SUCCESS.
**/
EFI_STATUS
SetVariableCheckHandlerMor (
@@ -58,9 +62,9 @@ SetVariableCheckHandlerMor (
}
/**
- Initialization for MOR Lock Control.
+ Initialization for MOR Control Lock.
- @retval EFI_SUCEESS MorLock initialization success.
+ @retval EFI_SUCCESS MorLock initialization success.
@return Others Some error occurs.
**/
EFI_STATUS
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index d06317ca9cf4..390c8fde4bd4 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -309,19 +309,23 @@ SetVariableCheckHandlerMorLock (
/**
This service is an MOR/MorLock checker handler for the SetVariable().
- @param VariableName the name of the vendor's variable, as a
- Null-Terminated Unicode String
- @param VendorGuid Unify identifier for vendor.
- @param Attributes Point to memory location to return the attributes of variable. If the point
- is NULL, the parameter would be ignored.
- @param DataSize The size in bytes of Data-Buffer.
- @param Data Point to the content of the variable.
+ @param[in] VariableName the name of the vendor's variable, as a
+ Null-Terminated Unicode String
+ @param[in] VendorGuid Unify identifier for vendor.
+ @param[in] Attributes Point to memory location to return the attributes of
+ variable. If the point is NULL, the parameter would
+ be ignored.
+ @param[in] DataSize The size in bytes of Data-Buffer.
+ @param[in] Data Point to the content of the variable.
- @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
- @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
+ @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
+ driver can store the variable data.
+ @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
+ attributes is not allowed for MOR variable.
@retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
- @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
- Variable driver can just return EFI_SUCCESS.
+ @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
+ function. Variable driver can just return
+ EFI_SUCCESS.
**/
EFI_STATUS
SetVariableCheckHandlerMor (
@@ -377,9 +381,9 @@ SetVariableCheckHandlerMor (
}
/**
- Initialization for MOR Lock Control.
+ Initialization for MOR Control Lock.
- @retval EFI_SUCEESS MorLock initialization success.
+ @retval EFI_SUCCESS MorLock initialization success.
@return Others Some error occurs.
**/
EFI_STATUS
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 28e4ac8f3819..d68dfbe648ce 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -97,43 +97,6 @@ AUTH_VAR_LIB_CONTEXT_IN mAuthContextIn = {
AUTH_VAR_LIB_CONTEXT_OUT mAuthContextOut;
-/**
- Initialization for MOR Lock Control.
-
- @retval EFI_SUCEESS MorLock initialization success.
- @return Others Some error occurs.
-**/
-EFI_STATUS
-MorLockInit (
- VOID
- );
-
-/**
- This service is an MOR/MorLock checker handler for the SetVariable().
-
- @param VariableName the name of the vendor's variable, as a
- Null-Terminated Unicode String
- @param VendorGuid Unify identifier for vendor.
- @param Attributes Point to memory location to return the attributes of variable. If the point
- is NULL, the parameter would be ignored.
- @param DataSize The size in bytes of Data-Buffer.
- @param Data Point to the content of the variable.
-
- @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
- @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
- @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
- @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
- Variable driver can just return EFI_SUCCESS.
-**/
-EFI_STATUS
-SetVariableCheckHandlerMor (
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid,
- IN UINT32 Attributes,
- IN UINTN DataSize,
- IN VOID *Data
- );
-
/**
Routine used to track statistical information about variable usage.
The data is stored in the EFI system table so it can be accessed later.
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe() hook
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
2017-10-03 21:28 ` [PATCH 1/6] MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new header Laszlo Ersek
2017-10-03 21:28 ` [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header Laszlo Ersek
@ 2017-10-03 21:28 ` Laszlo Ersek
2017-10-03 21:28 ` [PATCH 4/6] MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru req Laszlo Ersek
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
Introduce the MorLockInitAtEndOfDxe() hook, in order to allow
MorLockInit() to delay / queue operations until EndOfDxe. (Or, if the
platform never signals EndOfDxe, until ReadyToBoot.)
Call MorLockInitAtEndOfDxe() whenever we set "mEndOfDxe" to TRUE:
- in VariableRuntimeDxe:
- in the OnReadyToBoot() function,
- in the OnEndOfDxe() function;
- in VariableSmm:
- on the SMM_VARIABLE_FUNCTION_READY_TO_BOOT SMI request,
- in the SmmEndOfDxeCallback() function.
For now, implement MorLockInitAtEndOfDxe() as a no-op in both
VariableRuntimeDxe and VariableSmm.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 10 ++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 15 +++++++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 15 +++++++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 ++
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 ++
5 files changed, 44 insertions(+)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
index 1118f4b52e49..759e47db7f29 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
@@ -46,6 +46,16 @@ MorLockInit (
VOID
);
+/**
+ Delayed initialization for MOR Control Lock at EndOfDxe.
+
+ This function performs any operations queued by MorLockInit().
+**/
+VOID
+MorLockInitAtEndOfDxe (
+ VOID
+ );
+
/**
This service is an MOR/MorLock checker handler for the SetVariable().
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
index ab3e5d416cd4..a91fc42ff465 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
@@ -91,3 +91,18 @@ MorLockInit (
VariableLockRequestToLock (&mVariableLock, MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid);
return EFI_SUCCESS;
}
+
+/**
+ Delayed initialization for MOR Control Lock at EndOfDxe.
+
+ This function performs any operations queued by MorLockInit().
+**/
+VOID
+MorLockInitAtEndOfDxe (
+ VOID
+ )
+{
+ //
+ // Do nothing.
+ //
+}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index 390c8fde4bd4..24b7ffb84ce7 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -396,3 +396,18 @@ MorLockInit (
//
return SetMorLockVariable (0);
}
+
+/**
+ Delayed initialization for MOR Control Lock at EndOfDxe.
+
+ This function performs any operations queued by MorLockInit().
+**/
+VOID
+MorLockInitAtEndOfDxe (
+ VOID
+ )
+{
+ //
+ // Do nothing.
+ //
+}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
index fe1b2b588cb9..b2a373cf98aa 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
@@ -291,6 +291,7 @@ OnReadyToBoot (
)
{
if (!mEndOfDxe) {
+ MorLockInitAtEndOfDxe ();
//
// Set the End Of DXE bit in case the EFI_END_OF_DXE_EVENT_GROUP_GUID event is not signaled.
//
@@ -330,6 +331,7 @@ OnEndOfDxe (
)
{
DEBUG ((EFI_D_INFO, "[Variable]END_OF_DXE is signaled\n"));
+ MorLockInitAtEndOfDxe ();
mEndOfDxe = TRUE;
mVarCheckAddressPointer = VarCheckLibInitializeAtEndOfDxe (&mVarCheckAddressPointerCount);
//
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index 2184634f3544..8d73b6edee51 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -679,6 +679,7 @@ SmmVariableHandler (
break;
}
if (!mEndOfDxe) {
+ MorLockInitAtEndOfDxe ();
mEndOfDxe = TRUE;
VarCheckLibInitializeAtEndOfDxe (NULL);
//
@@ -811,6 +812,7 @@ SmmEndOfDxeCallback (
)
{
DEBUG ((EFI_D_INFO, "[Variable]SMM_END_OF_DXE is signaled\n"));
+ MorLockInitAtEndOfDxe ();
mEndOfDxe = TRUE;
VarCheckLibInitializeAtEndOfDxe (NULL);
//
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru req
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
` (2 preceding siblings ...)
2017-10-03 21:28 ` [PATCH 3/6] MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe() hook Laszlo Ersek
@ 2017-10-03 21:28 ` Laszlo Ersek
2017-10-03 21:28 ` [PATCH 5/6] MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe Laszlo Ersek
` (3 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
The SetMorLockVariable() function sets "mMorLockPassThru" to TRUE
temporarily, so that it can set the MOR Control Lock variable to
well-formed values without permission checks.
In the next patch, we'll need the same override for deleting the MOR
Control Lock variable; hence obey "mMorLockPassThru" in the deletion
branch of SetVariableCheckHandlerMorLock() as well.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index 24b7ffb84ce7..1f495f847212 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -169,7 +169,10 @@ SetVariableCheckHandlerMorLock (
// Basic Check
//
if (Attributes == 0 || DataSize == 0 || Data == NULL) {
- return EFI_WRITE_PROTECTED;
+ //
+ // Permit deletion for passthru request, deny it otherwise.
+ //
+ return mMorLockPassThru ? EFI_SUCCESS : EFI_WRITE_PROTECTED;
}
if ((Attributes != (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS)) ||
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
` (3 preceding siblings ...)
2017-10-03 21:28 ` [PATCH 4/6] MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru req Laszlo Ersek
@ 2017-10-03 21:28 ` Laszlo Ersek
2017-10-03 21:28 ` [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable Laszlo Ersek
` (2 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
The "MemoryOverwriteRequestControl" (a.k.a. MOR) variable comes from the
"TCG Platform Reset Attack Mitigation Specification":
https://www.trustedcomputinggroup.org/wp-content/uploads/Platform-Reset-Attack-Mitigation-Specification.pdf
The "MemoryOverwriteRequestControlLock" variable (a.k.a. MORL) is a
Microsoft extension, called "Secure MOR implementation":
https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/device-guard-requirements
Currently the VariableSmm driver creates MORL without regard to MOR. This
can lead to a situation where a platform does not support MOR from the
prerequisite spec (because it does not include the
"SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf" driver), but appears
to support MORL from the dependent Microsoft spec.
"winload.efi" notices this inconsistency, and disables the Device Guard
Virtualization Based Security in Windows Server 2016 and Windows 10 64-bit
Enterprise.
If the platform includes
"SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf", then MOR will exist
by the time EndOfDxe is reached, and VariableSmm can safely create MORL.
Otherwise, do not create MORL (delete it if it exists), and also prevent
other modules from creating it.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=727
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1496170
Reported-by: Ladi Prosek <lprosek@redhat.com>
Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 62 ++++++++++++++++++--
1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index 1f495f847212..6d14b0042f4d 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -45,6 +45,7 @@ typedef enum {
MorLockStateLocked = 1,
} MOR_LOCK_STATE;
+BOOLEAN mMorLockInitializationRequired = FALSE;
UINT8 mMorLockKey[MOR_LOCK_V2_KEY_SIZE];
BOOLEAN mMorLockKeyEmpty = TRUE;
BOOLEAN mMorLockPassThru = FALSE;
@@ -394,10 +395,8 @@ MorLockInit (
VOID
)
{
- //
- // Set variable to report capability to OS
- //
- return SetMorLockVariable (0);
+ mMorLockInitializationRequired = TRUE;
+ return EFI_SUCCESS;
}
/**
@@ -410,7 +409,60 @@ MorLockInitAtEndOfDxe (
VOID
)
{
+ UINTN MorSize;
+ EFI_STATUS MorStatus;
+
+ if (!mMorLockInitializationRequired) {
+ //
+ // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
+ // the variable write service is unavailable. Do nothing.
+ //
+ return;
+ }
+
//
- // Do nothing.
+ // Check if the MOR variable exists.
//
+ MorSize = 0;
+ MorStatus = VariableServiceGetVariable (
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid,
+ NULL, // Attributes
+ &MorSize,
+ NULL // Data
+ );
+ //
+ // We provided a zero-sized buffer, so the above call can never succeed.
+ //
+ ASSERT (EFI_ERROR (MorStatus));
+
+ if (MorStatus == EFI_BUFFER_TOO_SMALL) {
+ //
+ // The MOR variable exists; set the MOR Control Lock variable to report the
+ // capability to the OS.
+ //
+ SetMorLockVariable (0);
+ return;
+ }
+
+ //
+ // The platform does not support the MOR variable. Delete the MOR Control
+ // Lock variable (should it exists for some reason) and prevent other modules
+ // from creating it.
+ //
+ mMorLockPassThru = TRUE;
+ VariableServiceSetVariable (
+ MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME,
+ &gEfiMemoryOverwriteRequestControlLockGuid,
+ 0, // Attributes
+ 0, // DataSize
+ NULL // Data
+ );
+ mMorLockPassThru = FALSE;
+
+ VariableLockRequestToLock (
+ NULL, // This
+ MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME,
+ &gEfiMemoryOverwriteRequestControlLockGuid
+ );
}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
` (4 preceding siblings ...)
2017-10-03 21:28 ` [PATCH 5/6] MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe Laszlo Ersek
@ 2017-10-03 21:28 ` Laszlo Ersek
2017-10-09 7:12 ` Zeng, Star
2017-10-04 1:18 ` [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Yao, Jiewen
2017-10-05 7:42 ` Zeng, Star
7 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-03 21:28 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Ladi Prosek, Star Zeng
According to the TCG Platform Reset Attack Mitigation Specification (May
15, 2008):
> 5 Interface for UEFI
> 5.1 UEFI Variable
> 5.1.1 The MemoryOverwriteRequestControl
>
> Start of informative comment:
>
> [...] The OS loader should not create the variable. Rather, the firmware
> is required to create it and must support the semantics described here.
>
> End of informative comment.
However, some OS kernels create the MOR variable even if the platform
firmware does not support it (see one Bugzilla reference below). This OS
issue breaks the logic added in the last patch.
Strengthen the MOR check by searching for the TCG or TCG2 protocols, as
edk2's implementation of MOR depends on (one of) those protocols.
The protocols are defined under MdePkg, thus there's no inter-package
dependency issue. In addition, calling UEFI services in
MorLockInitAtEndOfDxe() is safe, due to the following order of events /
actions:
- platform BDS signals the EndOfDxe event group,
- the SMM core installs the SmmEndOfDxe protocol,
- MorLockInitAtEndOfDxe() is invoked, and it calls UEFI services,
- some time later, platform BDS installs the DxeSmmReadyToLock protocol,
- SMM / SMRAM is locked down and UEFI services become unavailable to SMM
drivers.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1498159
Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 +
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 81 ++++++++++++++++++--
2 files changed, 77 insertions(+), 7 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
index 404164366579..69966f0d37ee 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
@@ -74,6 +74,7 @@ [LibraryClasses]
SmmMemLib
AuthVariableLib
VarCheckLib
+ UefiBootServicesTableLib
[Protocols]
gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES
@@ -85,6 +86,8 @@ [Protocols]
gEfiSmmVariableProtocolGuid
gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
gEdkiiSmmVarCheckProtocolGuid ## PRODUCES
+ gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
+ gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
[Guids]
## SOMETIMES_CONSUMES ## GUID # Signature of Variable store header
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index 6d14b0042f4d..0a0281e44bc1 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/DebugLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/UefiBootServicesTableLib.h>
#include "Variable.h"
typedef struct {
@@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] = {
{MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid},
};
+BOOLEAN mMorPassThru = FALSE;
+
#define MOR_LOCK_DATA_UNLOCKED 0x0
#define MOR_LOCK_DATA_LOCKED_WITHOUT_KEY 0x1
#define MOR_LOCK_DATA_LOCKED_WITH_KEY 0x2
@@ -364,6 +367,13 @@ SetVariableCheckHandlerMor (
// Mor Variable
//
+ //
+ // Permit deletion for passthru request.
+ //
+ if (((Attributes == 0) || (DataSize == 0)) && mMorPassThru) {
+ return EFI_SUCCESS;
+ }
+
//
// Basic Check
//
@@ -411,6 +421,8 @@ MorLockInitAtEndOfDxe (
{
UINTN MorSize;
EFI_STATUS MorStatus;
+ EFI_STATUS TcgStatus;
+ VOID *TcgInterface;
if (!mMorLockInitializationRequired) {
//
@@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe (
if (MorStatus == EFI_BUFFER_TOO_SMALL) {
//
- // The MOR variable exists; set the MOR Control Lock variable to report the
- // capability to the OS.
+ // The MOR variable exists.
//
- SetMorLockVariable (0);
- return;
+ // Some OSes don't follow the TCG's Platform Reset Attack Mitigation spec
+ // in that the OS should never create the MOR variable, only read and write
+ // it -- these OSes (unintentionally) create MOR if the platform firmware
+ // does not produce it. Whether this is the case (from the last OS boot)
+ // can be deduced from the absence of the TCG / TCG2 protocols, as edk2's
+ // MOR implementation depends on (one of) those protocols.
+ //
+ TcgStatus = gBS->LocateProtocol (
+ &gEfiTcgProtocolGuid,
+ NULL, // Registration
+ &TcgInterface
+ );
+ if (EFI_ERROR (TcgStatus)) {
+ TcgStatus = gBS->LocateProtocol (
+ &gEfiTcg2ProtocolGuid,
+ NULL, // Registration
+ &TcgInterface
+ );
+ }
+
+ if (!EFI_ERROR (TcgStatus)) {
+ //
+ // The MOR variable originates from the platform firmware; set the MOR
+ // Control Lock variable to report the locking capability to the OS.
+ //
+ SetMorLockVariable (0);
+ return;
+ }
+
+ //
+ // The MOR variable's origin is inexplicable; delete it.
+ //
+ DEBUG ((
+ DEBUG_WARN,
+ "%a: deleting unexpected / unsupported variable %g:%s\n",
+ __FUNCTION__,
+ &gEfiMemoryOverwriteControlDataGuid,
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME
+ ));
+
+ mMorPassThru = TRUE;
+ VariableServiceSetVariable (
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid,
+ 0, // Attributes
+ 0, // DataSize
+ NULL // Data
+ );
+ mMorPassThru = FALSE;
}
//
- // The platform does not support the MOR variable. Delete the MOR Control
- // Lock variable (should it exists for some reason) and prevent other modules
- // from creating it.
+ // The MOR variable is absent; the platform firmware does not support it.
+ // Lock the variable so that no other module may create it.
+ //
+ VariableLockRequestToLock (
+ NULL, // This
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid
+ );
+
+ //
+ // Delete the MOR Control Lock variable too (should it exists for some
+ // reason) and prevent other modules from creating it.
//
mMorLockPassThru = TRUE;
VariableServiceSetVariable (
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
` (5 preceding siblings ...)
2017-10-03 21:28 ` [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable Laszlo Ersek
@ 2017-10-04 1:18 ` Yao, Jiewen
2017-10-04 10:39 ` Laszlo Ersek
2017-10-05 7:42 ` Zeng, Star
7 siblings, 1 reply; 22+ messages in thread
From: Yao, Jiewen @ 2017-10-04 1:18 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Ladi Prosek, Zeng, Star
Thanks. Laszlo.
This patch looks good to me in general.
One minor comment is below:
MorLockInitAtEndOfDxe()
+ if (!mMorLockInitializationRequired) {
+ //
+ // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
+ // the variable write service is unavailable. Do nothing.
+ //
+ return;
+ }
I think it is an illegal case. I would add ASSERT before return.
And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)
Thank you
Yao Jiewen
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:28 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi
> Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
> inconsistency
>
> Repo: https://github.com/lersek/edk2.git
> Branch: mor_lock_init_at_end_of_dxe
>
> This patch set fixes the issue reported in the following items:
>
> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
> Device Guard
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>
> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=727
>
> Patches #1 through #3 are cleanups.
>
> Patch #4 is a small helper patch for patch #5.
>
> Patch #5 is the actual fix, following Jiewen's suggestions from the
> edk2-devel thread
>
> * [edk2] multiple levels of support for MOR / MORLock
>
> https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
> https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>
> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
> Debian versions) that create the MOR variable even if the platform
> doesn't offer it up-front. This patch also follows Jiewen's suggestion
> from the same edk2-devel thread.
>
> (
>
> BTW, at Paolo's recommendation, I've now reported this kernel issue for
> Fedora, under
>
> * incorrect downstream-only Platform Reset Attack Mitigation patch in
> the F24-F26 kernels
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>
> )
>
> I've checked this set for basic regressions, using OVMF, normal boot and
> S3 suspend/resume:
>
> * Q35, SMM, IA32:
> - Fedora 25 -- verified patch #6 specifically
>
> * i440fx, no SMM, X64:
> - Fedora 24
>
> * Q35, SMM, IA32X64:
> - Fedora 26 -- verified patch #6 specifically
> - Windows 7
> - Windows 8.1
> - Windows 10
> - Windows Server 2008 R2
> - Windows Server 2012 R2
>
> I didn't / couldn't test this set in the following two environments:
>
> - on platforms where TcgMor.inf is included in the firmware, and the MOR
> variable exists genuinely,
>
> - in the nested virt setup where Ladi reported the Device Guard
> breakage. (If I understand correctly, ATM this requires additional
> host kernel (KVM) patches.)
>
> Test results / feedback from those envs would be appreciated.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (6):
> MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
> header
> MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
> header
> MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
> hook
> MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
> req
> MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
> EndOfDxe
> MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
> variable
>
> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> | 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h |
> 89 ++++++++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> | 45 +++--
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> | 173 ++++++++++++++++++--
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c |
> 51 ------
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h |
> 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c |
> 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf |
> 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> | 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> | 4 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> | 16 +-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
> | 1 +
> 12 files changed, 294 insertions(+), 94 deletions(-)
> create mode 100644
> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>
> --
> 2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-04 1:18 ` [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Yao, Jiewen
@ 2017-10-04 10:39 ` Laszlo Ersek
2017-10-04 12:24 ` Ladi Prosek
2017-10-10 4:17 ` Yao, Jiewen
0 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-04 10:39 UTC (permalink / raw)
To: Yao, Jiewen, edk2-devel-01; +Cc: Dong, Eric, Ladi Prosek, Zeng, Star
On 10/04/17 03:18, Yao, Jiewen wrote:
> Thanks. Laszlo.
> This patch looks good to me in general.
>
> One minor comment is below:
> MorLockInitAtEndOfDxe()
> + if (!mMorLockInitializationRequired) {
> + //
> + // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> + // the variable write service is unavailable. Do nothing.
> + //
> + return;
> + }
> I think it is an illegal case. I would add ASSERT before return.
OK, I will replace the sentence "Do nothing." with "This should never
happen.", and also add ASSERT (FALSE) above the "return".
> And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)
Right!
Ladi stated in the RHBZ that he had built OVMF; I'll talk with him
off-list regardless, to see if I can help with anything.
Thank you Jiewen!
Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, October 4, 2017 5:28 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi
>> Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
>> inconsistency
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: mor_lock_init_at_end_of_dxe
>>
>> This patch set fixes the issue reported in the following items:
>>
>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>> Device Guard
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>
>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>
>> Patches #1 through #3 are cleanups.
>>
>> Patch #4 is a small helper patch for patch #5.
>>
>> Patch #5 is the actual fix, following Jiewen's suggestions from the
>> edk2-devel thread
>>
>> * [edk2] multiple levels of support for MOR / MORLock
>>
>> https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>> https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>
>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
>> Debian versions) that create the MOR variable even if the platform
>> doesn't offer it up-front. This patch also follows Jiewen's suggestion
>> from the same edk2-devel thread.
>>
>> (
>>
>> BTW, at Paolo's recommendation, I've now reported this kernel issue for
>> Fedora, under
>>
>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>> the F24-F26 kernels
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>
>> )
>>
>> I've checked this set for basic regressions, using OVMF, normal boot and
>> S3 suspend/resume:
>>
>> * Q35, SMM, IA32:
>> - Fedora 25 -- verified patch #6 specifically
>>
>> * i440fx, no SMM, X64:
>> - Fedora 24
>>
>> * Q35, SMM, IA32X64:
>> - Fedora 26 -- verified patch #6 specifically
>> - Windows 7
>> - Windows 8.1
>> - Windows 10
>> - Windows Server 2008 R2
>> - Windows Server 2012 R2
>>
>> I didn't / couldn't test this set in the following two environments:
>>
>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>> variable exists genuinely,
>>
>> - in the nested virt setup where Ladi reported the Device Guard
>> breakage. (If I understand correctly, ATM this requires additional
>> host kernel (KVM) patches.)
>>
>> Test results / feedback from those envs would be appreciated.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ladi Prosek <lprosek@redhat.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>> MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>> header
>> MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>> header
>> MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>> hook
>> MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>> req
>> MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>> EndOfDxe
>> MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>> variable
>>
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
>> | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h |
>> 89 ++++++++++
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>> | 45 +++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>> | 173 ++++++++++++++++++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c |
>> 51 ------
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h |
>> 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c |
>> 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf |
>> 1 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>> | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>> | 4 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
>> | 16 +-
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>> | 1 +
>> 12 files changed, 294 insertions(+), 94 deletions(-)
>> create mode 100644
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-04 10:39 ` Laszlo Ersek
@ 2017-10-04 12:24 ` Ladi Prosek
2017-10-10 4:17 ` Yao, Jiewen
1 sibling, 0 replies; 22+ messages in thread
From: Ladi Prosek @ 2017-10-04 12:24 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Yao, Jiewen, edk2-devel-01, Dong, Eric, Zeng, Star
On Wed, Oct 4, 2017 at 12:39 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/04/17 03:18, Yao, Jiewen wrote:
>> Thanks. Laszlo.
>> This patch looks good to me in general.
>>
>> One minor comment is below:
>> MorLockInitAtEndOfDxe()
>> + if (!mMorLockInitializationRequired) {
>> + //
>> + // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
>> + // the variable write service is unavailable. Do nothing.
>> + //
>> + return;
>> + }
>> I think it is an illegal case. I would add ASSERT before return.
>
> OK, I will replace the sentence "Do nothing." with "This should never
> happen.", and also add ASSERT (FALSE) above the "return".
>
>> And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)
>
> Right!
I have tested the patch and can confirm that it fixes the Windows
device guard problem.
Tested-by: Ladi Prosek <lprosek@redhat.com>
> Ladi stated in the RHBZ that he had built OVMF; I'll talk with him
> off-list regardless, to see if I can help with anything.
This was my first time building OVMF on my machine (the builds I had
done before filing the RHBZ were all using Red Hat's internal build
service). The only thing that threw me off was -D SMM_REQUIRE which I
was initially missing so I couldn't reproduce the issue on baseline.
> Thank you Jiewen!
> Laszlo
Thank you Laszlo and Jiewen for the very quick turnaround on this!
Ladi
>
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Wednesday, October 4, 2017 5:28 AM
>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi
>>> Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
>>> inconsistency
>>>
>>> Repo: https://github.com/lersek/edk2.git
>>> Branch: mor_lock_init_at_end_of_dxe
>>>
>>> This patch set fixes the issue reported in the following items:
>>>
>>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>>> Device Guard
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>>
>>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>>
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>>
>>> Patches #1 through #3 are cleanups.
>>>
>>> Patch #4 is a small helper patch for patch #5.
>>>
>>> Patch #5 is the actual fix, following Jiewen's suggestions from the
>>> edk2-devel thread
>>>
>>> * [edk2] multiple levels of support for MOR / MORLock
>>>
>>> https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>>> https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>>
>>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
>>> Debian versions) that create the MOR variable even if the platform
>>> doesn't offer it up-front. This patch also follows Jiewen's suggestion
>>> from the same edk2-devel thread.
>>>
>>> (
>>>
>>> BTW, at Paolo's recommendation, I've now reported this kernel issue for
>>> Fedora, under
>>>
>>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>>> the F24-F26 kernels
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>>
>>> )
>>>
>>> I've checked this set for basic regressions, using OVMF, normal boot and
>>> S3 suspend/resume:
>>>
>>> * Q35, SMM, IA32:
>>> - Fedora 25 -- verified patch #6 specifically
>>>
>>> * i440fx, no SMM, X64:
>>> - Fedora 24
>>>
>>> * Q35, SMM, IA32X64:
>>> - Fedora 26 -- verified patch #6 specifically
>>> - Windows 7
>>> - Windows 8.1
>>> - Windows 10
>>> - Windows Server 2008 R2
>>> - Windows Server 2012 R2
>>>
>>> I didn't / couldn't test this set in the following two environments:
>>>
>>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>>> variable exists genuinely,
>>>
>>> - in the nested virt setup where Ladi reported the Device Guard
>>> breakage. (If I understand correctly, ATM this requires additional
>>> host kernel (KVM) patches.)
>>>
>>> Test results / feedback from those envs would be appreciated.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Ladi Prosek <lprosek@redhat.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Laszlo Ersek (6):
>>> MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>>> header
>>> MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>>> header
>>> MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>>> hook
>>> MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>>> req
>>> MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>>> EndOfDxe
>>> MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>>> variable
>>>
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
>>> | 2 +
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h |
>>> 89 ++++++++++
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>>> | 45 +++--
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>>> | 173 ++++++++++++++++++--
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c |
>>> 51 ------
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h |
>>> 2 +
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c |
>>> 2 +
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf |
>>> 1 +
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>>> | 2 +
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>>> | 4 +
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
>>> | 16 +-
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>>> | 1 +
>>> 12 files changed, 294 insertions(+), 94 deletions(-)
>>> create mode 100644
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>>
>>> --
>>> 2.14.1.3.gb7cf6e02401b
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
` (6 preceding siblings ...)
2017-10-04 1:18 ` [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Yao, Jiewen
@ 2017-10-05 7:42 ` Zeng, Star
2017-10-05 7:57 ` Laszlo Ersek
7 siblings, 1 reply; 22+ messages in thread
From: Zeng, Star @ 2017-10-05 7:42 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Dong, Eric, Yao, Jiewen, Ladi Prosek, Zeng, Star
Laszlo,
If the series is not so urgent to be pushed, I want to take some time(of maybe one or two days) to look at the discussion background and the patches.
If it is urgent, go ahead to push the patches if you have got Jiewen's RB.
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, October 4, 2017 5:28 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
Repo: https://github.com/lersek/edk2.git
Branch: mor_lock_init_at_end_of_dxe
This patch set fixes the issue reported in the following items:
* Inconsistent MOR control variables exposed by OVMF, breaks Windows
Device Guard
https://bugzilla.redhat.com/show_bug.cgi?id=1496170
* VariableSmm MorLockInit(): create MORLock only if / after MOR exists
https://bugzilla.tianocore.org/show_bug.cgi?id=727
Patches #1 through #3 are cleanups.
Patch #4 is a small helper patch for patch #5.
Patch #5 is the actual fix, following Jiewen's suggestions from the edk2-devel thread
* [edk2] multiple levels of support for MOR / MORLock
https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some Debian versions) that create the MOR variable even if the platform doesn't offer it up-front. This patch also follows Jiewen's suggestion from the same edk2-devel thread.
(
BTW, at Paolo's recommendation, I've now reported this kernel issue for Fedora, under
* incorrect downstream-only Platform Reset Attack Mitigation patch in
the F24-F26 kernels
https://bugzilla.redhat.com/show_bug.cgi?id=1498159
)
I've checked this set for basic regressions, using OVMF, normal boot and
S3 suspend/resume:
* Q35, SMM, IA32:
- Fedora 25 -- verified patch #6 specifically
* i440fx, no SMM, X64:
- Fedora 24
* Q35, SMM, IA32X64:
- Fedora 26 -- verified patch #6 specifically
- Windows 7
- Windows 8.1
- Windows 10
- Windows Server 2008 R2
- Windows Server 2012 R2
I didn't / couldn't test this set in the following two environments:
- on platforms where TcgMor.inf is included in the firmware, and the MOR
variable exists genuinely,
- in the nested virt setup where Ladi reported the Device Guard
breakage. (If I understand correctly, ATM this requires additional
host kernel (KVM) patches.)
Test results / feedback from those envs would be appreciated.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Thanks,
Laszlo
Laszlo Ersek (6):
MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
header
MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
header
MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
hook
MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
req
MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
EndOfDxe
MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
variable
MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 89 ++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 45 +++--
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 173 ++++++++++++++++++--
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 51 ------
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 4 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 16 +-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 1 +
12 files changed, 294 insertions(+), 94 deletions(-) create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-05 7:42 ` Zeng, Star
@ 2017-10-05 7:57 ` Laszlo Ersek
2017-10-05 9:12 ` Yao, Jiewen
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-05 7:57 UTC (permalink / raw)
To: Zeng, Star, edk2-devel-01; +Cc: Dong, Eric, Yao, Jiewen, Ladi Prosek
Hi Star,
On 10/05/17 09:42, Zeng, Star wrote:
> Laszlo,
>
> If the series is not so urgent to be pushed, I want to take some time(of maybe one or two days) to look at the discussion background and the patches.
> If it is urgent, go ahead to push the patches if you have got Jiewen's RB.
Jiewen hasn't given his official R-b yet; he said that the patches
looked good to him in general.
Beyond an R-b from Jiewen and/or you and/or Eric, I wouldn't like to
push the patches until Intel QA (or one of you guys) can regression-test
the series, on a platform where TcgMor.inf is included -- that is, on a
platform where the MOR and MorLock variables exist *genuinely*. I don't
have access to such a platform (OVMF does not support these variables),
so I couldn't regression-test the series that way.
The variable driver is very important and it is shipped on all physical
platforms as well, so we shouldn't push these patches before thorough
regression-testing. I'd rather delay committing this set and do a bit
more work in RHEL7 downstream (backports) than have to fix an ugly
upstream regression in a panic.
Please take your time and review the patches and the background
discussion in detail. And, again, I would very much appreciate if you
guys or someone from Intel QA could fetch the branch and regression-test
the work, using a platform that supports MOR and MorLock for real.
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:28 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
>
> Repo: https://github.com/lersek/edk2.git
> Branch: mor_lock_init_at_end_of_dxe
>
> This patch set fixes the issue reported in the following items:
>
> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
> Device Guard
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>
> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=727
>
> Patches #1 through #3 are cleanups.
>
> Patch #4 is a small helper patch for patch #5.
>
> Patch #5 is the actual fix, following Jiewen's suggestions from the edk2-devel thread
>
> * [edk2] multiple levels of support for MOR / MORLock
>
> https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
> https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>
> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some Debian versions) that create the MOR variable even if the platform doesn't offer it up-front. This patch also follows Jiewen's suggestion from the same edk2-devel thread.
>
> (
>
> BTW, at Paolo's recommendation, I've now reported this kernel issue for Fedora, under
>
> * incorrect downstream-only Platform Reset Attack Mitigation patch in
> the F24-F26 kernels
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>
> )
>
> I've checked this set for basic regressions, using OVMF, normal boot and
> S3 suspend/resume:
>
> * Q35, SMM, IA32:
> - Fedora 25 -- verified patch #6 specifically
>
> * i440fx, no SMM, X64:
> - Fedora 24
>
> * Q35, SMM, IA32X64:
> - Fedora 26 -- verified patch #6 specifically
> - Windows 7
> - Windows 8.1
> - Windows 10
> - Windows Server 2008 R2
> - Windows Server 2012 R2
>
> I didn't / couldn't test this set in the following two environments:
>
> - on platforms where TcgMor.inf is included in the firmware, and the MOR
> variable exists genuinely,
>
> - in the nested virt setup where Ladi reported the Device Guard
> breakage. (If I understand correctly, ATM this requires additional
> host kernel (KVM) patches.)
>
> Test results / feedback from those envs would be appreciated.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (6):
> MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
> header
> MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
> header
> MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
> hook
> MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
> req
> MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
> EndOfDxe
> MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
> variable
>
> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 89 ++++++++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 45 +++--
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 173 ++++++++++++++++++--
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 51 ------
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 4 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 16 +-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 1 +
> 12 files changed, 294 insertions(+), 94 deletions(-) create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-05 7:57 ` Laszlo Ersek
@ 2017-10-05 9:12 ` Yao, Jiewen
0 siblings, 0 replies; 22+ messages in thread
From: Yao, Jiewen @ 2017-10-05 9:12 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Zeng, Star, edk2-devel-01, Dong, Eric
Thank you, Laszlo, for your patient.
Yes, I also hope to have a real platform to validate, before I give RB. :-)
Maybe some time in next week.
thank you!
Yao, Jiewen
> 在 2017年10月5日，下午3:57，Laszlo Ersek <lersek@redhat.com> 写道：
>
> Hi Star,
>
>> On 10/05/17 09:42, Zeng, Star wrote:
>> Laszlo,
>>
>> If the series is not so urgent to be pushed, I want to take some time(of maybe one or two days) to look at the discussion background and the patches.
>> If it is urgent, go ahead to push the patches if you have got Jiewen's RB.
>
> Jiewen hasn't given his official R-b yet; he said that the patches
> looked good to him in general.
>
> Beyond an R-b from Jiewen and/or you and/or Eric, I wouldn't like to
> push the patches until Intel QA (or one of you guys) can regression-test
> the series, on a platform where TcgMor.inf is included -- that is, on a
> platform where the MOR and MorLock variables exist *genuinely*. I don't
> have access to such a platform (OVMF does not support these variables),
> so I couldn't regression-test the series that way.
>
> The variable driver is very important and it is shipped on all physical
> platforms as well, so we shouldn't push these patches before thorough
> regression-testing. I'd rather delay committing this set and do a bit
> more work in RHEL7 downstream (backports) than have to fix an ugly
> upstream regression in a panic.
>
> Please take your time and review the patches and the background
> discussion in detail. And, again, I would very much appreciate if you
> guys or someone from Intel QA could fetch the branch and regression-test
> the work, using a platform that supports MOR and MorLock for real.
>
> Thanks!
> Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, October 4, 2017 5:28 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: mor_lock_init_at_end_of_dxe
>>
>> This patch set fixes the issue reported in the following items:
>>
>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>> Device Guard
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>
>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>
>> Patches #1 through #3 are cleanups.
>>
>> Patch #4 is a small helper patch for patch #5.
>>
>> Patch #5 is the actual fix, following Jiewen's suggestions from the edk2-devel thread
>>
>> * [edk2] multiple levels of support for MOR / MORLock
>>
>> https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>> https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>
>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some Debian versions) that create the MOR variable even if the platform doesn't offer it up-front. This patch also follows Jiewen's suggestion from the same edk2-devel thread.
>>
>> (
>>
>> BTW, at Paolo's recommendation, I've now reported this kernel issue for Fedora, under
>>
>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>> the F24-F26 kernels
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>
>> )
>>
>> I've checked this set for basic regressions, using OVMF, normal boot and
>> S3 suspend/resume:
>>
>> * Q35, SMM, IA32:
>> - Fedora 25 -- verified patch #6 specifically
>>
>> * i440fx, no SMM, X64:
>> - Fedora 24
>>
>> * Q35, SMM, IA32X64:
>> - Fedora 26 -- verified patch #6 specifically
>> - Windows 7
>> - Windows 8.1
>> - Windows 10
>> - Windows Server 2008 R2
>> - Windows Server 2012 R2
>>
>> I didn't / couldn't test this set in the following two environments:
>>
>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>> variable exists genuinely,
>>
>> - in the nested virt setup where Ladi reported the Device Guard
>> breakage. (If I understand correctly, ATM this requires additional
>> host kernel (KVM) patches.)
>>
>> Test results / feedback from those envs would be appreciated.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ladi Prosek <lprosek@redhat.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>> MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>> header
>> MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>> header
>> MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>> hook
>> MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>> req
>> MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>> EndOfDxe
>> MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>> variable
>>
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 89 ++++++++++
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 45 +++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 173 ++++++++++++++++++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 51 ------
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 4 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 16 +-
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 1 +
>> 12 files changed, 294 insertions(+), 94 deletions(-) create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header
2017-10-03 21:28 ` [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header Laszlo Ersek
@ 2017-10-09 6:55 ` Zeng, Star
2017-10-09 12:47 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Zeng, Star @ 2017-10-09 6:55 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Dong, Eric, Yao, Jiewen, Ladi Prosek, Zeng, Star
Minor comment:
How about also to fix the comment for Attributes parameter of SetVariableCheckHandlerMor() like below?
@param[in] Attributes Attributes bitmask to set for the variable.
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, October 4, 2017 5:29 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header
The MorLockInit() and SetVariableCheckHandlerMor() functions have separate implementations for VariableRuntimeDxe (= unprivileged, unified DXE_RUNTIME driver) and VariableSmm (= privileged, DXE_SMM back-end of the split variable driver).
Move their declarations from "Variable.c" to "PrivilegePolymorphic.h", so that the compiler enforce that the declarations and the definitions match.
(All C source files with the call sites and the function definitions already include "PrivilegePolymorphic.h" via "Variable.h".)
At the same time:
- replace two typos in the MorLockInit() description:
- replace "EFI_SUCEESS" with "EFI_SUCCESS",
- replace "MOR Lock Control" with "MOR Control Lock";
- in the SetVariableCheckHandlerMor() description:
- replace @param with @param[in],
- rewrap the comment to 80 columns.
This change cleans up commit 2f6aa774fe38 ("MdeModulePkg: Add MorLock to variable driver.", 2016-01-19).
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 41 ++++++++++++++++++++
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 30 +++++++-------
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 30 +++++++-------
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 37 ------------------
4 files changed, 75 insertions(+), 63 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
index 0aa0d4f48f10..1118f4b52e49 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
@@ -35,4 +35,45 @@ SecureBootHook (
IN EFI_GUID *VendorGuid
);
+/**
+ Initialization for MOR Control Lock.
+
+ @retval EFI_SUCCESS MorLock initialization success.
+ @return Others Some error occurs.
+**/
+EFI_STATUS
+MorLockInit (
+ VOID
+ );
+
+/**
+ This service is an MOR/MorLock checker handler for the SetVariable().
+
+ @param[in] VariableName the name of the vendor's variable, as a
+ Null-Terminated Unicode String
+ @param[in] VendorGuid Unify identifier for vendor.
+ @param[in] Attributes Point to memory location to return the attributes of
+ variable. If the point is NULL, the parameter would
+ be ignored.
+ @param[in] DataSize The size in bytes of Data-Buffer.
+ @param[in] Data Point to the content of the variable.
+
+ @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
+ driver can store the variable data.
+ @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
+ attributes is not allowed for MOR variable.
+ @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
+ @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
+ function. Variable driver can just return
+ EFI_SUCCESS.
+**/
+EFI_STATUS
+SetVariableCheckHandlerMor (
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid,
+ IN UINT32 Attributes,
+ IN UINTN DataSize,
+ IN VOID *Data
+ );
+
#endif
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
index c32eb3b1ac4b..ab3e5d416cd4 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
@@ -28,19 +28,23 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock;
/**
This service is an MOR/MorLock checker handler for the SetVariable().
- @param VariableName the name of the vendor's variable, as a
- Null-Terminated Unicode String
- @param VendorGuid Unify identifier for vendor.
- @param Attributes Point to memory location to return the attributes of variable. If the point
- is NULL, the parameter would be ignored.
- @param DataSize The size in bytes of Data-Buffer.
- @param Data Point to the content of the variable.
+ @param[in] VariableName the name of the vendor's variable, as a
+ Null-Terminated Unicode String
+ @param[in] VendorGuid Unify identifier for vendor.
+ @param[in] Attributes Point to memory location to return the attributes of
+ variable. If the point is NULL, the parameter would
+ be ignored.
+ @param[in] DataSize The size in bytes of Data-Buffer.
+ @param[in] Data Point to the content of the variable.
- @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
- @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
+ @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
+ driver can store the variable data.
+ @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
+ attributes is not allowed for MOR variable.
@retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
- @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
- Variable driver can just return EFI_SUCCESS.
+ @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
+ function. Variable driver can just return
+ EFI_SUCCESS.
**/
EFI_STATUS
SetVariableCheckHandlerMor (
@@ -58,9 +62,9 @@ SetVariableCheckHandlerMor ( }
/**
- Initialization for MOR Lock Control.
+ Initialization for MOR Control Lock.
- @retval EFI_SUCEESS MorLock initialization success.
+ @retval EFI_SUCCESS MorLock initialization success.
@return Others Some error occurs.
**/
EFI_STATUS
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index d06317ca9cf4..390c8fde4bd4 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -309,19 +309,23 @@ SetVariableCheckHandlerMorLock (
/**
This service is an MOR/MorLock checker handler for the SetVariable().
- @param VariableName the name of the vendor's variable, as a
- Null-Terminated Unicode String
- @param VendorGuid Unify identifier for vendor.
- @param Attributes Point to memory location to return the attributes of variable. If the point
- is NULL, the parameter would be ignored.
- @param DataSize The size in bytes of Data-Buffer.
- @param Data Point to the content of the variable.
+ @param[in] VariableName the name of the vendor's variable, as a
+ Null-Terminated Unicode String
+ @param[in] VendorGuid Unify identifier for vendor.
+ @param[in] Attributes Point to memory location to return the attributes of
+ variable. If the point is NULL, the parameter would
+ be ignored.
+ @param[in] DataSize The size in bytes of Data-Buffer.
+ @param[in] Data Point to the content of the variable.
- @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
- @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
+ @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
+ driver can store the variable data.
+ @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
+ attributes is not allowed for MOR variable.
@retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
- @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
- Variable driver can just return EFI_SUCCESS.
+ @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
+ function. Variable driver can just return
+ EFI_SUCCESS.
**/
EFI_STATUS
SetVariableCheckHandlerMor (
@@ -377,9 +381,9 @@ SetVariableCheckHandlerMor ( }
/**
- Initialization for MOR Lock Control.
+ Initialization for MOR Control Lock.
- @retval EFI_SUCEESS MorLock initialization success.
+ @retval EFI_SUCCESS MorLock initialization success.
@return Others Some error occurs.
**/
EFI_STATUS
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 28e4ac8f3819..d68dfbe648ce 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -97,43 +97,6 @@ AUTH_VAR_LIB_CONTEXT_IN mAuthContextIn = {
AUTH_VAR_LIB_CONTEXT_OUT mAuthContextOut;
-/**
- Initialization for MOR Lock Control.
-
- @retval EFI_SUCEESS MorLock initialization success.
- @return Others Some error occurs.
-**/
-EFI_STATUS
-MorLockInit (
- VOID
- );
-
-/**
- This service is an MOR/MorLock checker handler for the SetVariable().
-
- @param VariableName the name of the vendor's variable, as a
- Null-Terminated Unicode String
- @param VendorGuid Unify identifier for vendor.
- @param Attributes Point to memory location to return the attributes of variable. If the point
- is NULL, the parameter would be ignored.
- @param DataSize The size in bytes of Data-Buffer.
- @param Data Point to the content of the variable.
-
- @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
- @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
- @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
- @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
- Variable driver can just return EFI_SUCCESS.
-**/
-EFI_STATUS
-SetVariableCheckHandlerMor (
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid,
- IN UINT32 Attributes,
- IN UINTN DataSize,
- IN VOID *Data
- );
-
/**
Routine used to track statistical information about variable usage.
The data is stored in the EFI system table so it can be accessed later.
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
2017-10-03 21:28 ` [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable Laszlo Ersek
@ 2017-10-09 7:12 ` Zeng, Star
2017-10-09 15:20 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Zeng, Star @ 2017-10-09 7:12 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Dong, Eric, Yao, Jiewen, Ladi Prosek, Zeng, Star
Laszlo,
Do you think VariableRuntimeDxe(TcgMorLockDxe.c) also needs to delete and lock OS-created MOR variable?
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, October 4, 2017 5:29 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
According to the TCG Platform Reset Attack Mitigation Specification (May 15, 2008):
> 5 Interface for UEFI
> 5.1 UEFI Variable
> 5.1.1 The MemoryOverwriteRequestControl
>
> Start of informative comment:
>
> [...] The OS loader should not create the variable. Rather, the
> firmware is required to create it and must support the semantics described here.
>
> End of informative comment.
However, some OS kernels create the MOR variable even if the platform firmware does not support it (see one Bugzilla reference below). This OS issue breaks the logic added in the last patch.
Strengthen the MOR check by searching for the TCG or TCG2 protocols, as edk2's implementation of MOR depends on (one of) those protocols.
The protocols are defined under MdePkg, thus there's no inter-package dependency issue. In addition, calling UEFI services in
MorLockInitAtEndOfDxe() is safe, due to the following order of events /
actions:
- platform BDS signals the EndOfDxe event group,
- the SMM core installs the SmmEndOfDxe protocol,
- MorLockInitAtEndOfDxe() is invoked, and it calls UEFI services,
- some time later, platform BDS installs the DxeSmmReadyToLock protocol,
- SMM / SMRAM is locked down and UEFI services become unavailable to SMM
drivers.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1498159
Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 + MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 81 ++++++++++++++++++--
2 files changed, 77 insertions(+), 7 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
index 404164366579..69966f0d37ee 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
@@ -74,6 +74,7 @@ [LibraryClasses]
SmmMemLib
AuthVariableLib
VarCheckLib
+ UefiBootServicesTableLib
[Protocols]
gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES
@@ -85,6 +86,8 @@ [Protocols]
gEfiSmmVariableProtocolGuid
gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
gEdkiiSmmVarCheckProtocolGuid ## PRODUCES
+ gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
+ gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
[Guids]
## SOMETIMES_CONSUMES ## GUID # Signature of Variable store header
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
index 6d14b0042f4d..0a0281e44bc1 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/DebugLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/UefiBootServicesTableLib.h>
#include "Variable.h"
typedef struct {
@@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] = {
{MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid},
};
+BOOLEAN mMorPassThru = FALSE;
+
#define MOR_LOCK_DATA_UNLOCKED 0x0
#define MOR_LOCK_DATA_LOCKED_WITHOUT_KEY 0x1
#define MOR_LOCK_DATA_LOCKED_WITH_KEY 0x2
@@ -364,6 +367,13 @@ SetVariableCheckHandlerMor (
// Mor Variable
//
+ //
+ // Permit deletion for passthru request.
+ //
+ if (((Attributes == 0) || (DataSize == 0)) && mMorPassThru) {
+ return EFI_SUCCESS;
+ }
+
//
// Basic Check
//
@@ -411,6 +421,8 @@ MorLockInitAtEndOfDxe ( {
UINTN MorSize;
EFI_STATUS MorStatus;
+ EFI_STATUS TcgStatus;
+ VOID *TcgInterface;
if (!mMorLockInitializationRequired) {
//
@@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe (
if (MorStatus == EFI_BUFFER_TOO_SMALL) {
//
- // The MOR variable exists; set the MOR Control Lock variable to report the
- // capability to the OS.
+ // The MOR variable exists.
//
- SetMorLockVariable (0);
- return;
+ // Some OSes don't follow the TCG's Platform Reset Attack Mitigation spec
+ // in that the OS should never create the MOR variable, only read and write
+ // it -- these OSes (unintentionally) create MOR if the platform firmware
+ // does not produce it. Whether this is the case (from the last OS boot)
+ // can be deduced from the absence of the TCG / TCG2 protocols, as edk2's
+ // MOR implementation depends on (one of) those protocols.
+ //
+ TcgStatus = gBS->LocateProtocol (
+ &gEfiTcgProtocolGuid,
+ NULL, // Registration
+ &TcgInterface
+ );
+ if (EFI_ERROR (TcgStatus)) {
+ TcgStatus = gBS->LocateProtocol (
+ &gEfiTcg2ProtocolGuid,
+ NULL, // Registration
+ &TcgInterface
+ );
+ }
+
+ if (!EFI_ERROR (TcgStatus)) {
+ //
+ // The MOR variable originates from the platform firmware; set the MOR
+ // Control Lock variable to report the locking capability to the OS.
+ //
+ SetMorLockVariable (0);
+ return;
+ }
+
+ //
+ // The MOR variable's origin is inexplicable; delete it.
+ //
+ DEBUG ((
+ DEBUG_WARN,
+ "%a: deleting unexpected / unsupported variable %g:%s\n",
+ __FUNCTION__,
+ &gEfiMemoryOverwriteControlDataGuid,
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME
+ ));
+
+ mMorPassThru = TRUE;
+ VariableServiceSetVariable (
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid,
+ 0, // Attributes
+ 0, // DataSize
+ NULL // Data
+ );
+ mMorPassThru = FALSE;
}
//
- // The platform does not support the MOR variable. Delete the MOR Control
- // Lock variable (should it exists for some reason) and prevent other modules
- // from creating it.
+ // The MOR variable is absent; the platform firmware does not support it.
+ // Lock the variable so that no other module may create it.
+ //
+ VariableLockRequestToLock (
+ NULL, // This
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid
+ );
+
+ //
+ // Delete the MOR Control Lock variable too (should it exists for
+ some // reason) and prevent other modules from creating it.
//
mMorLockPassThru = TRUE;
VariableServiceSetVariable (
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header
2017-10-09 6:55 ` Zeng, Star
@ 2017-10-09 12:47 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-09 12:47 UTC (permalink / raw)
To: Zeng, Star, edk2-devel-01; +Cc: Dong, Eric, Yao, Jiewen, Ladi Prosek
On 10/09/17 08:55, Zeng, Star wrote:
> Minor comment:
>
> How about also to fix the comment for Attributes parameter of SetVariableCheckHandlerMor() like below?
>
> @param[in] Attributes Attributes bitmask to set for the variable.
Right, I can do that.
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:29 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header
>
> The MorLockInit() and SetVariableCheckHandlerMor() functions have separate implementations for VariableRuntimeDxe (= unprivileged, unified DXE_RUNTIME driver) and VariableSmm (= privileged, DXE_SMM back-end of the split variable driver).
>
> Move their declarations from "Variable.c" to "PrivilegePolymorphic.h", so that the compiler enforce that the declarations and the definitions match.
> (All C source files with the call sites and the function definitions already include "PrivilegePolymorphic.h" via "Variable.h".)
>
> At the same time:
>
> - replace two typos in the MorLockInit() description:
> - replace "EFI_SUCEESS" with "EFI_SUCCESS",
> - replace "MOR Lock Control" with "MOR Control Lock";
>
> - in the SetVariableCheckHandlerMor() description:
> - replace @param with @param[in],
> - rewrap the comment to 80 columns.
>
> This change cleans up commit 2f6aa774fe38 ("MdeModulePkg: Add MorLock to variable driver.", 2016-01-19).
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 41 ++++++++++++++++++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 30 +++++++-------
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 30 +++++++-------
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 37 ------------------
> 4 files changed, 75 insertions(+), 63 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index 0aa0d4f48f10..1118f4b52e49 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> @@ -35,4 +35,45 @@ SecureBootHook (
> IN EFI_GUID *VendorGuid
> );
>
> +/**
> + Initialization for MOR Control Lock.
> +
> + @retval EFI_SUCCESS MorLock initialization success.
> + @return Others Some error occurs.
> +**/
> +EFI_STATUS
> +MorLockInit (
> + VOID
> + );
> +
> +/**
> + This service is an MOR/MorLock checker handler for the SetVariable().
> +
> + @param[in] VariableName the name of the vendor's variable, as a
> + Null-Terminated Unicode String
> + @param[in] VendorGuid Unify identifier for vendor.
> + @param[in] Attributes Point to memory location to return the attributes of
> + variable. If the point is NULL, the parameter would
> + be ignored.
> + @param[in] DataSize The size in bytes of Data-Buffer.
> + @param[in] Data Point to the content of the variable.
> +
> + @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
> + driver can store the variable data.
> + @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
> + attributes is not allowed for MOR variable.
> + @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
> + @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
> + function. Variable driver can just return
> + EFI_SUCCESS.
> +**/
> +EFI_STATUS
> +SetVariableCheckHandlerMor (
> + IN CHAR16 *VariableName,
> + IN EFI_GUID *VendorGuid,
> + IN UINT32 Attributes,
> + IN UINTN DataSize,
> + IN VOID *Data
> + );
> +
> #endif
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index c32eb3b1ac4b..ab3e5d416cd4 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -28,19 +28,23 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock;
> /**
> This service is an MOR/MorLock checker handler for the SetVariable().
>
> - @param VariableName the name of the vendor's variable, as a
> - Null-Terminated Unicode String
> - @param VendorGuid Unify identifier for vendor.
> - @param Attributes Point to memory location to return the attributes of variable. If the point
> - is NULL, the parameter would be ignored.
> - @param DataSize The size in bytes of Data-Buffer.
> - @param Data Point to the content of the variable.
> + @param[in] VariableName the name of the vendor's variable, as a
> + Null-Terminated Unicode String
> + @param[in] VendorGuid Unify identifier for vendor.
> + @param[in] Attributes Point to memory location to return the attributes of
> + variable. If the point is NULL, the parameter would
> + be ignored.
> + @param[in] DataSize The size in bytes of Data-Buffer.
> + @param[in] Data Point to the content of the variable.
>
> - @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
> - @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
> + @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
> + driver can store the variable data.
> + @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
> + attributes is not allowed for MOR variable.
> @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
> - @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
> - Variable driver can just return EFI_SUCCESS.
> + @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
> + function. Variable driver can just return
> + EFI_SUCCESS.
> **/
> EFI_STATUS
> SetVariableCheckHandlerMor (
> @@ -58,9 +62,9 @@ SetVariableCheckHandlerMor ( }
>
> /**
> - Initialization for MOR Lock Control.
> + Initialization for MOR Control Lock.
>
> - @retval EFI_SUCEESS MorLock initialization success.
> + @retval EFI_SUCCESS MorLock initialization success.
> @return Others Some error occurs.
> **/
> EFI_STATUS
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index d06317ca9cf4..390c8fde4bd4 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -309,19 +309,23 @@ SetVariableCheckHandlerMorLock (
> /**
> This service is an MOR/MorLock checker handler for the SetVariable().
>
> - @param VariableName the name of the vendor's variable, as a
> - Null-Terminated Unicode String
> - @param VendorGuid Unify identifier for vendor.
> - @param Attributes Point to memory location to return the attributes of variable. If the point
> - is NULL, the parameter would be ignored.
> - @param DataSize The size in bytes of Data-Buffer.
> - @param Data Point to the content of the variable.
> + @param[in] VariableName the name of the vendor's variable, as a
> + Null-Terminated Unicode String
> + @param[in] VendorGuid Unify identifier for vendor.
> + @param[in] Attributes Point to memory location to return the attributes of
> + variable. If the point is NULL, the parameter would
> + be ignored.
> + @param[in] DataSize The size in bytes of Data-Buffer.
> + @param[in] Data Point to the content of the variable.
>
> - @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
> - @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
> + @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable
> + driver can store the variable data.
> + @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or
> + attributes is not allowed for MOR variable.
> @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
> - @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
> - Variable driver can just return EFI_SUCCESS.
> + @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this
> + function. Variable driver can just return
> + EFI_SUCCESS.
> **/
> EFI_STATUS
> SetVariableCheckHandlerMor (
> @@ -377,9 +381,9 @@ SetVariableCheckHandlerMor ( }
>
> /**
> - Initialization for MOR Lock Control.
> + Initialization for MOR Control Lock.
>
> - @retval EFI_SUCEESS MorLock initialization success.
> + @retval EFI_SUCCESS MorLock initialization success.
> @return Others Some error occurs.
> **/
> EFI_STATUS
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 28e4ac8f3819..d68dfbe648ce 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -97,43 +97,6 @@ AUTH_VAR_LIB_CONTEXT_IN mAuthContextIn = {
>
> AUTH_VAR_LIB_CONTEXT_OUT mAuthContextOut;
>
> -/**
> - Initialization for MOR Lock Control.
> -
> - @retval EFI_SUCEESS MorLock initialization success.
> - @return Others Some error occurs.
> -**/
> -EFI_STATUS
> -MorLockInit (
> - VOID
> - );
> -
> -/**
> - This service is an MOR/MorLock checker handler for the SetVariable().
> -
> - @param VariableName the name of the vendor's variable, as a
> - Null-Terminated Unicode String
> - @param VendorGuid Unify identifier for vendor.
> - @param Attributes Point to memory location to return the attributes of variable. If the point
> - is NULL, the parameter would be ignored.
> - @param DataSize The size in bytes of Data-Buffer.
> - @param Data Point to the content of the variable.
> -
> - @retval EFI_SUCCESS The MOR/MorLock check pass, and Variable driver can store the variable data.
> - @retval EFI_INVALID_PARAMETER The MOR/MorLock data or data size or attributes is not allowed for MOR variable.
> - @retval EFI_ACCESS_DENIED The MOR/MorLock is locked.
> - @retval EFI_ALREADY_STARTED The MorLock variable is handled inside this function.
> - Variable driver can just return EFI_SUCCESS.
> -**/
> -EFI_STATUS
> -SetVariableCheckHandlerMor (
> - IN CHAR16 *VariableName,
> - IN EFI_GUID *VendorGuid,
> - IN UINT32 Attributes,
> - IN UINTN DataSize,
> - IN VOID *Data
> - );
> -
> /**
> Routine used to track statistical information about variable usage.
> The data is stored in the EFI system table so it can be accessed later.
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
2017-10-09 7:12 ` Zeng, Star
@ 2017-10-09 15:20 ` Laszlo Ersek
2017-10-10 4:15 ` Yao, Jiewen
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-09 15:20 UTC (permalink / raw)
To: Zeng, Star, edk2-devel-01; +Cc: Yao, Jiewen, Dong, Eric
On 10/09/17 09:12, Zeng, Star wrote:
> Laszlo,
>
> Do you think VariableRuntimeDxe(TcgMorLockDxe.c) also needs to delete and lock OS-created MOR variable?
Maybe -- I'm not sure.
If an edk2 platform uses "VariableRuntimeDxe", then it cannot securely
support MorLock, that's for sure. However, the same platform might still
support plain MOR.
How can we determine if the platform wants to support MOR? Should we
again look for the presence of the TCG / TCG2 protocols? I cannot tell
if TcgMor.inf, and other modules under SecurityPkg, intend to support a
"no-SMM" configuration.
So I can only give you a conditional answer:
(a) If an edk2 platform can *never* sensibly support plain MOR without
SMM, then yes, we should delete and lock the MOR variable in
"TcgMorLockDxe.c", function MorLockInit().
(b) If an edk2 platform *may* opt to support plain MOR (but not MorLock)
without SMM, then we should apply the same End-of-Dxe trick as in the
SMM case. This is however quite a bit of development and I suggest that
we postpone it -- file a BZ about it -- until an edk2 platform actually
needs this. (It looks like a quite unlikely use case though: MOR is a
security feature that is not really secure without MorLock, and MorLock
is insecure without SMM. So one might reason that MOR-without-SMM is
useless.)
(c) We can also say "we don't care", because, technically speaking, no
inconsistency between the MOR and MorLock variables can be created in
the "no-SMM" case (because it is never possible to create MorLock
without MOR -- since creating MorLock is prevented already).
So, I don't know. If Jiewen thinks that "MOR without SMM" is useless
(because it is inherently insecure --> option (a)), I can append a small
patch #7, in order to delete and lock MOR too, in MorLockInit()
[TcgMorLockDxe.c].
Here's another idea: if the "no SMM" case requires extra thinking and
regression testing (i.e. the patch would be more complex than simple MOR
deletion + locking) , can we go ahead with this series for now, and file
a TianoCore BZ about the "no SMM" case? (I should say in advance that
I'm not volunteering to address the "no SMM" Bugzilla any time soon; I
don't know much (yet) about the TCG modules in SecurityPkg, and I think
I won't have time for the investigation in the next weeks.)
Jiewen, what do you say?
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:29 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
>
> According to the TCG Platform Reset Attack Mitigation Specification (May 15, 2008):
>
>> 5 Interface for UEFI
>> 5.1 UEFI Variable
>> 5.1.1 The MemoryOverwriteRequestControl
>>
>> Start of informative comment:
>>
>> [...] The OS loader should not create the variable. Rather, the
>> firmware is required to create it and must support the semantics described here.
>>
>> End of informative comment.
>
> However, some OS kernels create the MOR variable even if the platform firmware does not support it (see one Bugzilla reference below). This OS issue breaks the logic added in the last patch.
>
> Strengthen the MOR check by searching for the TCG or TCG2 protocols, as edk2's implementation of MOR depends on (one of) those protocols.
>
> The protocols are defined under MdePkg, thus there's no inter-package dependency issue. In addition, calling UEFI services in
> MorLockInitAtEndOfDxe() is safe, due to the following order of events /
> actions:
>
> - platform BDS signals the EndOfDxe event group,
> - the SMM core installs the SmmEndOfDxe protocol,
> - MorLockInitAtEndOfDxe() is invoked, and it calls UEFI services,
> - some time later, platform BDS installs the DxeSmmReadyToLock protocol,
> - SMM / SMRAM is locked down and UEFI services become unavailable to SMM
> drivers.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1498159
> Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 + MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 81 ++++++++++++++++++--
> 2 files changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 404164366579..69966f0d37ee 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -74,6 +74,7 @@ [LibraryClasses]
> SmmMemLib
> AuthVariableLib
> VarCheckLib
> + UefiBootServicesTableLib
>
> [Protocols]
> gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES
> @@ -85,6 +86,8 @@ [Protocols]
> gEfiSmmVariableProtocolGuid
> gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
> gEdkiiSmmVarCheckProtocolGuid ## PRODUCES
> + gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
>
> [Guids]
> ## SOMETIMES_CONSUMES ## GUID # Signature of Variable store header
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 6d14b0042f4d..0a0281e44bc1 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Library/DebugLib.h>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> #include "Variable.h"
>
> typedef struct {
> @@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] = {
> {MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid},
> };
>
> +BOOLEAN mMorPassThru = FALSE;
> +
> #define MOR_LOCK_DATA_UNLOCKED 0x0
> #define MOR_LOCK_DATA_LOCKED_WITHOUT_KEY 0x1
> #define MOR_LOCK_DATA_LOCKED_WITH_KEY 0x2
> @@ -364,6 +367,13 @@ SetVariableCheckHandlerMor (
> // Mor Variable
> //
>
> + //
> + // Permit deletion for passthru request.
> + //
> + if (((Attributes == 0) || (DataSize == 0)) && mMorPassThru) {
> + return EFI_SUCCESS;
> + }
> +
> //
> // Basic Check
> //
> @@ -411,6 +421,8 @@ MorLockInitAtEndOfDxe ( {
> UINTN MorSize;
> EFI_STATUS MorStatus;
> + EFI_STATUS TcgStatus;
> + VOID *TcgInterface;
>
> if (!mMorLockInitializationRequired) {
> //
> @@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe (
>
> if (MorStatus == EFI_BUFFER_TOO_SMALL) {
> //
> - // The MOR variable exists; set the MOR Control Lock variable to report the
> - // capability to the OS.
> + // The MOR variable exists.
> //
> - SetMorLockVariable (0);
> - return;
> + // Some OSes don't follow the TCG's Platform Reset Attack Mitigation spec
> + // in that the OS should never create the MOR variable, only read and write
> + // it -- these OSes (unintentionally) create MOR if the platform firmware
> + // does not produce it. Whether this is the case (from the last OS boot)
> + // can be deduced from the absence of the TCG / TCG2 protocols, as edk2's
> + // MOR implementation depends on (one of) those protocols.
> + //
> + TcgStatus = gBS->LocateProtocol (
> + &gEfiTcgProtocolGuid,
> + NULL, // Registration
> + &TcgInterface
> + );
> + if (EFI_ERROR (TcgStatus)) {
> + TcgStatus = gBS->LocateProtocol (
> + &gEfiTcg2ProtocolGuid,
> + NULL, // Registration
> + &TcgInterface
> + );
> + }
> +
> + if (!EFI_ERROR (TcgStatus)) {
> + //
> + // The MOR variable originates from the platform firmware; set the MOR
> + // Control Lock variable to report the locking capability to the OS.
> + //
> + SetMorLockVariable (0);
> + return;
> + }
> +
> + //
> + // The MOR variable's origin is inexplicable; delete it.
> + //
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: deleting unexpected / unsupported variable %g:%s\n",
> + __FUNCTION__,
> + &gEfiMemoryOverwriteControlDataGuid,
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME
> + ));
> +
> + mMorPassThru = TRUE;
> + VariableServiceSetVariable (
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> + &gEfiMemoryOverwriteControlDataGuid,
> + 0, // Attributes
> + 0, // DataSize
> + NULL // Data
> + );
> + mMorPassThru = FALSE;
> }
>
> //
> - // The platform does not support the MOR variable. Delete the MOR Control
> - // Lock variable (should it exists for some reason) and prevent other modules
> - // from creating it.
> + // The MOR variable is absent; the platform firmware does not support it.
> + // Lock the variable so that no other module may create it.
> + //
> + VariableLockRequestToLock (
> + NULL, // This
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> + &gEfiMemoryOverwriteControlDataGuid
> + );
> +
> + //
> + // Delete the MOR Control Lock variable too (should it exists for
> + some // reason) and prevent other modules from creating it.
> //
> mMorLockPassThru = TRUE;
> VariableServiceSetVariable (
> --
> 2.14.1.3.gb7cf6e02401b
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
2017-10-09 15:20 ` Laszlo Ersek
@ 2017-10-10 4:15 ` Yao, Jiewen
2017-10-10 13:14 ` Zeng, Star
0 siblings, 1 reply; 22+ messages in thread
From: Yao, Jiewen @ 2017-10-10 4:15 UTC (permalink / raw)
To: Laszlo Ersek, Zeng, Star, edk2-devel-01; +Cc: Dong, Eric
Yes, option a) looks good to me. A simple patch is good enough in this case.
In addition, this original patch passed our validation with MOR and MORL.
The series is reviewed-by: Jiewen.Yao@intel.com<mailto:Jiewen.Yao@intel.com>
We can treat a) in another patch.
Thank you
Yao Jiewen
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, October 9, 2017 11:21 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
On 10/09/17 09:12, Zeng, Star wrote:
> Laszlo,
>
> Do you think VariableRuntimeDxe(TcgMorLockDxe.c) also needs to delete and lock OS-created MOR variable?
Maybe -- I'm not sure.
If an edk2 platform uses "VariableRuntimeDxe", then it cannot securely
support MorLock, that's for sure. However, the same platform might still
support plain MOR.
How can we determine if the platform wants to support MOR? Should we
again look for the presence of the TCG / TCG2 protocols? I cannot tell
if TcgMor.inf, and other modules under SecurityPkg, intend to support a
"no-SMM" configuration.
So I can only give you a conditional answer:
(a) If an edk2 platform can *never* sensibly support plain MOR without
SMM, then yes, we should delete and lock the MOR variable in
"TcgMorLockDxe.c", function MorLockInit().
(b) If an edk2 platform *may* opt to support plain MOR (but not MorLock)
without SMM, then we should apply the same End-of-Dxe trick as in the
SMM case. This is however quite a bit of development and I suggest that
we postpone it -- file a BZ about it -- until an edk2 platform actually
needs this. (It looks like a quite unlikely use case though: MOR is a
security feature that is not really secure without MorLock, and MorLock
is insecure without SMM. So one might reason that MOR-without-SMM is
useless.)
(c) We can also say "we don't care", because, technically speaking, no
inconsistency between the MOR and MorLock variables can be created in
the "no-SMM" case (because it is never possible to create MorLock
without MOR -- since creating MorLock is prevented already).
So, I don't know. If Jiewen thinks that "MOR without SMM" is useless
(because it is inherently insecure --> option (a)), I can append a small
patch #7, in order to delete and lock MOR too, in MorLockInit()
[TcgMorLockDxe.c].
Here's another idea: if the "no SMM" case requires extra thinking and
regression testing (i.e. the patch would be more complex than simple MOR
deletion + locking) , can we go ahead with this series for now, and file
a TianoCore BZ about the "no SMM" case? (I should say in advance that
I'm not volunteering to address the "no SMM" Bugzilla any time soon; I
don't know much (yet) about the TCG modules in SecurityPkg, and I think
I won't have time for the investigation in the next weeks.)
Jiewen, what do you say?
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:29 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Subject: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
>
> According to the TCG Platform Reset Attack Mitigation Specification (May 15, 2008):
>
>> 5 Interface for UEFI
>> 5.1 UEFI Variable
>> 5.1.1 The MemoryOverwriteRequestControl
>>
>> Start of informative comment:
>>
>> [...] The OS loader should not create the variable. Rather, the
>> firmware is required to create it and must support the semantics described here.
>>
>> End of informative comment.
>
> However, some OS kernels create the MOR variable even if the platform firmware does not support it (see one Bugzilla reference below). This OS issue breaks the logic added in the last patch.
>
> Strengthen the MOR check by searching for the TCG or TCG2 protocols, as edk2's implementation of MOR depends on (one of) those protocols.
>
> The protocols are defined under MdePkg, thus there's no inter-package dependency issue. In addition, calling UEFI services in
> MorLockInitAtEndOfDxe() is safe, due to the following order of events /
> actions:
>
> - platform BDS signals the EndOfDxe event group,
> - the SMM core installs the SmmEndOfDxe protocol,
> - MorLockInitAtEndOfDxe() is invoked, and it calls UEFI services,
> - some time later, platform BDS installs the DxeSmmReadyToLock protocol,
> - SMM / SMRAM is locked down and UEFI services become unavailable to SMM
> drivers.
>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1498159
> Suggested-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 + MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 81 ++++++++++++++++++--
> 2 files changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 404164366579..69966f0d37ee 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -74,6 +74,7 @@ [LibraryClasses]
> SmmMemLib
> AuthVariableLib
> VarCheckLib
> + UefiBootServicesTableLib
>
> [Protocols]
> gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES
> @@ -85,6 +86,8 @@ [Protocols]
> gEfiSmmVariableProtocolGuid
> gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
> gEdkiiSmmVarCheckProtocolGuid ## PRODUCES
> + gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
>
> [Guids]
> ## SOMETIMES_CONSUMES ## GUID # Signature of Variable store header
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 6d14b0042f4d..0a0281e44bc1 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Library/DebugLib.h>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> #include "Variable.h"
>
> typedef struct {
> @@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] = {
> {MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid},
> };
>
> +BOOLEAN mMorPassThru = FALSE;
> +
> #define MOR_LOCK_DATA_UNLOCKED 0x0
> #define MOR_LOCK_DATA_LOCKED_WITHOUT_KEY 0x1
> #define MOR_LOCK_DATA_LOCKED_WITH_KEY 0x2
> @@ -364,6 +367,13 @@ SetVariableCheckHandlerMor (
> // Mor Variable
> //
>
> + //
> + // Permit deletion for passthru request.
> + //
> + if (((Attributes == 0) || (DataSize == 0)) && mMorPassThru) {
> + return EFI_SUCCESS;
> + }
> +
> //
> // Basic Check
> //
> @@ -411,6 +421,8 @@ MorLockInitAtEndOfDxe ( {
> UINTN MorSize;
> EFI_STATUS MorStatus;
> + EFI_STATUS TcgStatus;
> + VOID *TcgInterface;
>
> if (!mMorLockInitializationRequired) {
> //
> @@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe (
>
> if (MorStatus == EFI_BUFFER_TOO_SMALL) {
> //
> - // The MOR variable exists; set the MOR Control Lock variable to report the
> - // capability to the OS.
> + // The MOR variable exists.
> //
> - SetMorLockVariable (0);
> - return;
> + // Some OSes don't follow the TCG's Platform Reset Attack Mitigation spec
> + // in that the OS should never create the MOR variable, only read and write
> + // it -- these OSes (unintentionally) create MOR if the platform firmware
> + // does not produce it. Whether this is the case (from the last OS boot)
> + // can be deduced from the absence of the TCG / TCG2 protocols, as edk2's
> + // MOR implementation depends on (one of) those protocols.
> + //
> + TcgStatus = gBS->LocateProtocol (
> + &gEfiTcgProtocolGuid,
> + NULL, // Registration
> + &TcgInterface
> + );
> + if (EFI_ERROR (TcgStatus)) {
> + TcgStatus = gBS->LocateProtocol (
> + &gEfiTcg2ProtocolGuid,
> + NULL, // Registration
> + &TcgInterface
> + );
> + }
> +
> + if (!EFI_ERROR (TcgStatus)) {
> + //
> + // The MOR variable originates from the platform firmware; set the MOR
> + // Control Lock variable to report the locking capability to the OS.
> + //
> + SetMorLockVariable (0);
> + return;
> + }
> +
> + //
> + // The MOR variable's origin is inexplicable; delete it.
> + //
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: deleting unexpected / unsupported variable %g:%s\n",
> + __FUNCTION__,
> + &gEfiMemoryOverwriteControlDataGuid,
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME
> + ));
> +
> + mMorPassThru = TRUE;
> + VariableServiceSetVariable (
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> + &gEfiMemoryOverwriteControlDataGuid,
> + 0, // Attributes
> + 0, // DataSize
> + NULL // Data
> + );
> + mMorPassThru = FALSE;
> }
>
> //
> - // The platform does not support the MOR variable. Delete the MOR Control
> - // Lock variable (should it exists for some reason) and prevent other modules
> - // from creating it.
> + // The MOR variable is absent; the platform firmware does not support it.
> + // Lock the variable so that no other module may create it.
> + //
> + VariableLockRequestToLock (
> + NULL, // This
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> + &gEfiMemoryOverwriteControlDataGuid
> + );
> +
> + //
> + // Delete the MOR Control Lock variable too (should it exists for
> + some // reason) and prevent other modules from creating it.
> //
> mMorLockPassThru = TRUE;
> VariableServiceSetVariable (
> --
> 2.14.1.3.gb7cf6e02401b
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-04 10:39 ` Laszlo Ersek
2017-10-04 12:24 ` Ladi Prosek
@ 2017-10-10 4:17 ` Yao, Jiewen
2017-10-10 10:09 ` Laszlo Ersek
1 sibling, 1 reply; 22+ messages in thread
From: Yao, Jiewen @ 2017-10-10 4:17 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Ladi Prosek, Zeng, Star
Thanks.
Please use ASSERT() when you check in.
Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, October 4, 2017 6:40 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
On 10/04/17 03:18, Yao, Jiewen wrote:
> Thanks. Laszlo.
> This patch looks good to me in general.
>
> One minor comment is below:
> MorLockInitAtEndOfDxe()
> + if (!mMorLockInitializationRequired) {
> + //
> + // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> + // the variable write service is unavailable. Do nothing.
> + //
> + return;
> + }
> I think it is an illegal case. I would add ASSERT before return.
OK, I will replace the sentence "Do nothing." with "This should never
happen.", and also add ASSERT (FALSE) above the "return".
> And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)
Right!
Ladi stated in the RHBZ that he had built OVMF; I'll talk with him
off-list regardless, to see if I can help with anything.
Thank you Jiewen!
Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, October 4, 2017 5:28 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Ladi
>> Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
>> inconsistency
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: mor_lock_init_at_end_of_dxe
>>
>> This patch set fixes the issue reported in the following items:
>>
>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>> Device Guard
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>
>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>
>> Patches #1 through #3 are cleanups.
>>
>> Patch #4 is a small helper patch for patch #5.
>>
>> Patch #5 is the actual fix, following Jiewen's suggestions from the
>> edk2-devel thread
>>
>> * [edk2] multiple levels of support for MOR / MORLock
>>
>> https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>> https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>
>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
>> Debian versions) that create the MOR variable even if the platform
>> doesn't offer it up-front. This patch also follows Jiewen's suggestion
>> from the same edk2-devel thread.
>>
>> (
>>
>> BTW, at Paolo's recommendation, I've now reported this kernel issue for
>> Fedora, under
>>
>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>> the F24-F26 kernels
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>
>> )
>>
>> I've checked this set for basic regressions, using OVMF, normal boot and
>> S3 suspend/resume:
>>
>> * Q35, SMM, IA32:
>> - Fedora 25 -- verified patch #6 specifically
>>
>> * i440fx, no SMM, X64:
>> - Fedora 24
>>
>> * Q35, SMM, IA32X64:
>> - Fedora 26 -- verified patch #6 specifically
>> - Windows 7
>> - Windows 8.1
>> - Windows 10
>> - Windows Server 2008 R2
>> - Windows Server 2012 R2
>>
>> I didn't / couldn't test this set in the following two environments:
>>
>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>> variable exists genuinely,
>>
>> - in the nested virt setup where Ladi reported the Device Guard
>> breakage. (If I understand correctly, ATM this requires additional
>> host kernel (KVM) patches.)
>>
>> Test results / feedback from those envs would be appreciated.
>>
>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>> Cc: Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>> MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>> header
>> MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>> header
>> MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>> hook
>> MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>> req
>> MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>> EndOfDxe
>> MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>> variable
>>
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
>> | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h |
>> 89 ++++++++++
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>> | 45 +++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>> | 173 ++++++++++++++++++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c |
>> 51 ------
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h |
>> 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c |
>> 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf |
>> 1 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>> | 2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>> | 4 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
>> | 16 +-
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>> | 1 +
>> 12 files changed, 294 insertions(+), 94 deletions(-)
>> create mode 100644
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-10 4:17 ` Yao, Jiewen
@ 2017-10-10 10:09 ` Laszlo Ersek
2017-10-10 12:16 ` Zeng, Star
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2017-10-10 10:09 UTC (permalink / raw)
To: Yao, Jiewen, Zeng, Star; +Cc: edk2-devel-01, Dong, Eric, Ladi Prosek
On 10/10/17 06:17, Yao, Jiewen wrote:
> Thanks.
> Please use ASSERT() when you check in.
>
> Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
Thank you all for the (quick!) feedback; I've pushed the series:
35ac962b5473..fda8f631edbb.
Here's the cumulative diff between the posted (v1) and the pushed
series, based on the comments:
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index 759e47db7f29..b98b8556a23a 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> @@ -62,9 +62,7 @@ MorLockInitAtEndOfDxe (
> @param[in] VariableName the name of the vendor's variable, as a
> Null-Terminated Unicode String
> @param[in] VendorGuid Unify identifier for vendor.
> - @param[in] Attributes Point to memory location to return the attributes of
> - variable. If the point is NULL, the parameter would
> - be ignored.
> + @param[in] Attributes Attributes bitmask to set for the variable.
> @param[in] DataSize The size in bytes of Data-Buffer.
> @param[in] Data Point to the content of the variable.
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index a91fc42ff465..7142e2da2073 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -31,9 +31,7 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock;
> @param[in] VariableName the name of the vendor's variable, as a
> Null-Terminated Unicode String
> @param[in] VendorGuid Unify identifier for vendor.
> - @param[in] Attributes Point to memory location to return the attributes of
> - variable. If the point is NULL, the parameter would
> - be ignored.
> + @param[in] Attributes Attributes bitmask to set for the variable.
> @param[in] DataSize The size in bytes of Data-Buffer.
> @param[in] Data Point to the content of the variable.
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 0a0281e44bc1..93a300a84677 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -319,9 +319,7 @@ SetVariableCheckHandlerMorLock (
> @param[in] VariableName the name of the vendor's variable, as a
> Null-Terminated Unicode String
> @param[in] VendorGuid Unify identifier for vendor.
> - @param[in] Attributes Point to memory location to return the attributes of
> - variable. If the point is NULL, the parameter would
> - be ignored.
> + @param[in] Attributes Attributes bitmask to set for the variable.
> @param[in] DataSize The size in bytes of Data-Buffer.
> @param[in] Data Point to the content of the variable.
>
> @@ -427,8 +425,9 @@ MorLockInitAtEndOfDxe (
> if (!mMorLockInitializationRequired) {
> //
> // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> - // the variable write service is unavailable. Do nothing.
> + // the variable write service is unavailable. This should never happen.
> //
> + ASSERT (FALSE);
> return;
> }
>
In addition, I modified the commit message of patch #2 (now commit
03877377e326, "MdeModulePkg/Variable/RuntimeDxe: move MOR func.
declarations to header", 2017-09-30), to credit Star for the fixed
"Attributes" param description.
(The ASSERT from Jiewen needed no commit message update; commit
7516532f9c2d ("MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation
until EndOfDxe", 2017-09-30) already carried "Suggested-by: Jiewen Yao
<jiewen.yao@intel.com>".)
I plan to post the separate patch for option (a) soon (see
<http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A9D71F4@shsmsx102.ccr.corp.intel.com>).
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
2017-10-10 10:09 ` Laszlo Ersek
@ 2017-10-10 12:16 ` Zeng, Star
0 siblings, 0 replies; 22+ messages in thread
From: Zeng, Star @ 2017-10-10 12:16 UTC (permalink / raw)
To: Laszlo Ersek, Yao, Jiewen
Cc: edk2-devel-01, Dong, Eric, Ladi Prosek, Zeng, Star
Good to see the series pushed.
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, October 10, 2017 6:09 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Dong, Eric <eric.dong@intel.com>; Ladi Prosek <lprosek@redhat.com>
Subject: Re: [edk2] [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
On 10/10/17 06:17, Yao, Jiewen wrote:
> Thanks.
> Please use ASSERT() when you check in.
>
> Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
Thank you all for the (quick!) feedback; I've pushed the series:
35ac962b5473..fda8f631edbb.
Here's the cumulative diff between the posted (v1) and the pushed series, based on the comments:
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index 759e47db7f29..b98b8556a23a 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.
> +++ h
> @@ -62,9 +62,7 @@ MorLockInitAtEndOfDxe (
> @param[in] VariableName the name of the vendor's variable, as a
> Null-Terminated Unicode String
> @param[in] VendorGuid Unify identifier for vendor.
> - @param[in] Attributes Point to memory location to return the attributes of
> - variable. If the point is NULL, the parameter would
> - be ignored.
> + @param[in] Attributes Attributes bitmask to set for the variable.
> @param[in] DataSize The size in bytes of Data-Buffer.
> @param[in] Data Point to the content of the variable.
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index a91fc42ff465..7142e2da2073 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -31,9 +31,7 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock;
> @param[in] VariableName the name of the vendor's variable, as a
> Null-Terminated Unicode String
> @param[in] VendorGuid Unify identifier for vendor.
> - @param[in] Attributes Point to memory location to return the attributes of
> - variable. If the point is NULL, the parameter would
> - be ignored.
> + @param[in] Attributes Attributes bitmask to set for the variable.
> @param[in] DataSize The size in bytes of Data-Buffer.
> @param[in] Data Point to the content of the variable.
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 0a0281e44bc1..93a300a84677 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -319,9 +319,7 @@ SetVariableCheckHandlerMorLock (
> @param[in] VariableName the name of the vendor's variable, as a
> Null-Terminated Unicode String
> @param[in] VendorGuid Unify identifier for vendor.
> - @param[in] Attributes Point to memory location to return the attributes of
> - variable. If the point is NULL, the parameter would
> - be ignored.
> + @param[in] Attributes Attributes bitmask to set for the variable.
> @param[in] DataSize The size in bytes of Data-Buffer.
> @param[in] Data Point to the content of the variable.
>
> @@ -427,8 +425,9 @@ MorLockInitAtEndOfDxe (
> if (!mMorLockInitializationRequired) {
> //
> // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> - // the variable write service is unavailable. Do nothing.
> + // the variable write service is unavailable. This should never happen.
> //
> + ASSERT (FALSE);
> return;
> }
>
In addition, I modified the commit message of patch #2 (now commit 03877377e326, "MdeModulePkg/Variable/RuntimeDxe: move MOR func.
declarations to header", 2017-09-30), to credit Star for the fixed "Attributes" param description.
(The ASSERT from Jiewen needed no commit message update; commit 7516532f9c2d ("MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe", 2017-09-30) already carried "Suggested-by: Jiewen Yao
<jiewen.yao@intel.com>".)
I plan to post the separate patch for option (a) soon (see <http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A9D71F4@shsmsx102.ccr.corp.intel.com>).
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
2017-10-10 4:15 ` Yao, Jiewen
@ 2017-10-10 13:14 ` Zeng, Star
0 siblings, 0 replies; 22+ messages in thread
From: Zeng, Star @ 2017-10-10 13:14 UTC (permalink / raw)
To: Yao, Jiewen, Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Zeng, Star
I agree with this direction a) if we think MOR with non-SMM is useless.
Thanks,
Star
From: Yao, Jiewen
Sent: Tuesday, October 10, 2017 12:15 PM
To: Laszlo Ersek <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2] [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
Yes, option a) looks good to me. A simple patch is good enough in this case.
In addition, this original patch passed our validation with MOR and MORL.
The series is reviewed-by: Jiewen.Yao@intel.com<mailto:Jiewen.Yao@intel.com>
We can treat a) in another patch.
Thank you
Yao Jiewen
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, October 9, 2017 11:21 PM
To: Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Subject: Re: [edk2] [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
On 10/09/17 09:12, Zeng, Star wrote:
> Laszlo,
>
> Do you think VariableRuntimeDxe(TcgMorLockDxe.c) also needs to delete and lock OS-created MOR variable?
Maybe -- I'm not sure.
If an edk2 platform uses "VariableRuntimeDxe", then it cannot securely
support MorLock, that's for sure. However, the same platform might still
support plain MOR.
How can we determine if the platform wants to support MOR? Should we
again look for the presence of the TCG / TCG2 protocols? I cannot tell
if TcgMor.inf, and other modules under SecurityPkg, intend to support a
"no-SMM" configuration.
So I can only give you a conditional answer:
(a) If an edk2 platform can *never* sensibly support plain MOR without
SMM, then yes, we should delete and lock the MOR variable in
"TcgMorLockDxe.c", function MorLockInit().
(b) If an edk2 platform *may* opt to support plain MOR (but not MorLock)
without SMM, then we should apply the same End-of-Dxe trick as in the
SMM case. This is however quite a bit of development and I suggest that
we postpone it -- file a BZ about it -- until an edk2 platform actually
needs this. (It looks like a quite unlikely use case though: MOR is a
security feature that is not really secure without MorLock, and MorLock
is insecure without SMM. So one might reason that MOR-without-SMM is
useless.)
(c) We can also say "we don't care", because, technically speaking, no
inconsistency between the MOR and MorLock variables can be created in
the "no-SMM" case (because it is never possible to create MorLock
without MOR -- since creating MorLock is prevented already).
So, I don't know. If Jiewen thinks that "MOR without SMM" is useless
(because it is inherently insecure --> option (a)), I can append a small
patch #7, in order to delete and lock MOR too, in MorLockInit()
[TcgMorLockDxe.c].
Here's another idea: if the "no SMM" case requires extra thinking and
regression testing (i.e. the patch would be more complex than simple MOR
deletion + locking) , can we go ahead with this series for now, and file
a TianoCore BZ about the "no SMM" case? (I should say in advance that
I'm not volunteering to address the "no SMM" Bugzilla any time soon; I
don't know much (yet) about the TCG modules in SecurityPkg, and I think
I won't have time for the investigation in the next weeks.)
Jiewen, what do you say?
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:29 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Subject: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
>
> According to the TCG Platform Reset Attack Mitigation Specification (May 15, 2008):
>
>> 5 Interface for UEFI
>> 5.1 UEFI Variable
>> 5.1.1 The MemoryOverwriteRequestControl
>>
>> Start of informative comment:
>>
>> [...] The OS loader should not create the variable. Rather, the
>> firmware is required to create it and must support the semantics described here.
>>
>> End of informative comment.
>
> However, some OS kernels create the MOR variable even if the platform firmware does not support it (see one Bugzilla reference below). This OS issue breaks the logic added in the last patch.
>
> Strengthen the MOR check by searching for the TCG or TCG2 protocols, as edk2's implementation of MOR depends on (one of) those protocols.
>
> The protocols are defined under MdePkg, thus there's no inter-package dependency issue. In addition, calling UEFI services in
> MorLockInitAtEndOfDxe() is safe, due to the following order of events /
> actions:
>
> - platform BDS signals the EndOfDxe event group,
> - the SMM core installs the SmmEndOfDxe protocol,
> - MorLockInitAtEndOfDxe() is invoked, and it calls UEFI services,
> - some time later, platform BDS installs the DxeSmmReadyToLock protocol,
> - SMM / SMRAM is locked down and UEFI services become unavailable to SMM
> drivers.
>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1498159
> Suggested-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 + MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 81 ++++++++++++++++++--
> 2 files changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 404164366579..69966f0d37ee 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -74,6 +74,7 @@ [LibraryClasses]
> SmmMemLib
> AuthVariableLib
> VarCheckLib
> + UefiBootServicesTableLib
>
> [Protocols]
> gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES
> @@ -85,6 +86,8 @@ [Protocols]
> gEfiSmmVariableProtocolGuid
> gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
> gEdkiiSmmVarCheckProtocolGuid ## PRODUCES
> + gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
>
> [Guids]
> ## SOMETIMES_CONSUMES ## GUID # Signature of Variable store header
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 6d14b0042f4d..0a0281e44bc1 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Library/DebugLib.h>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> #include "Variable.h"
>
> typedef struct {
> @@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] = {
> {MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid},
> };
>
> +BOOLEAN mMorPassThru = FALSE;
> +
> #define MOR_LOCK_DATA_UNLOCKED 0x0
> #define MOR_LOCK_DATA_LOCKED_WITHOUT_KEY 0x1
> #define MOR_LOCK_DATA_LOCKED_WITH_KEY 0x2
> @@ -364,6 +367,13 @@ SetVariableCheckHandlerMor (
> // Mor Variable
> //
>
> + //
> + // Permit deletion for passthru request.
> + //
> + if (((Attributes == 0) || (DataSize == 0)) && mMorPassThru) {
> + return EFI_SUCCESS;
> + }
> +
> //
> // Basic Check
> //
> @@ -411,6 +421,8 @@ MorLockInitAtEndOfDxe ( {
> UINTN MorSize;
> EFI_STATUS MorStatus;
> + EFI_STATUS TcgStatus;
> + VOID *TcgInterface;
>
> if (!mMorLockInitializationRequired) {
> //
> @@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe (
>
> if (MorStatus == EFI_BUFFER_TOO_SMALL) {
> //
> - // The MOR variable exists; set the MOR Control Lock variable to report the
> - // capability to the OS.
> + // The MOR variable exists.
> //
> - SetMorLockVariable (0);
> - return;
> + // Some OSes don't follow the TCG's Platform Reset Attack Mitigation spec
> + // in that the OS should never create the MOR variable, only read and write
> + // it -- these OSes (unintentionally) create MOR if the platform firmware
> + // does not produce it. Whether this is the case (from the last OS boot)
> + // can be deduced from the absence of the TCG / TCG2 protocols, as edk2's
> + // MOR implementation depends on (one of) those protocols.
> + //
> + TcgStatus = gBS->LocateProtocol (
> + &gEfiTcgProtocolGuid,
> + NULL, // Registration
> + &TcgInterface
> + );
> + if (EFI_ERROR (TcgStatus)) {
> + TcgStatus = gBS->LocateProtocol (
> + &gEfiTcg2ProtocolGuid,
> + NULL, // Registration
> + &TcgInterface
> + );
> + }
> +
> + if (!EFI_ERROR (TcgStatus)) {
> + //
> + // The MOR variable originates from the platform firmware; set the MOR
> + // Control Lock variable to report the locking capability to the OS.
> + //
> + SetMorLockVariable (0);
> + return;
> + }
> +
> + //
> + // The MOR variable's origin is inexplicable; delete it.
> + //
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: deleting unexpected / unsupported variable %g:%s\n",
> + __FUNCTION__,
> + &gEfiMemoryOverwriteControlDataGuid,
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME
> + ));
> +
> + mMorPassThru = TRUE;
> + VariableServiceSetVariable (
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> + &gEfiMemoryOverwriteControlDataGuid,
> + 0, // Attributes
> + 0, // DataSize
> + NULL // Data
> + );
> + mMorPassThru = FALSE;
> }
>
> //
> - // The platform does not support the MOR variable. Delete the MOR Control
> - // Lock variable (should it exists for some reason) and prevent other modules
> - // from creating it.
> + // The MOR variable is absent; the platform firmware does not support it.
> + // Lock the variable so that no other module may create it.
> + //
> + VariableLockRequestToLock (
> + NULL, // This
> + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> + &gEfiMemoryOverwriteControlDataGuid
> + );
> +
> + //
> + // Delete the MOR Control Lock variable too (should it exists for
> + some // reason) and prevent other modules from creating it.
> //
> mMorLockPassThru = TRUE;
> VariableServiceSetVariable (
> --
> 2.14.1.3.gb7cf6e02401b
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-10-10 13:10 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-03 21:28 [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Laszlo Ersek
2017-10-03 21:28 ` [PATCH 1/6] MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new header Laszlo Ersek
2017-10-03 21:28 ` [PATCH 2/6] MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to header Laszlo Ersek
2017-10-09 6:55 ` Zeng, Star
2017-10-09 12:47 ` Laszlo Ersek
2017-10-03 21:28 ` [PATCH 3/6] MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe() hook Laszlo Ersek
2017-10-03 21:28 ` [PATCH 4/6] MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru req Laszlo Ersek
2017-10-03 21:28 ` [PATCH 5/6] MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe Laszlo Ersek
2017-10-03 21:28 ` [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable Laszlo Ersek
2017-10-09 7:12 ` Zeng, Star
2017-10-09 15:20 ` Laszlo Ersek
2017-10-10 4:15 ` Yao, Jiewen
2017-10-10 13:14 ` Zeng, Star
2017-10-04 1:18 ` [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Yao, Jiewen
2017-10-04 10:39 ` Laszlo Ersek
2017-10-04 12:24 ` Ladi Prosek
2017-10-10 4:17 ` Yao, Jiewen
2017-10-10 10:09 ` Laszlo Ersek
2017-10-10 12:16 ` Zeng, Star
2017-10-05 7:42 ` Zeng, Star
2017-10-05 7:57 ` Laszlo Ersek
2017-10-05 9:12 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox