Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

["Laszlo Ersek" <lersek@redhat.com>]

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 <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> ---
>  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