UefiCpuPkg: Add Ro to Ap Buffer after copying ApLoopCode. (#1356)

## Description

MpLib allocates a buffer for Ap Loop code. It manually removes the Nx
protection but never applies any other protections to the buffer,
leaving it RWX. After the code is copied into the allocated buffer,
apply Read Only permissions so that nothing can overwrite it while the
APs are executing from it.

In the update to 202502, Edk2 modified their code layout, resulting in
the AP buffer being allocated with Nx, having Nx removed, and then never
applying any other memory protections.


- [x] Impacts functionality?
- [x] Impacts security?
- [ ] Breaking change?
- [ ] Includes tests?
- [ ] Includes documentation?
- [x] Backport to release branch?

## How This Was Tested
Executing DxePagingAudit test application on Q35 found one region of
memory marked RWX.
Applying these modifications, DxePagingAudit test application no longer
reports a RWX buffer.

## Integration Instructions
No integration necessary.

Signed-off-by: Aaron Pop <[email protected]>
Co-authored-by: Oliver Smith-Denny <[email protected]>

Author: apop5

UefiCpuPkg/Library/MpInitLib/DxeMpLib.c

@@ -39,14 +39,12 @@ CPU_MP_DEBUG_PROTOCOL  mCpuMpDebugProtocol = {
 
 #define  AP_SAFE_STACK_SIZE  128
 
-CPU_MP_DATA       *mCpuMpData                  = NULL;
-EFI_EVENT         mCheckAllApsEvent            = NULL;
-EFI_EVENT         mMpInitExitBootServicesEvent = NULL;
-EFI_EVENT         mLegacyBootEvent             = NULL;
-volatile BOOLEAN  mStopCheckAllApsStatus       = TRUE;
-// MU_CHANGE START: Enable removal of NX attribute from buffer
+CPU_MP_DATA                    *mCpuMpData                  = NULL;
+EFI_EVENT                      mCheckAllApsEvent            = NULL;
+EFI_EVENT                      mMpInitExitBootServicesEvent = NULL;
+EFI_EVENT                      mLegacyBootEvent             = NULL;
+volatile BOOLEAN               mStopCheckAllApsStatus       = TRUE;
 extern RELOCATE_AP_LOOP_ENTRY  mReservedApLoop;
-// MU_CHANGE END: Enable removal of NX attribute from buffer
 
 //
 // Begin wakeup buffer allocation below 0x88000
@@ -110,57 +108,6 @@ InstallCpuMpDebugProtocol (
   DEBUG ((DEBUG_INFO, "Installed gCpuMpDebugProtocolGuid - Status: %r\n", Status));
 }
 
-// MU_CHANGE START: Enable removal of NX attribute from buffer
-
-/**
-  Remove NX attribute from Buffer and apply RO to Buffer
-
-  @param[in]  Buffer      Buffer whose attributes will be altered
-  @param[in]  Size        Size of the buffer
-
-  @retval EFI_SUCCESS             NX attribute removed, RO attribute applied
-  @retval EFI_INVALID_PARAMETER   Buffer is not page-aligned or Buffer is 0 or Size of buffer
-                                  is not page-aligned
-  @retval Other                   Return value of LocateProtocol, ClearMemoryAttributes, or SetMemoryAttributes
-**/
-EFI_STATUS
-BufferRemoveNoExecuteSetReadOnly (
-  IN EFI_PHYSICAL_ADDRESS  Buffer,
-  IN UINTN                 Size
-  )
-{
-  EFI_CPU_ARCH_PROTOCOL  *CpuProtocol = NULL;
-  EFI_STATUS             Status;
-
-  if ((Buffer == 0) || (Buffer % EFI_PAGE_SIZE != 0) || (Size % EFI_PAGE_SIZE != 0)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuProtocol);
-
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a - Unable to locate gEfiCpuArchProtocolGuid\n", __func__));
-    ASSERT_EFI_ERROR (Status);
-    return Status;
-  }
-
-  Status = CpuProtocol->SetMemoryAttributes (
-                          CpuProtocol,
-                          Buffer,
-                          Size,
-                          EFI_MEMORY_RO
-                          );
-
-  if EFI_ERROR (Status) {
-    DEBUG ((DEBUG_INFO, "%a - Unable to update buffer attributes!\n", __func__));
-    ASSERT_EFI_ERROR (Status);
-  }
-
-  return Status;
-}
-
-// MU_CHANGE END Enable removal of NX attribute from buffer
-
 /**
   Enable Debug Agent to support source debugging on AP function.
 
@@ -550,32 +497,70 @@ AllocateApLoopCodeBuffer (
 /**
   Remove Nx protection for the range specific by BaseAddress and Length.
 
-  The PEI implementation uses CpuPageTableLib to change the attribute.
-  The DXE implementation uses gDS to change the attribute.
+  @param[in] BaseAddress  BaseAddress of the range.
+  @param[in] Length       Length of the range.
+**/
+VOID
+RemoveNxProtection (
+  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN UINTN                 Length
+  )
+{
+  EFI_STATUS                       Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  MemDesc;
+
+  Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &MemDesc);
+  if (!EFI_ERROR (Status)) {
+    if (((MemDesc.Capabilities & EFI_MEMORY_XP) == EFI_MEMORY_XP) && ((MemDesc.Attributes & EFI_MEMORY_XP) == EFI_MEMORY_XP)) {
+      Status = gDS->SetMemorySpaceAttributes (
+                      BaseAddress,
+                      Length,
+                      MemDesc.Attributes & (~EFI_MEMORY_XP)
+                      );
+
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "%a - Setting Ro on 0x%p returned %r\n", __func__, BaseAddress, Status));
+        ASSERT_EFI_ERROR (Status);
+      }
+    } else {
+      DEBUG ((DEBUG_ERROR, "%a - Memory Address was not found in Memory map! %lp %r\n", __func__, BaseAddress, Status));
+      ASSERT_EFI_ERROR (Status);
+    }
+  }
+}
+
+/**
+  Add ReadOnly protection to the range specified by BaseAddress and Length.
 
   @param[in] BaseAddress  BaseAddress of the range.
   @param[in] Length       Length of the range.
 **/
 VOID
-RemoveNxprotection (
+ApplyReadOnlyMemoryProtection (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
   IN UINTN                 Length
   )
 {
   EFI_STATUS                       Status;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  MemDesc;
 
-  //
-  // TODO: Check EFI_MEMORY_XP bit set or not once it's available in DXE GCD
-  //       service.
-  //
   Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &MemDesc);
   if (!EFI_ERROR (Status)) {
-    gDS->SetMemorySpaceAttributes (
-           BaseAddress,
-           Length,
-           MemDesc.Attributes & (~EFI_MEMORY_XP)
-           );
+    if (((MemDesc.Capabilities & EFI_MEMORY_RO) == EFI_MEMORY_RO) && ((MemDesc.Attributes & EFI_MEMORY_RO) != EFI_MEMORY_RO)) {
+      Status = gDS->SetMemorySpaceAttributes (
+                      BaseAddress,
+                      Length,
+                      MemDesc.Attributes | EFI_MEMORY_RO
+                      );
+
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "%a - Setting Ro on 0x%p returned %r\n", __func__, BaseAddress, Status));
+        ASSERT_EFI_ERROR (Status);
+      }
+    } else {
+      DEBUG ((DEBUG_ERROR, "%a - Memory Address was not found in Memory map! %lp %r\n", __func__, BaseAddress, Status));
+      ASSERT_EFI_ERROR (Status);
+    }
   }
 }
 
@@ -601,12 +586,15 @@ MpInitChangeApLoopCallback (
   CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
   CpuMpData->ApLoopMode      = PcdGet8 (PcdCpuApLoopMode);
   mNumberToFinish            = CpuMpData->CpuCount - 1;
-  // MU_CHANGE END Enable removal of NX attribute from buffer
-  BufferRemoveNoExecuteSetReadOnly (
+  RemoveNxProtection (
+    (EFI_PHYSICAL_ADDRESS)(UINTN)mReservedApLoop.Data,
+    EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (CpuMpData->AddressMap.RelocateApLoopFuncSizeAmdSev))
+    );
+
+  ApplyReadOnlyMemoryProtection (
     (EFI_PHYSICAL_ADDRESS)(UINTN)mReservedApLoop.Data,
     EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (CpuMpData->AddressMap.RelocateApLoopFuncSizeAmdSev))
     );
-  // MU_CHANGE END Enable removal of NX attribute from buffer
   WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
   while (mNumberToFinish > 0) {
     CpuPause ();
@@ -691,7 +679,7 @@ InitMpGlobalData (
                       MemDesc.Attributes | EFI_MEMORY_RP
                       );
       ASSERT_EFI_ERROR (Status);
-      AppendCpuMpDebugProtocolEntry (StackBase, CpuMpData->CpuApStackSize, Index, FALSE); // MU_CHANGE
+      AppendCpuMpDebugProtocolEntry (StackBase, CpuMpData->CpuApStackSize, Index, FALSE);     // MU_CHANGE
       DEBUG ((
         DEBUG_INFO,
         "Stack Guard set at %lx [cpu%lu]!\n",
@@ -700,7 +688,7 @@ InitMpGlobalData (
         ));
     }
 
-    InstallCpuMpDebugProtocol (); // MU_CHANGE
+    InstallCpuMpDebugProtocol ();     // MU_CHANGE
   }
   // MU_CHANGE START: Add the Debug Protocol in the case that CpuStackGuard is not active
   else {

UefiCpuPkg/Library/MpInitLib/MpLib.c

@@ -2261,10 +2261,7 @@ MpInitLibInitialize (
     );
   DEBUG ((DEBUG_INFO, "AP Vector: non-16-bit = %p/%x\n", CpuMpData->WakeupBufferHigh, ApResetVectorSizeAbove1Mb));
 
-  // MU_CHANGE START: Enable removal of NX attribute from buffer
-  BufferRemoveNoExecuteSetReadOnly (CpuMpData->WakeupBufferHigh, ALIGN_VALUE (ApResetVectorSizeAbove1Mb, EFI_PAGE_SIZE));
-  // MU_CHANGE END: Enable removal of NX attribute from buffer
-
+  ApplyReadOnlyMemoryProtection ((EFI_PHYSICAL_ADDRESS)(UINTN)CpuMpData->WakeupBufferHigh, ALIGN_VALUE (ApResetVectorSizeAbove1Mb, EFI_PAGE_SIZE));
   //
   // Save APIC mode for AP to sync
   //
@@ -3492,14 +3489,15 @@ PrepareApLoopCode (
     // Make sure that the buffer memory is executable if NX protection is enabled
     // for EfiReservedMemoryType.
     //
-    RemoveNxprotection (Address, EFI_PAGES_TO_SIZE (FuncPages));
+    RemoveNxProtection (Address, EFI_PAGES_TO_SIZE (FuncPages));
   }
 
   mReservedTopOfApStack = (UINTN)Address + EFI_PAGES_TO_SIZE (StackPages+FuncPages);
   ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
   mReservedApLoop.Data = (VOID *)(UINTN)Address;
   ASSERT (mReservedApLoop.Data != NULL);
   CopyMem (mReservedApLoop.Data, ApLoopFunc, ApLoopFuncSize);
+  ApplyReadOnlyMemoryProtection (Address, EFI_PAGES_TO_SIZE (FuncPages));
   if (!CpuMpData->UseSevEsAPMethod) {
     //
     // processors without SEV-ES and paging is enabled

UefiCpuPkg/Library/MpInitLib/MpLib.h

@@ -985,36 +985,25 @@ AllocateApLoopCodeBuffer (
 /**
   Remove Nx protection for the range specific by BaseAddress and Length.
 
-  The PEI implementation uses CpuPageTableLib to change the attribute.
-  The DXE implementation uses gDS to change the attribute.
-
   @param[in] BaseAddress  BaseAddress of the range.
   @param[in] Length       Length of the range.
 **/
 VOID
-RemoveNxprotection (
+RemoveNxProtection (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
   IN UINTN                 Length
   );
 
-// MU_CHANGE START: Enable removal of NX attribute from buffer
-
 /**
-  Remove NX attribute from Buffer and apply RO to Buffer
+Add ReadOnly protection to the range specified by BaseAddress and Length.
 
-  @param[in]  Buffer      Buffer whose attributes will be altered
-  @param[in]  Size        Size of the buffer
-
-  @retval EFI_SUCCESS             NX attribute removed, RO attribute applied
-  @retval EFI_INVALID_PARAMETER   Buffer is not page-aligned or Buffer is 0 or Size of buffer
-                                  is not page-aligned
-  @retval Other                   Return value of LocateProtocol, ClearMemoryAttributes, or SetMemoryAttributes
+@param[in] BaseAddress  BaseAddress of the range.
+@param[in] Length       Length of the range.
 **/
-EFI_STATUS
-BufferRemoveNoExecuteSetReadOnly (
-  IN EFI_PHYSICAL_ADDRESS  Buffer,
-  IN UINTN                 Size
+VOID
+ApplyReadOnlyMemoryProtection (
+  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN UINTN                 Length
   );
 
-// MU_CHANGE END: Enable removal of NX attribute from buffer
 #endif

UefiCpuPkg/Library/MpInitLib/PeiMpLib.c

@@ -14,30 +14,6 @@
 
 STATIC UINT64  mSevEsPeiWakeupBuffer = BASE_1MB;
 
-// MU_CHANGE START: Enable removal of NX attribute from buffer
-
-/**
-  Remove NX attribute from Buffer and apply RO to Buffer
-
-  @param[in]  Buffer      Buffer whose attributes will be altered
-  @param[in]  Size        Size of the buffer
-
-  @retval EFI_SUCCESS             NX attribute removed, RO attribute applied
-  @retval EFI_INVALID_PARAMETER   Buffer is not page-aligned or Buffer is 0 or Size of buffer
-                                  is not page-aligned
-  @retval Other                   Return value of LocateProtocol, ClearMemoryAttributes, or SetMemoryAttributes
-**/
-EFI_STATUS
-BufferRemoveNoExecuteSetReadOnly (
-  IN EFI_PHYSICAL_ADDRESS  Buffer,
-  IN UINTN                 Size
-  )
-{
-  return EFI_SUCCESS;
-}
-
-// MU_CHANGE End: Enable removal of NX attribute from buffer
-
 /**
   Enable Debug Agent to support source debugging on AP function.
 
@@ -877,14 +853,11 @@ AllocateApLoopCodeBuffer (
 /**
   Remove Nx protection for the range specific by BaseAddress and Length.
 
-  The PEI implementation uses CpuPageTableLib to change the attribute.
-  The DXE implementation uses gDS to change the attribute.
-
   @param[in] BaseAddress  BaseAddress of the range.
   @param[in] Length       Length of the range.
 **/
 VOID
-RemoveNxprotection (
+RemoveNxProtection (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
   IN UINTN                 Length
   )
@@ -960,6 +933,22 @@ RemoveNxprotection (
   AsmWriteCr3 (PageTable);
 }
 
+/**
+  Add ReadOnly protection to the range specified by BaseAddress and Length.
+
+  @param[in] BaseAddress  BaseAddress of the range.
+  @param[in] Length       Length of the range.
+**/
+VOID
+ApplyReadOnlyMemoryProtection (
+  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN UINTN                 Length
+  )
+{
+  // stub for now, page tables are limited in PEI
+  return;
+}
+
 // MU_CHANGE START: Support for protocol for reporting multi-processor debug info
 
 /**