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 5856721F657D5 for ; Wed, 4 Oct 2017 03:36:27 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 794DA2FA1; Wed, 4 Oct 2017 10:39:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 794DA2FA1 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-111.rdu2.redhat.com [10.10.120.111]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2A24360179; Wed, 4 Oct 2017 10:39:46 +0000 (UTC) To: "Yao, Jiewen" , edk2-devel-01 Cc: "Dong, Eric" , Ladi Prosek , "Zeng, Star" References: <20171003212834.25740-1-lersek@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503A9CE249@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <352f1572-161d-9a32-6628-7f07b17c7ecc@redhat.com> Date: Wed, 4 Oct 2017 12:39:46 +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: <74D8A39837DF1E4DA445A8C0B3885C503A9CE249@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 04 Oct 2017 10:39:48 +0000 (UTC) Subject: Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency 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: Wed, 04 Oct 2017 10:36:28 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Dong, Eric ; Yao, Jiewen ; Ladi >> Prosek ; Zeng, Star >> 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 >> Cc: Jiewen Yao >> Cc: Ladi Prosek >> Cc: Star Zeng >> >> 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 >