From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 5C1962095B066 for ; Mon, 9 Oct 2017 21:11:39 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP; 09 Oct 2017 21:15:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,503,1500966000"; d="scan'208,217";a="144673234" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga002.jf.intel.com with ESMTP; 09 Oct 2017 21:15:06 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 9 Oct 2017 21:15:06 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 9 Oct 2017 21:15:06 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Tue, 10 Oct 2017 12:15:03 +0800 From: "Yao, Jiewen" To: Laszlo Ersek , "Zeng, Star" , edk2-devel-01 CC: "Dong, Eric" Thread-Topic: [edk2] [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable Thread-Index: AQHTPI6ha+eVZ5nuMUSoZ/sJFhecoqLanKSAgACIbICAAV28oA== Date: Tue, 10 Oct 2017 04:15:02 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9D71F4@shsmsx102.ccr.corp.intel.com> References: <20171003212834.25740-1-lersek@redhat.com> <20171003212834.25740-7-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B97F30F@shsmsx102.ccr.corp.intel.com> <620b6a28-2087-aecb-8818-71739f36073b@redhat.com> In-Reply-To: <620b6a28-2087-aecb-8818-71739f36073b@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 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: Tue, 10 Oct 2017 04:11:40 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 We can treat a) in another patch. Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Monday, October 9, 2017 11:21 PM To: Zeng, Star ; edk2-devel-01 Cc: Yao, Jiewen ; Dong, Eric Subject: Re: [edk2] [PATCH 6/6] MdeModulePkg/Variable/RuntimeDxe: delete an= d 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 > > Cc: Dong, Eric >; Yao, Ji= ewen >; Ladi Prosek >; Zeng, Star > > 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 describ= ed here. >> >> End of informative comment. > > However, some OS kernels create the MOR variable even if the platform fir= mware 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 e= dk2's implementation of MOR depends on (one of) those protocols. > > The protocols are defined under MdePkg, thus there's no inter-package dep= endency 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 + MdeMo= dulePkg/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, EITH= ER EXPRESS OR IMPLIED. > #include > #include > #include > +#include > #include "Variable.h" > > typedef struct { > @@ -33,6 +34,8 @@ VARIABLE_TYPE mMorVariableType[] =3D { > {MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequ= estControlLockGuid}, > }; > > +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 > // > > + // > + // 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; > > if (!mMorLockInitializationRequired) { > // > @@ -438,17 +450,72 @@ MorLockInitAtEndOfDxe ( > > if (MorStatus =3D=3D EFI_BUFFER_TOO_SMALL) { > // > - // The MOR variable exists; set the MOR Control Lock variable to rep= ort 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 an= d write > + // it -- these OSes (unintentionally) create MOR if the platform fir= mware > + // does not produce it. Whether this is the case (from the last OS b= oot) > + // can be deduced from the absence of the TCG / TCG2 protocols, as e= dk2'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= MOR > + // Control Lock variable to report the locking capability to the O= S. > + // > + 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; > } > > // > - // The platform does not support the MOR variable. Delete the MOR Cont= rol > - // 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 =3D TRUE; > VariableServiceSetVariable ( > -- > 2.14.1.3.gb7cf6e02401b > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel