From 2613601640be75f79e9dd8d2db21ad45d227d907 Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

Date: Fri, 17 Jan 2020 11:33:43 +0100

Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting

 regression for PDEs

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

RH-Author: Laszlo Ersek <lersek@redhat.com>

Message-id: <20200117113343.30392-2-lersek@redhat.com>

Patchwork-id: 93389

O-Subject: [RHEL-8.2.0 edk2 PATCH 1/1] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

Bugzilla: 1789335

RH-Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

RH-Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>

In commit 4eee0cc7cc0d ("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when

CPU supports", 2019-07-12), the Page Directory Entry setting was regressed

(corrupted) when splitting a 2MB page to 512 4KB pages, in the

InitPaging() function.

Consider the following hunk, displayed with

$ git show --function-context --ignore-space-change 4eee0cc7cc0db

>            //

>            // If it is 2M page, check IsAddressSplit()

>            //

>            if (((*Pd & IA32_PG_PS) != 0) && IsAddressSplit (Address)) {

>              //

>              // Based on current page table, create 4KB page table for split area.

>              //

>              ASSERT (Address == (*Pd & PHYSICAL_ADDRESS_MASK));

>

>              Pt = AllocatePageTableMemory (1);

>              ASSERT (Pt != NULL);

>

> +            *Pd = (UINTN) Pt | IA32_PG_RW | IA32_PG_P;

> +

>              // Split it

> -          for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++) {

> -            Pt[PtIndex] = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS);

> +            for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++, Pt++) {

> +              *Pt = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS);

>              } // end for PT

>              *Pd = (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBUTE_BITS;

>            } // end if IsAddressSplit

>          } // end for PD

First, the new assignment to the Page Directory Entry (*Pd) is

superfluous. That's because (a) we set (*Pd) after the Page Table Entry

loop anyway, and (b) here we do not attempt to access the memory starting

at "Address" (which is mapped by the original value of the Page Directory

Entry).

Second, appending "Pt++" to the incrementing expression of the PTE loop is

a bug. It causes "Pt" to point *right past* the just-allocated Page Table,

once we finish the loop. But the PDE assignment that immediately follows

the loop assumes that "Pt" still points to the *start* of the new Page

Table.

The result is that the originally mapped 2MB page disappears from the

processor's view. The PDE now points to a "Page Table" that is filled with

garbage. The random entries in that "Page Table" will cause some virtual

addresses in the original 2MB area to fault. Other virtual addresses in

the same range will no longer have a 1:1 physical mapping, but be

scattered over random physical page frames.

The second phase of the InitPaging() function ("Go through page table and

set several page table entries to absent or execute-disable") already

manipulates entries in wrong Page Tables, for such PDEs that got split in

the first phase.

This issue has been caught as follows:

- OVMF is started with 2001 MB of guest RAM.

- This places the main SMRAM window at 0x7C10_1000.

- The SMRAM management in the SMM Core links this SMRAM window into

  "mSmmMemoryMap", with a FREE_PAGE_LIST record placed at the start of the

  area.

- At "SMM Ready To Lock" time, PiSmmCpuDxeSmm calls InitPaging(). The

  first phase (quoted above) decides to split the 2MB page at 0x7C00_0000

  into 512 4KB pages, and corrupts the PDE. The new Page Table is

  allocated at 0x7CE0_D000, but the PDE is set to 0x7CE0_E000 (plus

  attributes 0x67).

- Due to the corrupted PDE, the second phase of InitPaging() already looks

  up the PTE for Address=0x7C10_1000 in the wrong place. The second phase

  goes on to mark bogus PTEs as "NX".

- PiSmmCpuDxeSmm calls SetMemMapAttributes(). Address 0x7C10_1000 is at

  the base of the SMRAM window, therefore it happens to be listed in the

  SMRAM map as an EfiConventionalMemory region. SetMemMapAttributes()

  calls SmmSetMemoryAttributes() to mark the region as XP. However,

  GetPageTableEntry() in ConvertMemoryPageAttributes() fails -- address

  0x7C10_1000 is no longer mapped by anything! -- and so the attribute

  setting fails with RETURN_UNSUPPORTED. This error goes unnoticed, as

  SetMemMapAttributes() ignores the return value of

  SmmSetMemoryAttributes().

- When SetMemMapAttributes() reaches another entry in the SMRAM map,

  ConvertMemoryPageAttributes() decides it needs to split a 2MB page, and

  calls SplitPage().

- SplitPage() calls AllocatePageTableMemory() for the new Page Table,

  which takes us to InternalAllocMaxAddress() in the SMM Core.

- The SMM core attempts to read the FREE_PAGE_LIST record at 0x7C10_1000.

  Because this virtual address is no longer mapped, the firmware crashes

  in InternalAllocMaxAddress(), when accessing (Pages->NumberOfPages).

Remove the useless assignment to (*Pd) from before the loop. Revert the

loop incrementing and the PTE assignment to the known good version.

Cc: Eric Dong <eric.dong@intel.com>

Cc: Ray Ni <ray.ni@intel.com>

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1789335

Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358

Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Reviewed-by: Ray Ni <ray.ni@intel.com>

(cherry picked from commit a5235562444021e9c5aff08f45daa6b5b7952c7a)

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

---

 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 6 ++----

 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c

index c513152..c47b557 100644

--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c

+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c

@@ -657,11 +657,9 @@ InitPaging (

             Pt = AllocatePageTableMemory (1);

             ASSERT (Pt != NULL);

-            *Pd = (UINTN) Pt | IA32_PG_RW | IA32_PG_P;

-

             // Split it

-            for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++, Pt++) {

-              *Pt = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS);

+            for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++) {

+              Pt[PtIndex] = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS);

             } // end for PT

             *Pd = (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBUTE_BITS;

           } // end if IsAddressSplit

-- 

1.8.3.1