From: Laszlo Ersek <lersek@redhat.com>
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Eric Dong <eric.dong@intel.com>,
Jiewen Yao <jiewen.yao@intel.com>,
Ladi Prosek <lprosek@redhat.com>, Star Zeng <star.zeng@intel.com>
Subject: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable
Date: Tue, 3 Oct 2017 23:28:34 +0200 [thread overview]
Message-ID: <20171003212834.25740-7-lersek@redhat.com> (raw)
In-Reply-To: <20171003212834.25740-1-lersek@redhat.com>
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
next prev parent reply other threads:[~2017-10-03 21:25 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Laszlo Ersek [this message]
2017-10-09 7:12 ` [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171003212834.25740-7-lersek@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox