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

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

[Laszlo Ersek]

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>
---

Notes:
    Repo:   https://github.com/lersek/edk2.git
    Branch: smm_cpu_page_split_corrupt_pde

 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 c5131526f0c6..c47b5573e366 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
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

[Laszlo Ersek]

Hi Ray, Eric,

any comments about the patch below?

It hasn't been a week yet (I usually don't ping before a full week), but
the patch is not large, so I figure maybe you've missed it. (Traffic on
<devel@edk2.groups.io> has been quite high recently.)

Thanks!
Laszlo

On 01/10/20 00:25, Laszlo Ersek wrote:
> 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>
> ---
> 
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: smm_cpu_page_split_corrupt_pde
> 
>  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 c5131526f0c6..c47b5573e366 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
> 

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

[Philippe Mathieu-Daudé]

On 1/10/20 12:25 AM, Laszlo Ersek wrote:
> 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>
> ---
> 
> Notes:
>      Repo:   https://github.com/lersek/edk2.git
>      Branch: smm_cpu_page_split_corrupt_pde
> 
>   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 c5131526f0c6..c47b5573e366 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
> 

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

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

[Laszlo Ersek]

On 01/15/20 12:08, Philippe Mathieu-Daudé wrote:
> On 1/10/20 12:25 AM, Laszlo Ersek wrote:
>> 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.

>> Remove the useless assignment to (*Pd) from before the loop. Revert the
>> loop incrementing and the PTE assignment to the known good version.

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

Thank you!
Laszlo

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

[Ni, Ray]

Laszlo,
Thanks for finding and fixing the bug.

The code change for 5level paging was done many years ago
before mAddressEncMask was added.

The two lines of *Pd assignment might be caused when resolving
the local merge conflict.

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

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 10, 2020 7:25 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
> 
> 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>
> ---
> 
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: smm_cpu_page_split_corrupt_pde
> 
>  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 c5131526f0c6..c47b5573e366 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
> --
> 2.19.1.3.g30247aa5d201

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

[Laszlo Ersek]

On 01/16/20 13:22, Ni, Ray wrote:
> Laszlo,
> Thanks for finding and fixing the bug.
> 
> The code change for 5level paging was done many years ago
> before mAddressEncMask was added.
> 
> The two lines of *Pd assignment might be caused when resolving
> the local merge conflict.

Ah, that makes total sense. I didn't expect this reason!

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

Thanks!
Laszlo

> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Friday, January 10, 2020 7:25 AM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
>>
>> 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>
>> ---
>>
>> Notes:
>>     Repo:   https://github.com/lersek/edk2.git
>>     Branch: smm_cpu_page_split_corrupt_pde
>>
>>  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 c5131526f0c6..c47b5573e366 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
>> --
>> 2.19.1.3.g30247aa5d201
> 

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs

[Laszlo Ersek]

On 01/10/20 00:25, Laszlo Ersek wrote:
> 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>
> ---
> 
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: smm_cpu_page_split_corrupt_pde
> 
>  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 c5131526f0c6..c47b5573e366 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
> 

Pushed as commit a52355624440; thank you, Phil & Ray!
Laszlo