From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D57A21FC7464 for ; Mon, 9 Oct 2017 00:08:51 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Oct 2017 00:12:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,499,1500966000"; d="scan'208";a="160457415" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 09 Oct 2017 00:12:17 -0700 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 9 Oct 2017 00:12:17 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 9 Oct 2017 00:12:17 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Mon, 9 Oct 2017 15:12:16 +0800 From: "Zeng, Star" To: Laszlo Ersek , edk2-devel-01 CC: "Dong, Eric" , "Yao, Jiewen" , Ladi Prosek , "Zeng, Star" Thread-Topic: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable Thread-Index: AQHTPI6hvjRgID5siUW1cftt6LBLuaLbIXaw Date: Mon, 9 Oct 2017 07:12:15 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B97F30F@shsmsx102.ccr.corp.intel.com> References: <20171003212834.25740-1-lersek@redhat.com> <20171003212834.25740-7-lersek@redhat.com> In-Reply-To: <20171003212834.25740-7-lersek@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Oct 2017 07:08:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Do you think VariableRuntimeDxe(TcgMorLockDxe.c) also needs to delete and l= ock OS-created MOR variable? Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Wednesday, October 4, 2017 5:29 AM To: edk2-devel-01 Cc: Dong, Eric ; Yao, Jiewen ; L= adi Prosek ; Zeng, Star Subject: [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-c= reated 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=20 > firmware is required to create it and must support the semantics describe= d here. > > End of informative comment. However, some OS kernels create the MOR variable even if the platform firmw= are does not support it (see one Bugzilla reference below). This OS issue b= reaks the logic added in the last patch. Strengthen the MOR check by searching for the TCG or TCG2 protocols, as edk= 2's implementation of MOR depends on (one of) those protocols. The protocols are defined under MdePkg, thus there's no inter-package depen= dency 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 Cc: Jiewen Yao Cc: Ladi Prosek Cc: Star Zeng Ref: https://bugzilla.redhat.com/show_bug.cgi?id=3D1498159 Suggested-by: Jiewen Yao Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek --- MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 + MdeModu= lePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 81 ++++++++++++++++++= -- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/M= deModulePkg/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 =20 [Protocols] gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES @@ -85,6 +86,8 @@ [Protocols] gEfiSmmVariableProtocolGuid gEfiSmmEndOfDxeProtocolGuid ## NOTIFY gEdkiiSmmVarCheckProtocolGuid ## PRODUCES + gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES + gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES =20 [Guids] ## SOMETIMES_CONSUMES ## GUID # Signature of Variable store header diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/M= deModulePkg/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 #include #include +#include #include "Variable.h" =20 typedef struct { @@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] =3D { {MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteReques= tControlLockGuid}, }; =20 +BOOLEAN mMorPassThru =3D 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 // =20 + // + // Permit deletion for passthru request. + // + if (((Attributes =3D=3D 0) || (DataSize =3D=3D 0)) && mMorPassThru) { + return EFI_SUCCESS; + } + // // Basic Check // @@ -411,6 +421,8 @@ MorLockInitAtEndOfDxe ( { UINTN MorSize; EFI_STATUS MorStatus; + EFI_STATUS TcgStatus; + VOID *TcgInterface; =20 if (!mMorLockInitializationRequired) { // @@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe ( =20 if (MorStatus =3D=3D EFI_BUFFER_TOO_SMALL) { // - // The MOR variable exists; set the MOR Control Lock variable to repor= t the - // capability to the OS. + // The MOR variable exists. // - SetMorLockVariable (0); - return; + // Some OSes don't follow the TCG's Platform Reset Attack Mitigation s= pec + // in that the OS should never create the MOR variable, only read and = write + // it -- these OSes (unintentionally) create MOR if the platform firmw= are + // does not produce it. Whether this is the case (from the last OS boo= t) + // can be deduced from the absence of the TCG / TCG2 protocols, as edk= 2's + // MOR implementation depends on (one of) those protocols. + // + TcgStatus =3D gBS->LocateProtocol ( + &gEfiTcgProtocolGuid, + NULL, // Registration + &TcgInterface + ); + if (EFI_ERROR (TcgStatus)) { + TcgStatus =3D gBS->LocateProtocol ( + &gEfiTcg2ProtocolGuid, + NULL, // Registration + &TcgInterface + ); + } + + if (!EFI_ERROR (TcgStatus)) { + // + // The MOR variable originates from the platform firmware; set the M= OR + // 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 =3D TRUE; + VariableServiceSetVariable ( + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, + &gEfiMemoryOverwriteControlDataGuid, + 0, // Attributes + 0, // DataSize + NULL // Data + ); + mMorPassThru =3D FALSE; } =20 // - // The platform does not support the MOR variable. Delete the MOR Contro= l - // Lock variable (should it exists for some reason) and prevent other mo= dules - // 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=20 + some // reason) and prevent other modules from creating it. // mMorLockPassThru =3D TRUE; VariableServiceSetVariable ( -- 2.14.1.3.gb7cf6e02401b