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

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

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
>