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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53098): https://edk2.groups.io/g/devel/message/53098
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53269): https://edk2.groups.io/g/devel/message/53269
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53270): https://edk2.groups.io/g/devel/message/53270
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53271): https://edk2.groups.io/g/devel/message/53271
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53307): https://edk2.groups.io/g/devel/message/53307
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53345): https://edk2.groups.io/g/devel/message/53345
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53356): https://edk2.groups.io/g/devel/message/53356
Mute This Topic: https://groups.io/mt/69590542/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.