1. Patchew
  2. EDK2
  3. [edk2-devel] [Patch V2 00/14] Fix issues in CpuPageTableLib
  4. View patch

[edk2-devel] [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

duntan posted 14 patches 2 years, 9 months ago
There is a newer version of this series
[edk2-devel] [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
Posted by duntan 2 years, 9 months ago 
In previous code logic, when splitting a leaf parent entry to
smaller granularity child page table, if the parent entry
Attribute&Mask(without PageTableBaseAddress field) is equal to the
input attribute&mask(without PageTableBaseAddress field), the split
process won't happen. This may lead to failure in non-1:1 mapping.

For example, there is a page table in which [0, 1G] is mapped(Lv4[0]
,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry). And we
want to remap [0, 2M] linear address range to [1G, 1G + 2M] with the
same attibute. The expected behaviour should be: split Lv3[0,0]
entry into 512 level2 entries and remap the first level2 entry to
cover [0, 2M]. But the split won't happen in previous code since
PageTableBaseAddress of input Attribute is not checked.

So, when checking if a leaf parent entry needs to be splitted, we
should also check if PageTableBaseAddress calculated by parent entry
is equal to the value caculated by input attribute.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 4c9d70fa0a..ee27238edb 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -258,6 +258,7 @@ PageTableLibMapInLevel (
   UINTN               BitStart;
   UINTN               Index;
   IA32_PAGING_ENTRY   *PagingEntry;
+  UINTN               PagingEntryIndex;
   IA32_PAGING_ENTRY   *CurrentPagingEntry;
   UINT64              RegionLength;
   UINT64              SubLength;
@@ -288,6 +289,13 @@ PageTableLibMapInLevel (
   LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
   ParentAttribute             = &LocalParentAttribute;
 
+  //
+  // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
+  //
+  BitStart         = 12 + (Level - 1) * 9;
+  PagingEntryIndex = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart, BitStart + 9 - 1);
+  RegionLength     = REGION_LENGTH (Level);
+
   //
   // ParentPagingEntry ONLY is deferenced for checking Present and MustBeOne bits
   // when Modify is FALSE.
@@ -325,8 +333,11 @@ PageTableLibMapInLevel (
     // the actual attributes of grand-parents when determing the memory type.
     //
     PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute (&ParentPagingEntry->PleB, ParentAttribute);
-    if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
-        == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
+    if ((((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
+          == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))) &&
+        (  (Mask->Bits.PageTableBaseAddress == 0)
+        || ((IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + PagingEntryIndex * RegionLength)
+            == (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset))))
     {
       //
       // This function is called when the memory length is less than the region length of the parent level.
@@ -353,8 +364,7 @@ PageTableLibMapInLevel (
       //
       PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
 
-      RegionLength = REGION_LENGTH (Level);
-      PagingEntry  = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+      PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
       for (SubOffset = 0, Index = 0; Index < 512; Index++) {
         PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
         SubOffset                += RegionLength;
@@ -425,14 +435,11 @@ PageTableLibMapInLevel (
   }
 
   //
-  // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
   // RegionStart:  points to the linear address that's aligned on RegionLength and lower than (LinearAddress + Offset).
   //
-  BitStart     = 12 + (Level - 1) * 9;
-  Index        = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart, BitStart + 9 - 1);
-  RegionLength = LShiftU64 (1, BitStart);
-  RegionMask   = RegionLength - 1;
-  RegionStart  = (LinearAddress + Offset) & ~RegionMask;
+  Index       = PagingEntryIndex;
+  RegionMask  = RegionLength - 1;
+  RegionStart = (LinearAddress + Offset) & ~RegionMask;
 
   ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute (&ParentPagingEntry->Pnle, ParentAttribute);
 
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100850): https://edk2.groups.io/g/devel/message/100850
Mute This Topic: https://groups.io/mt/97469471/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
Posted by Ni, Ray 2 years, 9 months ago 
Dun,
Can you split this patch to 2 patches?
One to move some local variable initialization to the beginning of the function.
The other to fix the bug. So the bug fix changes look smaller.

Thanks,
Ray

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1
> mapping issue
> 
> In previous code logic, when splitting a leaf parent entry to
> smaller granularity child page table, if the parent entry
> Attribute&Mask(without PageTableBaseAddress field) is equal to the
> input attribute&mask(without PageTableBaseAddress field), the split
> process won't happen. This may lead to failure in non-1:1 mapping.
> 
> For example, there is a page table in which [0, 1G] is mapped(Lv4[0]
> ,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry). And we
> want to remap [0, 2M] linear address range to [1G, 1G + 2M] with the
> same attibute. The expected behaviour should be: split Lv3[0,0]
> entry into 512 level2 entries and remap the first level2 entry to
> cover [0, 2M]. But the split won't happen in previous code since
> PageTableBaseAddress of input Attribute is not checked.
> 
> So, when checking if a leaf parent entry needs to be splitted, we
> should also check if PageTableBaseAddress calculated by parent entry
> is equal to the value caculated by input attribute.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 27
> +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 4c9d70fa0a..ee27238edb 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -258,6 +258,7 @@ PageTableLibMapInLevel (
>    UINTN               BitStart;
>    UINTN               Index;
>    IA32_PAGING_ENTRY   *PagingEntry;
> +  UINTN               PagingEntryIndex;
>    IA32_PAGING_ENTRY   *CurrentPagingEntry;
>    UINT64              RegionLength;
>    UINT64              SubLength;
> @@ -288,6 +289,13 @@ PageTableLibMapInLevel (
>    LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
>    ParentAttribute             = &LocalParentAttribute;
> 
> +  //
> +  // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21)
> or 4K (1 << 12).
> +  //
> +  BitStart         = 12 + (Level - 1) * 9;
> +  PagingEntryIndex = (UINTN)BitFieldRead64 (LinearAddress + Offset,
> BitStart, BitStart + 9 - 1);
> +  RegionLength     = REGION_LENGTH (Level);
> +
>    //
>    // ParentPagingEntry ONLY is deferenced for checking Present and
> MustBeOne bits
>    // when Modify is FALSE.
> @@ -325,8 +333,11 @@ PageTableLibMapInLevel (
>      // the actual attributes of grand-parents when determing the memory
> type.
>      //
>      PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
> -    if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> -        == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
> +    if ((((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> +          == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))) &&
> +        (  (Mask->Bits.PageTableBaseAddress == 0)
> +        || ((IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS
> (&PleBAttribute) + PagingEntryIndex * RegionLength)
> +            == (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute)
> + Offset))))
>      {
>        //
>        // This function is called when the memory length is less than the region
> length of the parent level.
> @@ -353,8 +364,7 @@ PageTableLibMapInLevel (
>        //
>        PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
> 
> -      RegionLength = REGION_LENGTH (Level);
> -      PagingEntry  = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +      PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
>        for (SubOffset = 0, Index = 0; Index < 512; Index++) {
>          PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
>          SubOffset                += RegionLength;
> @@ -425,14 +435,11 @@ PageTableLibMapInLevel (
>    }
> 
>    //
> -  // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or
> 4K (1 << 12).
>    // RegionStart:  points to the linear address that's aligned on RegionLength
> and lower than (LinearAddress + Offset).
>    //
> -  BitStart     = 12 + (Level - 1) * 9;
> -  Index        = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart,
> BitStart + 9 - 1);
> -  RegionLength = LShiftU64 (1, BitStart);
> -  RegionMask   = RegionLength - 1;
> -  RegionStart  = (LinearAddress + Offset) & ~RegionMask;
> +  Index       = PagingEntryIndex;
> +  RegionMask  = RegionLength - 1;
> +  RegionStart = (LinearAddress + Offset) & ~RegionMask;
> 
>    ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute
> (&ParentPagingEntry->Pnle, ParentAttribute);
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101188): https://edk2.groups.io/g/devel/message/101188
Mute This Topic: https://groups.io/mt/97469471/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
Posted by duntan 2 years, 9 months ago 
Okay, I'll split this patch into 2 patches in next version

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 9:28 AM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

Dun,
Can you split this patch to 2 patches?
One to move some local variable initialization to the beginning of the function.
The other to fix the bug. So the bug fix changes look smaller.

Thanks,
Ray

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; 
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann 
> <kraxel@redhat.com>
> Subject: [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 
> mapping issue
> 
> In previous code logic, when splitting a leaf parent entry to smaller 
> granularity child page table, if the parent entry 
> Attribute&Mask(without PageTableBaseAddress field) is equal to the 
> input attribute&mask(without PageTableBaseAddress field), the split 
> process won't happen. This may lead to failure in non-1:1 mapping.
> 
> For example, there is a page table in which [0, 1G] is mapped(Lv4[0] 
> ,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry). And we 
> want to remap [0, 2M] linear address range to [1G, 1G + 2M] with the 
> same attibute. The expected behaviour should be: split Lv3[0,0] entry 
> into 512 level2 entries and remap the first level2 entry to cover [0, 
> 2M]. But the split won't happen in previous code since 
> PageTableBaseAddress of input Attribute is not checked.
> 
> So, when checking if a leaf parent entry needs to be splitted, we 
> should also check if PageTableBaseAddress calculated by parent entry 
> is equal to the value caculated by input attribute.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 27
> +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 4c9d70fa0a..ee27238edb 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -258,6 +258,7 @@ PageTableLibMapInLevel (
>    UINTN               BitStart;
>    UINTN               Index;
>    IA32_PAGING_ENTRY   *PagingEntry;
> +  UINTN               PagingEntryIndex;
>    IA32_PAGING_ENTRY   *CurrentPagingEntry;
>    UINT64              RegionLength;
>    UINT64              SubLength;
> @@ -288,6 +289,13 @@ PageTableLibMapInLevel (
>    LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
>    ParentAttribute             = &LocalParentAttribute;
> 
> +  //
> +  // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 
> + << 21)
> or 4K (1 << 12).
> +  //
> +  BitStart         = 12 + (Level - 1) * 9;
> +  PagingEntryIndex = (UINTN)BitFieldRead64 (LinearAddress + Offset,
> BitStart, BitStart + 9 - 1);
> +  RegionLength     = REGION_LENGTH (Level);
> +
>    //
>    // ParentPagingEntry ONLY is deferenced for checking Present and 
> MustBeOne bits
>    // when Modify is FALSE.
> @@ -325,8 +333,11 @@ PageTableLibMapInLevel (
>      // the actual attributes of grand-parents when determing the 
> memory type.
>      //
>      PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute 
> (&ParentPagingEntry->PleB, ParentAttribute);
> -    if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> -        == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
> +    if ((((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> +          == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))) &&
> +        (  (Mask->Bits.PageTableBaseAddress == 0)
> +        || ((IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS
> (&PleBAttribute) + PagingEntryIndex * RegionLength)
> +            == (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
> + (Attribute)
> + Offset))))
>      {
>        //
>        // This function is called when the memory length is less than 
> the region length of the parent level.
> @@ -353,8 +364,7 @@ PageTableLibMapInLevel (
>        //
>        PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, 
> &AllOneMask);
> 
> -      RegionLength = REGION_LENGTH (Level);
> -      PagingEntry  = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +      PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
>        for (SubOffset = 0, Index = 0; Index < 512; Index++) {
>          PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
>          SubOffset                += RegionLength;
> @@ -425,14 +435,11 @@ PageTableLibMapInLevel (
>    }
> 
>    //
> -  // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 
> << 21) or 4K (1 << 12).
>    // RegionStart:  points to the linear address that's aligned on 
> RegionLength and lower than (LinearAddress + Offset).
>    //
> -  BitStart     = 12 + (Level - 1) * 9;
> -  Index        = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart,
> BitStart + 9 - 1);
> -  RegionLength = LShiftU64 (1, BitStart);
> -  RegionMask   = RegionLength - 1;
> -  RegionStart  = (LinearAddress + Offset) & ~RegionMask;
> +  Index       = PagingEntryIndex;
> +  RegionMask  = RegionLength - 1;
> +  RegionStart = (LinearAddress + Offset) & ~RegionMask;
> 
>    ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute 
> (&ParentPagingEntry->Pnle, ParentAttribute);
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101189): https://edk2.groups.io/g/devel/message/101189
Mute This Topic: https://groups.io/mt/97469471/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.