[edk2-devel] [PATCH] UefiCpuPkg:fix issue when splitting paging entry

[edk2-devel] [PATCH] UefiCpuPkg:fix issue when splitting paging entry

[duntan]

This patch is to fix issue when splitting leaf paging
entry in CpuPageTableLib code.

In previous code, before we assign the new child paging
structure address to the content of splitted paging entry,
PageTableLibSetPnle() is called to make sure the bit7 is
set to 0, which indicate the previous leaf entry is
changed to non-leaf entry now. There is a gap between
we change the bit7 and we assign the new child paging
structure address to the content of the splitted paging
entry. If the address of code execution or data access
happens to be in the range covered by the splitted paging
entry, this gap may cause issue.

In this patch, we prepare the new paging entry content
value in a local variable and assign the value to the
splitted paging entry at once. The volatile keyword
is used to ensure that no optimization will occur in
compilation.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Zhou Jianfeng <jianfeng.zhou@intel.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index b10a3008e4..bdc411338f 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -342,6 +342,7 @@ PageTableLibMapInLevel (
   UINT64              PhysicalAddrInAttr;
   IA32_PAGING_ENTRY   OriginalParentPagingEntry;
   IA32_PAGING_ENTRY   OriginalCurrentPagingEntry;
+  IA32_PAGING_ENTRY   TempPagingEntry;
 
   ASSERT (Level != 0);
   ASSERT ((Attribute != NULL) && (Mask != NULL));
@@ -359,6 +360,8 @@ PageTableLibMapInLevel (
 
   OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
   OneOfPagingEntry.Uint64          = 0;
+  TempPagingEntry.Uint64           = 0;
+
   //
   // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
   //
@@ -441,8 +444,10 @@ PageTableLibMapInLevel (
       // Non-leaf entry doesn't have PAT bit. So use ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit
       // (bit12) in original big-leaf entry is not assigned to PageTableBaseAddress field of non-leaf entry.
       //
-      PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
-      ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
+      TempPagingEntry.Uint64 = ParentPagingEntry->Uint64;
+      PageTableLibSetPnle (&TempPagingEntry.Pnle, &NopAttribute, &AllOneMask);
+      TempPagingEntry.Uint64                           = ((UINTN)(VOID *)PagingEntry) | (TempPagingEntry.Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
+      *(volatile UINT64 *)&(ParentPagingEntry->Uint64) = TempPagingEntry.Uint64;
     }
   } else {
     //
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118996): https://edk2.groups.io/g/devel/message/118996
Mute This Topic: https://groups.io/mt/106150750/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg:fix issue when splitting paging entry

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 3885 bytes --]

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

Thanks,
Ray
________________________________
From: Tan, Dun <dun.tan@intel.com>
Sent: Friday, May 17, 2024 17:44
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Subject: [PATCH] UefiCpuPkg:fix issue when splitting paging entry

This patch is to fix issue when splitting leaf paging
entry in CpuPageTableLib code.

In previous code, before we assign the new child paging
structure address to the content of splitted paging entry,
PageTableLibSetPnle() is called to make sure the bit7 is
set to 0, which indicate the previous leaf entry is
changed to non-leaf entry now. There is a gap between
we change the bit7 and we assign the new child paging
structure address to the content of the splitted paging
entry. If the address of code execution or data access
happens to be in the range covered by the splitted paging
entry, this gap may cause issue.

In this patch, we prepare the new paging entry content
value in a local variable and assign the value to the
splitted paging entry at once. The volatile keyword
is used to ensure that no optimization will occur in
compilation.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Zhou Jianfeng <jianfeng.zhou@intel.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index b10a3008e4..bdc411338f 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -342,6 +342,7 @@ PageTableLibMapInLevel (
   UINT64              PhysicalAddrInAttr;
   IA32_PAGING_ENTRY   OriginalParentPagingEntry;
   IA32_PAGING_ENTRY   OriginalCurrentPagingEntry;
+  IA32_PAGING_ENTRY   TempPagingEntry;

   ASSERT (Level != 0);
   ASSERT ((Attribute != NULL) && (Mask != NULL));
@@ -359,6 +360,8 @@ PageTableLibMapInLevel (

   OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
   OneOfPagingEntry.Uint64          = 0;
+  TempPagingEntry.Uint64           = 0;
+
   //
   // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
   //
@@ -441,8 +444,10 @@ PageTableLibMapInLevel (
       // Non-leaf entry doesn't have PAT bit. So use ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit
       // (bit12) in original big-leaf entry is not assigned to PageTableBaseAddress field of non-leaf entry.
       //
-      PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
-      ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
+      TempPagingEntry.Uint64 = ParentPagingEntry->Uint64;
+      PageTableLibSetPnle (&TempPagingEntry.Pnle, &NopAttribute, &AllOneMask);
+      TempPagingEntry.Uint64                           = ((UINTN)(VOID *)PagingEntry) | (TempPagingEntry.Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
+      *(volatile UINT64 *)&(ParentPagingEntry->Uint64) = TempPagingEntry.Uint64;
     }
   } else {
     //
--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119039): https://edk2.groups.io/g/devel/message/119039
Mute This Topic: https://groups.io/mt/106150750/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6768 bytes --]

Re: [edk2-devel] [PATCH] UefiCpuPkg:fix issue when splitting paging entry

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Friday, May 17, 2024 5:45 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>;
> Zhou, Jianfeng <jianfeng.zhou@intel.com>
> Subject: [PATCH] UefiCpuPkg:fix issue when splitting paging entry
> 
> This patch is to fix issue when splitting leaf paging
> entry in CpuPageTableLib code.
> 
> In previous code, before we assign the new child paging
> structure address to the content of splitted paging entry,
> PageTableLibSetPnle() is called to make sure the bit7 is
> set to 0, which indicate the previous leaf entry is
> changed to non-leaf entry now. There is a gap between
> we change the bit7 and we assign the new child paging
> structure address to the content of the splitted paging
> entry. If the address of code execution or data access
> happens to be in the range covered by the splitted paging
> entry, this gap may cause issue.
> 
> In this patch, we prepare the new paging entry content
> value in a local variable and assign the value to the
> splitted paging entry at once. The volatile keyword
> is used to ensure that no optimization will occur in
> compilation.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Zhou Jianfeng <jianfeng.zhou@intel.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index b10a3008e4..bdc411338f 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -342,6 +342,7 @@ PageTableLibMapInLevel (
>    UINT64              PhysicalAddrInAttr;
>    IA32_PAGING_ENTRY   OriginalParentPagingEntry;
>    IA32_PAGING_ENTRY   OriginalCurrentPagingEntry;
> +  IA32_PAGING_ENTRY   TempPagingEntry;
> 
>    ASSERT (Level != 0);
>    ASSERT ((Attribute != NULL) && (Mask != NULL));
> @@ -359,6 +360,8 @@ PageTableLibMapInLevel (
> 
>    OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
>    OneOfPagingEntry.Uint64          = 0;
> +  TempPagingEntry.Uint64           = 0;
> +
>    //
>    // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21)
> or 4K (1 << 12).
>    //
> @@ -441,8 +444,10 @@ PageTableLibMapInLevel (
>        // Non-leaf entry doesn't have PAT bit. So use
> ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit
>        // (bit12) in original big-leaf entry is not assigned to PageTableBaseAddress
> field of non-leaf entry.
>        //
> -      PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
> -      ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) |
> (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
> +      TempPagingEntry.Uint64 = ParentPagingEntry->Uint64;
> +      PageTableLibSetPnle (&TempPagingEntry.Pnle, &NopAttribute,
> &AllOneMask);
> +      TempPagingEntry.Uint64                           = ((UINTN)(VOID *)PagingEntry) |
> (TempPagingEntry.Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
> +      *(volatile UINT64 *)&(ParentPagingEntry->Uint64) =
> TempPagingEntry.Uint64;
>      }
>    } else {
>      //
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119183): https://edk2.groups.io/g/devel/message/119183
Mute This Topic: https://groups.io/mt/106150750/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-