From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.85.217.173; helo=mail-ua0-f173.google.com; envelope-from=lprosek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mail-ua0-f173.google.com (mail-ua0-f173.google.com [209.85.217.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AB7D321CF58A6 for ; Wed, 4 Oct 2017 05:20:43 -0700 (PDT) Received: by mail-ua0-f173.google.com with SMTP id v27so3120626uav.7 for ; Wed, 04 Oct 2017 05:24:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=wjoA6JjN11CcA40fy8e3ig5LVkeB1cpBk6vjYXlxHAk=; b=ORUg5EGwtVT7zIY2SZK1C2a0FTvzUfLPDij7z3Zhms7zZrNkeyiSoTfxcbrcSnXFkB aju//HLToKT9A7Zo3DbZdzuzJUwuVb14a0wXminyqHmOf2p3B05o3IJUIqQjH33CsTFS 6IGEXCLU7WJPCLGYk8vcqQh05l//xkA01X8I4xPhbMXwdIA5e+XhOqXqU1Jau0zMZE9T f2UdrNoqoyxo2ksYVikfC1TKK5NkHFG0cJu7HbhrmxWKFMlcACh7VtmPE0eJQQrJiRxM b0RoQJeCzhL74mkrJI9dLK8zqNS0ksaNVU/kRPhJ8LjqfWb7yDmti4ur6mpf6IivctKs aUkQ== X-Gm-Message-State: AHPjjUgR8Q6uNeBprq8S0gh7gmc/nuOueS9kILC7ZHJbWYYChh8MW5Yo e7TMMEp/OouuY1Oke5tBkexFSQzZdFq8wvfuJNPIGQ== X-Google-Smtp-Source: AOwi7QAPjbgXMA3DjY/GIm34v8UVs5r2YJzaNRcR4l0xj9kbRMZpG92yddUvpVEE80aaPgNAz+QgXNTY21e8fxQZSm4= X-Received: by 10.176.4.199 with SMTP id 65mr12293561uaw.47.1507119843673; Wed, 04 Oct 2017 05:24:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.103.60.26 with HTTP; Wed, 4 Oct 2017 05:24:03 -0700 (PDT) In-Reply-To: <352f1572-161d-9a32-6628-7f07b17c7ecc@redhat.com> References: <20171003212834.25740-1-lersek@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503A9CE249@shsmsx102.ccr.corp.intel.com> <352f1572-161d-9a32-6628-7f07b17c7ecc@redhat.com> From: Ladi Prosek Date: Wed, 4 Oct 2017 14:24:03 +0200 Message-ID: To: Laszlo Ersek Cc: "Yao, Jiewen" , edk2-devel-01 , "Dong, Eric" , "Zeng, Star" 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 12:20:43 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, Oct 4, 2017 at 12:39 PM, Laszlo Ersek wrote: > 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! I have tested the patch and can confirm that it fixes the Windows device guard problem. Tested-by: Ladi Prosek > 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. This was my first time building OVMF on my machine (the builds I had done before filing the RHBZ were all using Red Hat's internal build service). The only thing that threw me off was -D SMM_REQUIRE which I was initially missing so I couldn't reproduce the issue on baseline. > Thank you Jiewen! > Laszlo Thank you Laszlo and Jiewen for the very quick turnaround on this! Ladi > >> >>> -----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 >> >