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 02:58:19 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EB369821EF; Fri, 26 Jul 2019 09:58:18 +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 57F3D60267; Fri, 26 Jul 2019 09:58:17 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF 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> From: "Laszlo Ersek" Message-ID: Date: Fri, 26 Jul 2019 11:58:16 +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: <20190718065807.434928-1-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 26 Jul 2019 09:58:19 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. Thanks Laszlo