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; Fri, 26 Jul 2019 03:10:17 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3BEC8308424C; Fri, 26 Jul 2019 10:10:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-40.ams2.redhat.com [10.36.116.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id CC6DC18207; Fri, 26 Jul 2019 10:10:15 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF From: "Laszlo Ersek" To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong , Jiewen Yao , Jian J Wang References: <20190718065807.434928-1-ray.ni@intel.com> Message-ID: Date: Fri, 26 Jul 2019 12:10:14 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Fri, 26 Jul 2019 10:10:17 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/26/19 11:58, Laszlo Ersek wrote: > Ray, > > On 07/18/19 08:58, Ni, Ray wrote: >> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2 >> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used >> >> updated page fault handler to treat SMM access-out as allowed >> address when static paging is not used. >> >> But that commit is not complete because the page table is still >> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM >> code accesses non-SMRAM memory, page fault is still generated. >> >> This patch skips to update page table for non-SMRAM memory and >> page table itself. >> >> Signed-off-by: Ray Ni >> Cc: Eric Dong >> Cc: Jiewen Yao >> Cc: Jian J Wang >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index 2f7d777ee7..f75e75f55c 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -1103,6 +1103,9 @@ FindSmramInfo ( >> *SmrrBase = (UINT32)CurrentSmramRange->CpuStart; >> *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize; >> >> + // >> + // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges >> + // >> do { >> Found = FALSE; >> for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) { >> @@ -1414,14 +1417,20 @@ PerformRemainingTasks ( >> SetMemMapAttributes (); >> >> // >> - // For outside SMRAM, we only map SMM communication buffer or MMIO. >> + // Do not protect memory outside SMRAM when SMM static page table is not enabled. >> // >> - SetUefiMemMapAttributes (); >> + if (mCpuSmmStaticPageTable) { >> >> - // >> - // Set page table itself to be read-only >> - // >> - SetPageTableAttributes (); >> + // >> + // For outside SMRAM, we only map SMM communication buffer or MMIO. >> + // >> + SetUefiMemMapAttributes (); >> + >> + // >> + // Set page table itself to be read-only >> + // >> + SetPageTableAttributes (); >> + } >> >> // >> // Configure SMM Code Access Check feature if available. >> > > Commit 30f6148546c7 causes a build failure, when building for IA32: > >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function 'PerformRemainingTasks': >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: 'mCpuSmmStaticPageTable' undeclared (first use in this function) >> if (mCpuSmmStaticPageTable) { > > "mCpuSmmStaticPageTable" is an X64-only variable. It is defined in > "X64/PageTbl.c", which is not linked into the IA32 binary. We must not > reference the variable in such code that is linked into both IA32 and > X64 builds, such as "PiSmmCpuDxeSmm.c". > > We have encountered the same challenge at least once in the past: > > - https://bugzilla.tianocore.org/show_bug.cgi?id=1593 > - commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand > paging in SMM", 2019-04-04) > > The right approach is to declare a new function in "PiSmmCpuDxeSmm.h", > and to provide two definitions for the function, one in > "Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32 > implementation should return a constant value. The X64 implementation > should return "mCpuSmmStaticPageTable". (In the example named above, the > functions were SaveCr2() and RestoreCr2().) > > --*-- > > I'm going to revert commit 30f6148546c7 immediately, because it breaks > the build, and because catching this issue in advance would have been > trivial, if you had attempted to build for IA32. (To be sure, the > reviewers on this patch are not responsible; reviewers are welcome, but > not required, to catch such errors that the compiler/linker is supposed > to catch.) With this build breakage, I wouldn't be able to test Eric's > series > > [edk2-devel] [Patch v3 0/6] UefiCpuPkg: Enable Edkii Mp Services2 Ppi > > and I'd like to proceed with that. The revert has commit hash d47b85a621ad. Thanks Laszlo