[edk2-devel] [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

duntan posted 14 patches 2 years, 9 months ago
There is a newer version of this series
[edk2-devel] [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap
Posted by duntan 2 years, 9 months ago
When creating new page table or mapping not-present range in
existing page table, we need to make sure all the non-reserved
fields of input Mask are not 0 and Present field of input
Attribute is 1.

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 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 0f3d0d684e..56f762a15e 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -214,6 +214,33 @@ PageTableLibSetPnle (
   Pnle->Bits.CacheDisabled = 0;
 }
 
+/**
+  Check if any Non-Reserved field of Mask is 0 or Attribute->Bits.Present is 0
+  when creating new page table or mapping not-present range.
+
+  @param[in] Attribute    The attribute of the linear address range.
+  @param[in] Mask         The mask used for attribute to check.
+
+  @retval RETURN_INVALID_PARAMETER    There is 0-value field in Non-Reserved fields of Mask or Attribute->Bits.Present is 0.
+  @retval RETURN_SUCCESS              All Non-Reserved fields of Mask are not 0 and Attribute->Bits.Present is 1.
+**/
+RETURN_STATUS
+CheckMaskAndAttrForNotPresentEntry (
+  IN     IA32_MAP_ATTRIBUTE  *Attribute,
+  IN     IA32_MAP_ATTRIBUTE  *Mask
+  )
+{
+  if ((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || (Mask->Bits.ReadWrite == 0) ||
+      (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
+      (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask->Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
+      (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
+  {
+    return RETURN_INVALID_PARAMETER;
+  }
+
+  return RETURN_SUCCESS;
+}
+
 /**
   Update page table to map [LinearAddress, LinearAddress + Length) with specified attribute in the specified level.
 
@@ -259,6 +286,7 @@ PageTableLibMapInLevel (
   UINTN               Index;
   IA32_PAGING_ENTRY   *PagingEntry;
   UINTN               PagingEntryIndex;
+  UINTN               PagingEntryIndexLimit;
   IA32_PAGING_ENTRY   *CurrentPagingEntry;
   UINT64              RegionLength;
   UINT64              SubLength;
@@ -302,6 +330,14 @@ PageTableLibMapInLevel (
   //
 
   if (ParentPagingEntry->Pce.Present == 0) {
+    //
+    // [LinearAddress, LinearAddress + Length] contains not-present range.
+    //
+    Status = CheckMaskAndAttrForNotPresentEntry (Attribute, Mask);
+    if (RETURN_ERROR (Status)) {
+      return Status;
+    }
+
     //
     // The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
     // It does NOT point to an existing page directory.
@@ -372,6 +408,23 @@ PageTableLibMapInLevel (
       PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
     }
   } else {
+    PagingEntry           = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1, BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ? 511 :
+                            (UINTN)BitFieldRead64 (LinearAddress + Length - 1, BitStart, BitStart + 9 - 1);
+    for (Index = PagingEntryIndex; Index <= PagingEntryIndexLimit; Index++) {
+      if (PagingEntry[Index].Pce.Present == 0) {
+        //
+        // [LinearAddress, LinearAddress + Length] contains not-present range.
+        //
+        Status = CheckMaskAndAttrForNotPresentEntry (Attribute, Mask);
+        if (RETURN_ERROR (Status)) {
+          return Status;
+        }
+
+        break;
+      }
+    }
+
     //
     // It's a non-leaf entry
     //
@@ -419,7 +472,6 @@ PageTableLibMapInLevel (
         // Update child entries to use restrictive attribute inherited from parent.
         // e.g.: Set PDE[0-255].ReadWrite = 0
         //
-        PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
         for (Index = 0; Index < 512; Index++) {
           if (PagingEntry[Index].Pce.Present == 0) {
             continue;
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100852): https://edk2.groups.io/g/devel/message/100852
Mute This Topic: https://groups.io/mt/97469476/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap
Posted by Ni, Ray 2 years, 9 months ago
> +**/
> +RETURN_STATUS
> +CheckMaskAndAttrForNotPresentEntry (
> +  IN     IA32_MAP_ATTRIBUTE  *Attribute,
> +  IN     IA32_MAP_ATTRIBUTE  *Mask
> +  )
> +{
> +  if ((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || (Mask-
> >Bits.ReadWrite == 0) ||

1. I think we can allow caller to set a not-present range as not-present.
Even though it's meaningless😊
So, I think we can remove the Attribute.Present check.


2. The function name can be more readable: how about IsAllAttributesSetForNonPresentEntry()?


> +      (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) ||
> (Mask->Bits.CacheDisabled == 0) ||
> +      (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask-
> >Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
> +      (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey
> == 0) || (Mask->Bits.Nx == 0))
> +  {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}

> +    PagingEntry           = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :

3. Can you add more comments for the code here?





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101194): https://edk2.groups.io/g/devel/message/101194
Mute This Topic: https://groups.io/mt/97469476/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 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap
Posted by duntan 2 years, 9 months ago
Thanks for the comments. Will do the code change in next version patch set.
Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 1:34 PM
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 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

> +**/
> +RETURN_STATUS
> +CheckMaskAndAttrForNotPresentEntry (
> +  IN     IA32_MAP_ATTRIBUTE  *Attribute,
> +  IN     IA32_MAP_ATTRIBUTE  *Mask
> +  )
> +{
> +  if ((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || (Mask-
> >Bits.ReadWrite == 0) ||

1. I think we can allow caller to set a not-present range as not-present.
Even though it's meaningless😊
So, I think we can remove the Attribute.Present check.


2. The function name can be more readable: how about IsAllAttributesSetForNonPresentEntry()?


> +      (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) ||
> (Mask->Bits.CacheDisabled == 0) ||
> +      (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask-
> >Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
> +      (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey
> == 0) || (Mask->Bits.Nx == 0))
> +  {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}

> +    PagingEntry           = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :

3. Can you add more comments for the code here?





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