From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 31 Jul 2019 16:46:18 -0700 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 9EA6F5859E; Wed, 31 Jul 2019 23:46:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-110.ams2.redhat.com [10.36.116.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3412B60625; Wed, 31 Jul 2019 23:46:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy From: "Laszlo Ersek" To: ray.ni@intel.com, Jiewen Yao Cc: devel@edk2.groups.io, Eric Dong , Jian J Wang References: <20190731163852.191708-1-ray.ni@intel.com> <20190731163852.191708-3-ray.ni@intel.com> <50b073a1-bf04-4b8a-e267-2af933d34c5c@redhat.com> Message-ID: <9158c339-5af7-0d12-fbe9-c5c01499e54e@redhat.com> Date: Thu, 1 Aug 2019 01:46:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <50b073a1-bf04-4b8a-e267-2af933d34c5c@redhat.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.39]); Wed, 31 Jul 2019 23:46:17 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/01/19 01:13, Laszlo Ersek wrote: > Hi Ray, Jiewen, > > I've got several comments / questions: > > On 07/31/19 18:38, Ni, Ray wrote: >> This patch skips to update page table for non-SMRAM memory when >> PcdCpuSmmAccessOut is TRUE. >> So that when SMM accesses out, no page fault is triggered at all. >> >> Signed-off-by: Ray Ni >> Cc: Eric Dong >> Cc: Jiewen Yao >> Cc: Jian J Wang >> Cc: Laszlo Ersek >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++++---- >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +- >> 2 files changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index 69a04dfb23..427c33fb01 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -130,6 +130,11 @@ UINT8 mPhysicalAddressBits; >> UINT32 mSmmCr0; >> UINT32 mSmmCr4; >> >> +// >> +// Cache of PcdCpuSmmAccessOut >> +// >> +BOOLEAN mSmmAccessOut; >> + >> /** >> Initialize IDT to setup exception handlers for SMM. >> >> @@ -610,6 +615,12 @@ PiCpuSmmEntry ( >> mSmmCodeAccessCheckEnable = PcdGetBool (PcdCpuSmmCodeAccessCheckEnable); >> DEBUG ((EFI_D_INFO, "PcdCpuSmmCodeAccessCheckEnable = %d\n", mSmmCodeAccessCheckEnable)); >> >> + // >> + // Save the PcdCpuSmmAccessOut value into a global variable. >> + // >> + mSmmAccessOut = PcdGetBool (PcdCpuSmmAccessOut); >> + DEBUG ((DEBUG_INFO, "PcdCpuSmmAccessOut = %d\n", mSmmAccessOut)); >> + >> // >> // Save the PcdPteMemoryEncryptionAddressOrMask value into a global variable. >> // Make sure AddressEncMask is contained to smallest supported address field. > > The above looks correct; however, the PcdGetBool() call depends on the > INF file listing PcdCpuSmmAccessOut. > > (1) Ray, did you forget to stage the INF file update for this patch commit? > > >> @@ -1431,10 +1442,12 @@ PerformRemainingTasks ( >> // >> SetMemMapAttributes (); >> >> - // >> - // For outside SMRAM, we only map SMM communication buffer or MMIO. >> - // >> - SetUefiMemMapAttributes (); >> + if (!mSmmAccessOut) { >> + // >> + // For outside SMRAM, we only map SMM communication buffer or MMIO. >> + // >> + SetUefiMemMapAttributes (); >> + } > > This change looks OK. It seems to conditionalize the logic added in > commit d2fc7711136a ("UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging > Protection.", 2016-12-19). > > However, the comment confuses me a bit; it says "SMM communication > buffer *or MMIO*". > > I assume "SMM communication buffer" can be in "reserved, runtime and > ACPI NVS" RAM, so that part likely matches the new PCD's explanation > from patch#1. But MMIO is not mentioned in patch#1. > > (2) Should we extend the description of the PCD in patch#1, with MMIO? > > Or is MMIO considered *runtime* MMIO (and so covered by "runtime")? > >> >> // >> // Set page table itself to be read-only > > In the previous version of the patch series, namely > > [edk2-devel] [PATCH 3/3] > UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF > > http://mid.mail-archive.com/20190727032850.337840-4-ray.ni@intel.com > https://edk2.groups.io/g/devel/message/44470 > > the next operation (namely SetPageTableAttributes()) was conditionalized > too. Is that no longer necessary, with the new PCD? Shouldn't we still > conditionalize SetPageTableAttributes() on the *other* (static paging) PCD? > > Ah wait, we already do it! SetPageTableAttributes() has two > implementations, one for IA32 and another for X64. > > - The X64 variant checks for static page tables internally, and if they > are disabled, then SetPageTableAttributes() does nothing. > > - The IA32 variant does not contain that check, because on IA32 the page > tables are always built statically. > > So SetPageTableAttributes() already depends on static paging > *internally*, hence gating the SetPageTableAttributes() call > *externally*, like it was done in the previous version of the patch > series, is superfluous in the first place. > > Good! > > >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> index a3b62f7787..6699aac65d 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> @@ -1029,7 +1029,7 @@ SmiPFHandler ( >> goto Exit; >> } >> >> - if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFAddress)) { >> + if (IsSmmCommBufferForbiddenAddress (PFAddress)) { >> DumpCpuContext (InterruptType, SystemContext); >> DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress)); >> DEBUG_CODE ( >> > > This hunk looks good. Because: > > - we ensure that there is no page fault at all, in the "access-out > permitted" case, so there is no need to consider "access-out" in the > fault handler at all; > > - the "mSmmAccessOut" logic in PerformRemainingTasks() applies to both > IA32 and X64 (commonly), and the SmiPFHandler() function is implemented > for both IA32 and X64 (separately), and after this patch, both fault > handlers check IsSmmCommBufferForbiddenAddress() identically. So this is > nice and symmetric; > > - we don't interfere with on-demand page table building (when it is > enabled on X64) -- page faults should still be triggered by RAM pages > not yet mapped at all, and the X64 variant of SmiDefaultPFHandler() > should fix up *those* faults like before. > > > However: given that this hunk practically undes commit c60d36b4, I would > suggest that we revert commit c60d36b4 in a separate patch. So the > series would go like: > > - patch#1: commit c60d36b4 is not good enough, we're going to implement > a separate approach, so revert commit c60d36b4, at first. > - patch#2: introduce the new PCD > - patch#3: disable page fault generation for non-SMRAM addresses (= keep > them mapped as normal) to which access-out is permitted. > > (3) Ray, do you agree to structure the patch series like that? Hmmm wait a bit! Just because we skip SetUefiMemMapAttributes() in PerformRemainingTasks(), that doesn't mean we cannot end up in SmiPFHandler()! Because other faults are possible too. Whenever we skip SetUefiMemMapAttributes() in PerformRemainingTasks(), we should *also* skip the IsSmmCommBufferForbiddenAddress() check in SmiPFHandler(). Both SetUefiMemMapAttributes() and IsSmmCommBufferForbiddenAddress() come from commit d2fc7711136a -- they are two parts of the same protection feature. If we disable the protection for the sake of access-out, we should disable all parts. Furthermore, if we don't call either of IsSmmCommBufferForbiddenAddress() and SetUefiMemMapAttributes(), then we shouldn't call GetUefiMemoryMap() either! So ultimately, I would argue for the following patch series: - patch#1: Revert commit c60d36b4, and explain why, in the commit message. - patch#2: Introduce the new PCD, also mentioning MMIO. - Patch#3: modify *all* of the following functions, internally, to return immediately, if "mSmmAccessOut" is TRUE: - GetUefiMemoryMap() - SetUefiMemMapAttributes() - IsSmmCommBufferForbiddenAddress() Basically, the new PCD should short-circuit all three functions declared in "PiSmmCpuDxeSmm.h" by commit d2fc7711136a. Thanks! Laszlo > (4) Jiewen, are you OK with the general approach, namely to relax > access-out by eliminating *such* page faults, rather than fixing them up > in the fault handler? > > (I certainly agree with this approach: the fixup in the fault handler, > namely SmiDefaultPFHandler(), only covers the X64 case; in the IA32 > case, it just hangs. That shows that the fault fixup is inherently tied > to on-demand page table building, whereas the access-out feature should > be possible to permit on IA32 too.) > > Thanks, > Laszlo >