From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 2A3F52095B062 for ; Tue, 10 Oct 2017 10:18:39 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 21092C04B303; Tue, 10 Oct 2017 17:22:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 21092C04B303 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-87.rdu2.redhat.com [10.10.120.87]) by smtp.corp.redhat.com (Postfix) with ESMTP id C71B477BE4; Tue, 10 Oct 2017 17:22:05 +0000 (UTC) To: "Zeng, Star" , edk2-devel-01 Cc: "Dong, Eric" , "Yao, Jiewen" References: <20171010132443.11383-1-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B980020@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <948376a4-9609-b78f-3618-a1b45d1f0da4@redhat.com> Date: Tue, 10 Oct 2017 19:22:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B980020@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 10 Oct 2017 17:22:07 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM 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 17:18:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/10/17 15:54, Zeng, Star wrote: > Could you help make the code consistent to call > VariableServiceSetVariable() for the Attributes? One original for > MorLock is using real attributes, the new you added for Mor will use 0 > attributes. How about to both use 0 attributes. Sure, I can do that -- do you want me to set the attributes to 0 for the MorLock deletion in the same patch, or should I do it in a separate patch? > Another question, why empty MorLockInitAtEndOfDxe() needs to be > implemented in TcgMorLockDxe.c and called in VariableDxe.c? Because otherwise we would break the promise we make in "PrivilegePolymorphic.h": > 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. If I don't implement MorLockInitAtEndOfDxe() for VariableRuntimeDxe, then we'll have a declaration with no definition in VariableRuntimeDxe, and only one definition in total. The empty function call should be optimized out in RELEASE builds anyway (assuming LTO is used). It's easy to remove the empty definition and the calls to it, but then the purpose of "PrivilegePolymorphic.h" becomes less clear. For complete clarity, I would have had to introduce *two* new header files: - one for SecureBootHook(), MorLockInit() and SetVariableCheckHandlerMor() -- to be used by both SMM and non-SMM, - and another header file for just MorLockInitAtEndOfDxe() -- to be used only by SMM. (This is necessary because the call is made from "VariableSmm.c" to "TcgMorLockSmm.c"). I thought that introducing two new header files would not be accepted, so I opted for one header file only. But then I felt that the empty MorLockInitAtEndOfDxe() hook should be correctly implemented for VariableRuntimeDxe too. Thanks, Laszlo > > > Thanks, > Star > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, October 10, 2017 9:25 PM > To: edk2-devel-01 > Cc: Dong, Eric ; Yao, Jiewen ; Zeng, Star > Subject: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM > > VariableRuntimeDxe deletes and locks the MorLock variable in > MorLockInit(), with the argument that any protection provided by MorLock > can be circumvented if MorLock can be overwritten by unprivileged code > (i.e., outside of SMM). > > Extend the argument and the logic to the MOR variable, which is supposed > to be protected by MorLock. > > This change was suggested by Star; it is inspired by earlier VariableSmm > commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: delete and lock > OS-created MOR variable", 2017-10-03). > > Cc: Eric Dong > Cc: Jiewen Yao > Cc: Star Zeng > Suggested-by: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: del_and_lock_mor_without_smm > > MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 ++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > index 7142e2da2073..1610c7aa0706 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > @@ -69,29 +69,53 @@ EFI_STATUS > MorLockInit ( > VOID > ) > { > // > // Always clear variable to report unsupported to OS. > // The reason is that the DXE version is not proper to provide *protection*. > // BIOS should use SMM version variable driver to provide such capability. > // > VariableServiceSetVariable ( > MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, > &gEfiMemoryOverwriteRequestControlLockGuid, > EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, > 0, > NULL > ); > > // > // Need set this variable to be read-only to prevent other module set it. > // > VariableLockRequestToLock (&mVariableLock, MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid); > + > + // > + // The MOR variable can effectively improve platform security only when the > + // MorLock variable protects the MOR variable. In turn MorLock cannot be made > + // secure without SMM support in the platform firmware (see above). > + // > + // Thus, delete the MOR variable, should it exist for any reason (some OSes > + // are known to create MOR unintentionally, in an attempt to set it), then > + // also lock the MOR variable, in order to prevent other modules from > + // creating it. > + // > + VariableServiceSetVariable ( > + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, > + &gEfiMemoryOverwriteControlDataGuid, > + 0, // Attributes > + 0, // DataSize > + NULL // Data > + ); > + VariableLockRequestToLock ( > + &mVariableLock, > + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, > + &gEfiMemoryOverwriteControlDataGuid > + ); > + > return EFI_SUCCESS; > } > > /** > Delayed initialization for MOR Control Lock at EndOfDxe. > > This function performs any operations queued by MorLockInit(). > **/ >