* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
2020-01-09 23:25 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Laszlo Ersek
@ 2020-01-15 10:36 ` Laszlo Ersek
2020-01-15 11:08 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-01-15 10:36 UTC (permalink / raw)
To: Eric Dong, Ray Ni; +Cc: edk2-devel-groups-io
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
2020-01-09 23:25 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Laszlo Ersek
2020-01-15 10:36 ` [edk2-devel] " Laszlo Ersek
@ 2020-01-15 11:08 ` Philippe Mathieu-Daudé
2020-01-15 11:34 ` Laszlo Ersek
2020-01-16 12:22 ` Ni, Ray
2020-01-17 9:49 ` [edk2-devel] " Laszlo Ersek
3 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-15 11:08 UTC (permalink / raw)
To: devel, lersek; +Cc: Eric Dong, Ray Ni
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
2020-01-15 11:08 ` Philippe Mathieu-Daudé
@ 2020-01-15 11:34 ` Laszlo Ersek
0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-01-15 11:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, devel; +Cc: Eric Dong, Ray Ni
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
2020-01-09 23:25 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Laszlo Ersek
2020-01-15 10:36 ` [edk2-devel] " Laszlo Ersek
2020-01-15 11:08 ` Philippe Mathieu-Daudé
@ 2020-01-16 12:22 ` Ni, Ray
2020-01-17 7:34 ` Laszlo Ersek
2020-01-17 9:49 ` [edk2-devel] " Laszlo Ersek
3 siblings, 1 reply; 7+ messages in thread
From: Ni, Ray @ 2020-01-16 12:22 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Dong, Eric
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
2020-01-16 12:22 ` Ni, Ray
@ 2020-01-17 7:34 ` Laszlo Ersek
0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-01-17 7:34 UTC (permalink / raw)
To: Ni, Ray, edk2-devel-groups-io; +Cc: Dong, Eric
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs
2020-01-09 23:25 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Laszlo Ersek
` (2 preceding siblings ...)
2020-01-16 12:22 ` Ni, Ray
@ 2020-01-17 9:49 ` Laszlo Ersek
3 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2020-01-17 9:49 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Eric Dong, Ray Ni, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 7+ messages in thread