UefiCpuPkg: CpuDxe: Set RW and P Attributes on Split Pages

A memory range can be submitted for attribute changes which is large
enough to not require a page split during the attribute update. Consider
the following scenario:

1. An attribute update removed the RW attribute on a range large enough
to not require a page split.
2. Later, an attributes update is called to re-add the RW attribute for
a subsection of that larger page which requires a split
3. The attribute update logic performs a page split, so now the parent
and child pages have matching attributes
4. Then, the attribute update logic changes the child page to have the
RW attribute.
5. The child page would then correctly have the RW attribute added but
the parent page would still have the RW attribute removed which will
cause an improper access violation.

The page being split should have loose attributes to accommodate the
above case. The split page should always have the attributes set so
the lowest level page frame determines the access rights as detailed
in 4.10.2.2 of the Intel 64 and IA-32 Architectures Software
Developer Manual. Setting the User/Supervisor attribute shouldn't
be necessary.

Cc: Eric Dong <eric.dong@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Taylor Beebe <t@taylorbeebe.com>

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index f7a4d92e921a083de141c95b482acbbe9b964192..5b1dec2d67603480d9019b26b60ffb0cf57eace9 100644 (file)
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -37,7 +37,9 @@
 #define IA32_PG_PMNT    BIT62\r
 #define IA32_PG_NX      BIT63\r
 \r
-#define PAGE_ATTRIBUTE_BITS  (IA32_PG_D | IA32_PG_A | IA32_PG_U | IA32_PG_RW | IA32_PG_P)\r
+#define PAGE_ATTRIBUTE_BITS             (IA32_PG_D | IA32_PG_A | IA32_PG_U | IA32_PG_RW | IA32_PG_P)\r
+#define PAGE_ATTRIBUTE_BITS_POST_SPLIT  (IA32_PG_RW | IA32_PG_P)\r
+\r
 //\r
 // Bits 1, 2, 5, 6 are reserved in the IA32 PAE PDPTE\r
 // X64 PAE PDPTE does not have such restriction\r
@@ -583,7 +585,7 @@ SplitPage (
         NewPageEntry[Index] = (BaseAddress + SIZE_4KB * Index) | AddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS);\r
       }\r
 \r
-      (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS);\r
+      (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS_POST_SPLIT;\r
       return RETURN_SUCCESS;\r
     } else {\r
       return RETURN_UNSUPPORTED;\r
@@ -606,7 +608,7 @@ SplitPage (
         NewPageEntry[Index] = (BaseAddress + SIZE_2MB * Index) | AddressEncMask | IA32_PG_PS | ((*PageEntry) & PAGE_PROGATE_BITS);\r
       }\r
 \r
-      (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS);\r
+      (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS_POST_SPLIT;\r
       return RETURN_SUCCESS;\r
     } else {\r
       return RETURN_UNSUPPORTED;\r