[edk2-devel] [Patch V3 07/18] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

duntan posted 18 patches 2 years, 9 months ago
There is a newer version of this series
[edk2-devel] [Patch V3 07/18] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
Posted by duntan 2 years, 9 months ago
For different usage, check if the combination for Mask and
Attr is valid when creating or updating page table.

1.For non-present range
  1.1Mask.Present is 0 but some other attributes is provided.
     This case is invalid.
  1.2Mask.Present is 1 and Attr.Present is 0. In this case,all
     other attributes should not be provided.
  1.3Mask.Present is 1 and Attr.Present is 1. In this case,all
     attributes should be provided to intialize the attribute.

2.For present range
  2.1Mask.Present is 1 and Attr.Present is 0.In this case, all
     other attributes should not be provided.
All other usage for present range is permitted.
In the mentioned cases, 1.2 and 2.1 can be merged into 1 check.

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/Include/Library/CpuPageTableLib.h         |  2 +-
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 5f44ece548..6bda15b5bc 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -76,7 +76,7 @@ typedef enum {
   @param[in]      Mask           The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
 
   @retval RETURN_UNSUPPORTED        PagingMode is not supported.
-  @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL.
+  @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL or the combination of Attribute and Mask is invalid.
   @retval RETURN_INVALID_PARAMETER  *BufferSize is not multiple of 4KB.
   @retval RETURN_BUFFER_TOO_SMALL   The buffer is too small for page table creation/updating.
                                     BufferSize is updated to indicate the expected buffer size.
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 1fc696f572..925dd90f23 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -215,6 +215,43 @@ PageTableLibSetPnle (
   Pnle->Bits.CacheDisabled = 0;
 }
 
+/**
+  Check if the combination for Attribute and Mask is valid for non-present entry.
+  1.Mask.Present is 0 but some other attributes is provided. This case should be invalid.
+  2.Map non-present range to present. In this case, all attributes should be provided.
+
+  @param[in] Attribute    The attribute of the linear address range.
+  @param[in] Mask         The mask used for attribute to check.
+
+  @retval RETURN_INVALID_PARAMETER    The combination for Attribute and Mask is invalid.
+  @retval RETURN_SUCCESS              The combination for Attribute and Mask is valid.
+**/
+RETURN_STATUS
+IsAttributesAndMaskValidForNonPresentEntry (
+  IN     IA32_MAP_ATTRIBUTE  *Attribute,
+  IN     IA32_MAP_ATTRIBUTE  *Mask
+  )
+{
+  if ((Mask->Bits.Present == 1) && (Attribute->Bits.Present == 1)) {
+    //
+    // Creating new page table or remapping non-present range to present.
+    //
+    if ((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;
+    }
+  } else if ((Mask->Bits.Present == 0) && (Mask->Uint64 > 1)) {
+    //
+    // Only change other attributes for non-present range is not permitted.
+    //
+    return RETURN_INVALID_PARAMETER;
+  }
+
+  return RETURN_SUCCESS;
+}
+
 /**
   Update page table to map [LinearAddress, LinearAddress + Length) with specified attribute in the specified level.
 
@@ -237,6 +274,7 @@ PageTableLibSetPnle (
                                     when a new physical base address is set.
   @param[in]      Mask              The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
 
+  @retval RETURN_INVALID_PARAMETER  The combination of Attribute and Mask for non-present entry is invalid.
   @retval RETURN_SUCCESS            PageTable is created/updated successfully.
 **/
 RETURN_STATUS
@@ -260,6 +298,7 @@ PageTableLibMapInLevel (
   UINTN               Index;
   IA32_PAGING_ENTRY   *PagingEntry;
   UINTN               PagingEntryIndex;
+  UINTN               PagingEntryIndexEnd;
   IA32_PAGING_ENTRY   *CurrentPagingEntry;
   UINT64              RegionLength;
   UINT64              SubLength;
@@ -303,6 +342,14 @@ PageTableLibMapInLevel (
   //
 
   if (ParentPagingEntry->Pce.Present == 0) {
+    //
+    // [LinearAddress, LinearAddress + Length] contains non-present range.
+    //
+    Status = IsAttributesAndMaskValidForNonPresentEntry (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 +419,27 @@ PageTableLibMapInLevel (
       ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
     }
   } else {
+    //
+    // If (LinearAddress + Length - 1) is not in the same ParentPagingEntry with (LinearAddress + Offset), then the remaining child PagingEntry
+    // starting from PagingEntryIndex of ParentPagingEntry is all covered by [LinearAddress + Offset, LinearAddress + Length - 1].
+    //
+    PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length - 1, BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ? 511 :
+                          (UINTN)BitFieldRead64 (LinearAddress + Length - 1, BitStart, BitStart + 9 - 1);
+    PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+    for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
+      if (PagingEntry[Index].Pce.Present == 0) {
+        //
+        // [LinearAddress, LinearAddress + Length] contains non-present range.
+        //
+        Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
+        if (RETURN_ERROR (Status)) {
+          return Status;
+        }
+
+        break;
+      }
+    }
+
     //
     // It's a non-leaf entry
     //
@@ -419,7 +487,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;
@@ -547,7 +614,7 @@ PageTableLibMapInLevel (
   @param[in]      Mask           The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
 
   @retval RETURN_UNSUPPORTED        PagingMode is not supported.
-  @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL.
+  @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL or the combination of Attribute and Mask is invalid.
   @retval RETURN_INVALID_PARAMETER  *BufferSize is not multiple of 4KB.
   @retval RETURN_BUFFER_TOO_SMALL   The buffer is too small for page table creation/updating.
                                     BufferSize is updated to indicate the expected buffer size.
@@ -609,6 +676,14 @@ PageTableMap (
     return RETURN_INVALID_PARAMETER;
   }
 
+  //
+  // If to map [LinearAddress, LinearAddress + Length] as non-present,
+  // all attributes except Present should not be provided.
+  //
+  if ((Attribute->Bits.Present == 0) && (Mask->Bits.Present == 1) && (Mask->Uint64 > 1)) {
+    return RETURN_INVALID_PARAMETER;
+  }
+
   MaxLeafLevel     = (IA32_PAGE_LEVEL)(UINT8)PagingMode;
   MaxLevel         = (IA32_PAGE_LEVEL)(UINT8)(PagingMode >> 8);
   MaxLinearAddress = LShiftU64 (1, 12 + MaxLevel * 9);
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101387): https://edk2.groups.io/g/devel/message/101387
Mute This Topic: https://groups.io/mt/97725704/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V3 07/18] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
Posted by Gerd Hoffmann 2 years, 9 months ago
On Mon, Mar 20, 2023 at 01:33:18PM +0800, Dun Tan wrote:
> For different usage, check if the combination for Mask and
> Attr is valid when creating or updating page table.
> 
> 1.For non-present range
>   1.1Mask.Present is 0 but some other attributes is provided.
>      This case is invalid.
>   1.2Mask.Present is 1 and Attr.Present is 0. In this case,all
>      other attributes should not be provided.
>   1.3Mask.Present is 1 and Attr.Present is 1. In this case,all
>      attributes should be provided to intialize the attribute.
> 
> 2.For present range
>   2.1Mask.Present is 1 and Attr.Present is 0.In this case, all
>      other attributes should not be provided.
> All other usage for present range is permitted.
> In the mentioned cases, 1.2 and 2.1 can be merged into 1 check.

This patch breaks OVMF.  Log tail:

AP Loop Mode is 1
AP Vector: non-16-bit = 3EFCE000/447
GetMicrocodePatchInfoFromHob: MicrocodeBase = 0x0, MicrocodeSize = 0x0
WakeupBufferStart = 87000, WakeupBufferSize = DD
AP Vector: 16-bit = 87000/39, ExchangeInfo = 87039/A4
CpuDxe: 5-Level Paging = 0
CPU[0000]: Microcode revision = 00000000, expected = 00000000
CPU[0001]: Microcode revision = 00000000, expected = 00000000
ASSERT /home/kraxel/projects/edk2/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c(65): Status == ((RETURN_STATUS)(0x8000000000000000ULL | (5)))

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101431): https://edk2.groups.io/g/devel/message/101431
Mute This Topic: https://groups.io/mt/97725704/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V3 07/18] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
Posted by duntan 2 years, 9 months ago
Gred, 
Thanks for the test. I found the root cause and will fix the issue in next version patch.

Thanks,
Dun

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Monday, March 20, 2023 7:43 PM
To: Tan, Dun <dun.tan@intel.com>
Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: Re: [Patch V3 07/18] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr

On Mon, Mar 20, 2023 at 01:33:18PM +0800, Dun Tan wrote:
> For different usage, check if the combination for Mask and Attr is 
> valid when creating or updating page table.
> 
> 1.For non-present range
>   1.1Mask.Present is 0 but some other attributes is provided.
>      This case is invalid.
>   1.2Mask.Present is 1 and Attr.Present is 0. In this case,all
>      other attributes should not be provided.
>   1.3Mask.Present is 1 and Attr.Present is 1. In this case,all
>      attributes should be provided to intialize the attribute.
> 
> 2.For present range
>   2.1Mask.Present is 1 and Attr.Present is 0.In this case, all
>      other attributes should not be provided.
> All other usage for present range is permitted.
> In the mentioned cases, 1.2 and 2.1 can be merged into 1 check.

This patch breaks OVMF.  Log tail:

AP Loop Mode is 1
AP Vector: non-16-bit = 3EFCE000/447
GetMicrocodePatchInfoFromHob: MicrocodeBase = 0x0, MicrocodeSize = 0x0 WakeupBufferStart = 87000, WakeupBufferSize = DD AP Vector: 16-bit = 87000/39, ExchangeInfo = 87039/A4
CpuDxe: 5-Level Paging = 0
CPU[0000]: Microcode revision = 00000000, expected = 00000000
CPU[0001]: Microcode revision = 00000000, expected = 00000000 ASSERT /home/kraxel/projects/edk2/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c(65): Status == ((RETURN_STATUS)(0x8000000000000000ULL | (5)))

take care,
  Gerd



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