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.24; helo=mga09.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 213E621E78215 for ; Tue, 3 Oct 2017 18:14:54 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2017 18:18:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,475,1500966000"; d="scan'208";a="906442866" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 03 Oct 2017 18:18:15 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 3 Oct 2017 18:18:14 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 3 Oct 2017 18:18:14 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Wed, 4 Oct 2017 09:18:12 +0800 From: "Yao, Jiewen" To: Laszlo Ersek , edk2-devel-01 CC: "Dong, Eric" , Ladi Prosek , "Zeng, Star" Thread-Topic: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency Thread-Index: AQHTPI6VtxvKilSOPkeM5GLW0SvGL6LS4U8w Date: Wed, 4 Oct 2017 01:18:11 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9CE249@shsmsx102.ccr.corp.intel.com> References: <20171003212834.25740-1-lersek@redhat.com> In-Reply-To: <20171003212834.25740-1-lersek@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 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 01:14:55 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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. And I hope Ladi can help us double confirm if this patch fixed the device g= uard problem. :-) Thank you Yao Jiewen > -----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 >=20 > Repo: https://github.com/lersek/edk2.git > Branch: mor_lock_init_at_end_of_dxe >=20 > This patch set fixes the issue reported in the following items: >=20 > * Inconsistent MOR control variables exposed by OVMF, breaks Windows > Device Guard >=20 > https://bugzilla.redhat.com/show_bug.cgi?id=3D1496170 >=20 > * VariableSmm MorLockInit(): create MORLock only if / after MOR exists >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D727 >=20 > Patches #1 through #3 are cleanups. >=20 > Patch #4 is a small helper patch for patch #5. >=20 > Patch #5 is the actual fix, following Jiewen's suggestions from the > edk2-devel thread >=20 > * [edk2] multiple levels of support for MOR / MORLock >=20 > https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html > https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html >=20 > 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. >=20 > ( >=20 > BTW, at Paolo's recommendation, I've now reported this kernel issue for > Fedora, under >=20 > * incorrect downstream-only Platform Reset Attack Mitigation patch in > the F24-F26 kernels >=20 > https://bugzilla.redhat.com/show_bug.cgi?id=3D1498159 >=20 > ) >=20 > I've checked this set for basic regressions, using OVMF, normal boot and > S3 suspend/resume: >=20 > * Q35, SMM, IA32: > - Fedora 25 -- verified patch #6 specifically >=20 > * i440fx, no SMM, X64: > - Fedora 24 >=20 > * Q35, SMM, IA32X64: > - Fedora 26 -- verified patch #6 specifically > - Windows 7 > - Windows 8.1 > - Windows 10 > - Windows Server 2008 R2 > - Windows Server 2012 R2 >=20 > I didn't / couldn't test this set in the following two environments: >=20 > - on platforms where TcgMor.inf is included in the firmware, and the MOR > variable exists genuinely, >=20 > - in the nested virt setup where Ladi reported the Device Guard > breakage. (If I understand correctly, ATM this requires additional > host kernel (KVM) patches.) >=20 > Test results / feedback from those envs would be appreciated. >=20 > Cc: Eric Dong > Cc: Jiewen Yao > Cc: Ladi Prosek > Cc: Star Zeng >=20 > Thanks, > Laszlo >=20 > 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 >=20 > 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 >=20 > -- > 2.14.1.3.gb7cf6e02401b