From c83db74426c78a9213e5085f71f697e096bd8450 Mon Sep 17 00:00:00 2001 From: Yeoreum Yun Date: Fri, 1 May 2026 09:33:09 +0100 Subject: [PATCH] ArmPkg, StandaloneMmPkg: optimise memory permission change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In StandaloneMm, the allowed memory permission combinations are ROX, RO, and RW. At the initial stage, all regions are mapped as ROX. When drivers are loaded for relocation, the image is first mapped as RW. However, the current mechanism introduces significant overhead when changing memory permissions. For example, when changing permissions from ROX to RW, the process is performed in two steps: 1. Clear the execute (X) permission (ROX → RO) 2. Add the write (W) permission (RO → RW) To execute step (1), the system first retrieves the permissions of the target region. This may trigger multiple SMC calls, depending on the permissions of the pages within that region. This permission retrieval operation is redundant, as there is no need to maintain an intermediate state. Nevertheless, it can cause additional SMC calls and coherence operations from the SPMC (e.g., TLB flushes). Therefore, this redundant operation should be removed, and in most cases, the memory permission should be updated with a single “set memory permission” operation. Note: The ArmxxxMemoryRegionxxx() interfaces e.g. ArmSetMemoryRegionNoExec(), etc. return success when the Length parameter is 0 as the SectionHeader.Misc.VirtualSize could be 0 Length like .reloc section: UpdateMmFoundationPeCoffPermissions: Section 2 of image at 0x7004000 has 0x42000040 permissions UpdateMmFoundationPeCoffPermissions: Section 2 of image at 0x7004000 has .reloc name api_ffa_mem_perm_set page_count 0 ERROR: FFA_MEM_PERM_SET: page_count was zero Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Yeoreum Yun --- .../Library/ArmStandaloneMmCoreEntryPoint.h | 2 - ArmPkg/Include/Library/StandaloneMmMmuLib.h | 32 ++- .../ArmStandaloneMmCoreEntryPoint.c | 8 +- .../SetPermissions.c | 5 +- .../ArmMmuStandaloneMmLib.c | 242 ++++++++++-------- .../StandaloneMmPeCoffExtraActionLib.c | 12 +- 6 files changed, 173 insertions(+), 128 deletions(-) diff --git a/ArmPkg/Include/Library/ArmStandaloneMmCoreEntryPoint.h b/ArmPkg/Include/Library/ArmStandaloneMmCoreEntryPoint.h index d08db8c1d9..2f99fa1b6e 100644 --- a/ArmPkg/Include/Library/ArmStandaloneMmCoreEntryPoint.h +++ b/ArmPkg/Include/Library/ArmStandaloneMmCoreEntryPoint.h @@ -89,7 +89,6 @@ typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) ( @param [in] ImageBase Base of image in memory @param [in] SectionHeaderOffset Offset of PE/COFF image section header @param [in] NumberOfSections Number of Sections - @param [in] TextUpdater Function to change code permissions @param [in] ReadOnlyUpdater Function to change RO permissions @param [in] ReadWriteUpdater Function to change RW permissions @@ -101,7 +100,6 @@ UpdateMmFoundationPeCoffPermissions ( IN EFI_PHYSICAL_ADDRESS ImageBase, IN UINT32 SectionHeaderOffset, IN CONST UINT16 NumberOfSections, - IN REGION_PERMISSION_UPDATE_FUNC TextUpdater, IN REGION_PERMISSION_UPDATE_FUNC ReadOnlyUpdater, IN REGION_PERMISSION_UPDATE_FUNC ReadWriteUpdater ); diff --git a/ArmPkg/Include/Library/StandaloneMmMmuLib.h b/ArmPkg/Include/Library/StandaloneMmMmuLib.h index 76f9e443a1..4c4f155ed8 100644 --- a/ArmPkg/Include/Library/StandaloneMmMmuLib.h +++ b/ArmPkg/Include/Library/StandaloneMmMmuLib.h @@ -57,7 +57,7 @@ ArmClearMemoryRegionNoExec ( ); /** - Set the memory to read-only while preserving execute permission. + Change memory permission as RO ignoring former permission. @param [in] BaseAddress Base address for the memory region. @param [in] Length Length of the memory region. @@ -75,13 +75,13 @@ ArmClearMemoryRegionNoExec ( **/ EFI_STATUS -ArmSetMemoryRegionReadOnly ( +ArmSetMemoryRegionReadOnlyPerm ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length ); /** - Set the memory to read-write while preserving execute permission. + Change memory permission as RW ignoring former permission. @param [in] BaseAddress Base address for the memory region. @param [in] Length Length of the memory region. @@ -99,7 +99,31 @@ ArmSetMemoryRegionReadOnly ( **/ EFI_STATUS -ArmClearMemoryRegionReadOnly ( +ArmSetMemoryRegionReadWritePerm ( + IN EFI_PHYSICAL_ADDRESS BaseAddress, + IN UINT64 Length + ); + +/** + Change memory permission as ROX ignoring former permission. + + @param [in] BaseAddress Base address for the memory region. + @param [in] Length Length of the memory region. + + @retval EFI_SUCCESS Request successfull. + @retval EFI_INVALID_PARAMETER A parameter is invalid. + @retval EFI_NOT_READY Callee is busy or not in a state to handle + this request. + @retval EFI_UNSUPPORTED This function is not implemented by the + callee. + @retval EFI_ABORTED Message target ran into an unexpected error + and has aborted. + @retval EFI_ACCESS_DENIED Access denied. + @retval EFI_OUT_OF_RESOURCES Out of memory to perform operation. + +**/ +EFI_STATUS +ArmSetMemoryRegionReadOnlyExecPerm ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length ); diff --git a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c index d5c786ba88..344cfe3904 100644 --- a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c +++ b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c @@ -1198,9 +1198,8 @@ CEntryPoint ( ImageBase, SectionHeaderOffset, NumberOfSections, - ArmSetMemoryRegionNoExec, - ArmSetMemoryRegionReadOnly, - ArmClearMemoryRegionReadOnly + ArmSetMemoryRegionReadOnlyPerm, + ArmSetMemoryRegionReadWritePerm ); if (EFI_ERROR (Status)) { goto finish; @@ -1208,8 +1207,7 @@ CEntryPoint ( if (ImageContext.ImageAddress != (UINTN)TeData) { ImageContext.ImageAddress = (UINTN)TeData; - ArmSetMemoryRegionNoExec (ImageBase, SIZE_4KB); - ArmClearMemoryRegionReadOnly (ImageBase, SIZE_4KB); + ArmSetMemoryRegionReadWritePerm (ImageBase, SIZE_4KB); Status = PeCoffLoaderRelocateImage (&ImageContext); ASSERT_EFI_ERROR (Status); diff --git a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/SetPermissions.c b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/SetPermissions.c index 8fa748488b..56bf1c11f4 100644 --- a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/SetPermissions.c +++ b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/SetPermissions.c @@ -34,7 +34,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent @param [in] ImageBase Base of image in memory @param [in] SectionHeaderOffset Offset of PE/COFF image section header @param [in] NumberOfSections Number of Sections - @param [in] TextUpdater Function to change code permissions @param [in] ReadOnlyUpdater Function to change RO permissions @param [in] ReadWriteUpdater Function to change RW permissions @@ -46,7 +45,6 @@ UpdateMmFoundationPeCoffPermissions ( IN EFI_PHYSICAL_ADDRESS ImageBase, IN UINT32 SectionHeaderOffset, IN CONST UINT16 NumberOfSections, - IN REGION_PERMISSION_UPDATE_FUNC TextUpdater, IN REGION_PERMISSION_UPDATE_FUNC ReadOnlyUpdater, IN REGION_PERMISSION_UPDATE_FUNC ReadWriteUpdater ) @@ -128,8 +126,6 @@ UpdateMmFoundationPeCoffPermissions ( if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) { Base = ImageBase + SectionHeader.VirtualAddress; - TextUpdater (Base, ALIGN_VALUE (SectionHeader.Misc.VirtualSize, SectionAlignment)); - if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_WRITE) != 0) { ReadWriteUpdater (Base, ALIGN_VALUE (SectionHeader.Misc.VirtualSize, SectionAlignment)); DEBUG (( @@ -140,6 +136,7 @@ UpdateMmFoundationPeCoffPermissions ( ImageContext->ImageAddress )); } else { + ReadOnlyUpdater (Base, ALIGN_VALUE (SectionHeader.Misc.VirtualSize, SectionAlignment)); DEBUG (( DEBUG_INFO, "%a: Mapping section %d of image at 0x%lx with RO-XN permissions\n", diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c index 8199110aa2..eefed6ddb1 100644 --- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c +++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c @@ -309,6 +309,14 @@ ArmSetMemoryRegionNoExec ( UINTN Size; UINT32 PageCount; + /* + * The .reloc section and others may be empty. + * In that case, return success silently. + */ + if (Length == 0) { + return EFI_SUCCESS; + } + UseFfaAbis = IsFfaMemoryAbiSupported (&Version); while (Length > 0) { @@ -391,6 +399,14 @@ ArmClearMemoryRegionNoExec ( UINTN Size; UINT32 PageCount; + /* + * The .reloc section and others may be empty. + * In that case, return success silently. + */ + if (Length == 0) { + return EFI_SUCCESS; + } + UseFfaAbis = IsFfaMemoryAbiSupported (&Version); while (Length > 0) { @@ -442,7 +458,7 @@ ArmClearMemoryRegionNoExec ( } /** - Set the memory to read-only while preserving execute permission. + Change memory permission as RO ignoring former permission. @param [in] BaseAddress Base address for the memory region. @param [in] Length Length of the memory region. @@ -460,71 +476,47 @@ ArmClearMemoryRegionNoExec ( **/ EFI_STATUS -ArmSetMemoryRegionReadOnly ( +ArmSetMemoryRegionReadOnlyPerm ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length ) { - EFI_STATUS Status; - UINT32 MemoryAttributes; - UINT32 PermissionRequest; - BOOLEAN UseFfaAbis; - UINT32 Version; - UINTN Size; - UINT32 PageCount; + UINT32 PermissionRequest; + BOOLEAN UseFfaAbis; - UseFfaAbis = IsFfaMemoryAbiSupported (&Version); + /* + * The .reloc section and others may be empty. + * In that case, return success silently. + */ + if (Length == 0) { + return EFI_SUCCESS; + } - while (Length > 0) { - Status = GetMemoryPermissions ( - UseFfaAbis, - Version, - BaseAddress, - Length, - &MemoryAttributes, - &PageCount - ); - if (EFI_ERROR (Status)) { - break; - } + UseFfaAbis = IsFfaMemoryAbiSupported (NULL); + Length = ALIGN_VALUE (Length, EFI_PAGE_SIZE); - Size = EFI_PAGES_TO_SIZE (PageCount); + if (UseFfaAbis) { + PermissionRequest = ARM_FFA_SET_MEM_ATTR_MAKE_PERM_REQUEST ( + ARM_FFA_SET_MEM_ATTR_DATA_PERM_RO, + ARM_FFA_SET_MEM_ATTR_CODE_PERM_XN + ); + } else { + PermissionRequest = ARM_SPM_MM_SET_MEM_ATTR_MAKE_PERM_REQUEST ( + ARM_SPM_MM_SET_MEM_ATTR_DATA_PERM_RO, + ARM_SPM_MM_SET_MEM_ATTR_CODE_PERM_XN + ); + } - if (UseFfaAbis) { - PermissionRequest = ARM_FFA_SET_MEM_ATTR_MAKE_PERM_REQUEST ( - ARM_FFA_SET_MEM_ATTR_DATA_PERM_RO, - (MemoryAttributes >> ARM_FFA_SET_MEM_ATTR_CODE_PERM_SHIFT) - ); - } else { - PermissionRequest = ARM_SPM_MM_SET_MEM_ATTR_MAKE_PERM_REQUEST ( - ARM_SPM_MM_SET_MEM_ATTR_DATA_PERM_RO, - (MemoryAttributes >> ARM_SPM_MM_SET_MEM_ATTR_CODE_PERM_SHIFT) - ); - } - - if (Length < Size) { - Length = Size; - } - - Status = RequestMemoryPermissionChange ( - UseFfaAbis, - BaseAddress, - Size, - PermissionRequest - ); - if (EFI_ERROR (Status)) { - return Status; - } - - Length -= Size; - BaseAddress += Size; - } // while - - return Status; + return RequestMemoryPermissionChange ( + UseFfaAbis, + BaseAddress, + Length, + PermissionRequest + ); } /** - Set the memory to read-write while preserving execute permission. + Change memory permission as RW ignoring former permission. @param [in] BaseAddress Base address for the memory region. @param [in] Length Length of the memory region. @@ -542,65 +534,99 @@ ArmSetMemoryRegionReadOnly ( **/ EFI_STATUS -ArmClearMemoryRegionReadOnly ( +ArmSetMemoryRegionReadWritePerm ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length ) { - EFI_STATUS Status; - UINT32 MemoryAttributes; - UINT32 PermissionRequest; - BOOLEAN UseFfaAbis; - UINT32 Version; - UINTN Size; - UINT32 PageCount; + UINT32 PermissionRequest; + BOOLEAN UseFfaAbis; - UseFfaAbis = IsFfaMemoryAbiSupported (&Version); + /* + * The .reloc section and others may be empty. + * In that case, return success silently. + */ + if (Length == 0) { + return EFI_SUCCESS; + } - while (Length > 0) { - Status = GetMemoryPermissions ( - UseFfaAbis, - Version, - BaseAddress, - Length, - &MemoryAttributes, - &PageCount - ); - if (EFI_ERROR (Status)) { - break; - } + UseFfaAbis = IsFfaMemoryAbiSupported (NULL); + Length = ALIGN_VALUE (Length, EFI_PAGE_SIZE); - Size = EFI_PAGES_TO_SIZE (PageCount); + if (UseFfaAbis) { + PermissionRequest = ARM_FFA_SET_MEM_ATTR_MAKE_PERM_REQUEST ( + ARM_FFA_SET_MEM_ATTR_DATA_PERM_RW, + ARM_FFA_SET_MEM_ATTR_CODE_PERM_XN + ); + } else { + PermissionRequest = ARM_SPM_MM_SET_MEM_ATTR_MAKE_PERM_REQUEST ( + ARM_SPM_MM_SET_MEM_ATTR_DATA_PERM_RW, + ARM_SPM_MM_SET_MEM_ATTR_CODE_PERM_XN + ); + } - if (UseFfaAbis) { - PermissionRequest = ARM_FFA_SET_MEM_ATTR_MAKE_PERM_REQUEST ( - ARM_FFA_SET_MEM_ATTR_DATA_PERM_RW, - (MemoryAttributes >> ARM_FFA_SET_MEM_ATTR_CODE_PERM_SHIFT) - ); - } else { - PermissionRequest = ARM_SPM_MM_SET_MEM_ATTR_MAKE_PERM_REQUEST ( - ARM_SPM_MM_SET_MEM_ATTR_DATA_PERM_RW, - (MemoryAttributes >> ARM_SPM_MM_SET_MEM_ATTR_CODE_PERM_SHIFT) - ); - } - - if (Length < Size) { - Length = Size; - } - - Status = RequestMemoryPermissionChange ( - UseFfaAbis, - BaseAddress, - Size, - PermissionRequest - ); - if (EFI_ERROR (Status)) { - return Status; - } - - Length -= Size; - BaseAddress += Size; - } // while - - return Status; + return RequestMemoryPermissionChange ( + UseFfaAbis, + BaseAddress, + Length, + PermissionRequest + ); +} + +/** + Change memory permission as ROX ignoring former permission. + + @param [in] BaseAddress Base address for the memory region. + @param [in] Length Length of the memory region. + + @retval EFI_SUCCESS Request successfull. + @retval EFI_INVALID_PARAMETER A parameter is invalid. + @retval EFI_NOT_READY Callee is busy or not in a state to handle + this request. + @retval EFI_UNSUPPORTED This function is not implemented by the + callee. + @retval EFI_ABORTED Message target ran into an unexpected error + and has aborted. + @retval EFI_ACCESS_DENIED Access denied. + @retval EFI_OUT_OF_RESOURCES Out of memory to perform operation. + +**/ +EFI_STATUS +ArmSetMemoryRegionReadOnlyExecPerm ( + IN EFI_PHYSICAL_ADDRESS BaseAddress, + IN UINT64 Length + ) +{ + UINT32 PermissionRequest; + BOOLEAN UseFfaAbis; + + /* + * The .reloc section and others may be empty. + * In that case, return success silently. + */ + if (Length == 0) { + return EFI_SUCCESS; + } + + UseFfaAbis = IsFfaMemoryAbiSupported (NULL); + Length = ALIGN_VALUE (Length, EFI_PAGE_SIZE); + + if (UseFfaAbis) { + PermissionRequest = ARM_FFA_SET_MEM_ATTR_MAKE_PERM_REQUEST ( + ARM_FFA_SET_MEM_ATTR_DATA_PERM_RO, + ARM_FFA_SET_MEM_ATTR_CODE_PERM_X + ); + } else { + PermissionRequest = ARM_SPM_MM_SET_MEM_ATTR_MAKE_PERM_REQUEST ( + ARM_SPM_MM_SET_MEM_ATTR_DATA_PERM_RO, + ARM_SPM_MM_SET_MEM_ATTR_CODE_PERM_X + ); + } + + return RequestMemoryPermissionChange ( + UseFfaAbis, + BaseAddress, + Length, + PermissionRequest + ); } diff --git a/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c index 70ba26f78f..efe4621248 100644 --- a/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c +++ b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c @@ -28,7 +28,8 @@ RETURN_STATUS UpdatePeCoffPermissions ( IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext, IN REGION_PERMISSION_UPDATE_FUNC NoExecUpdater, - IN REGION_PERMISSION_UPDATE_FUNC ReadOnlyUpdater + IN REGION_PERMISSION_UPDATE_FUNC ReadOnlyUpdater, + IN REGION_PERMISSION_UPDATE_FUNC ReadOnlyExecUpdater ) { RETURN_STATUS Status; @@ -205,8 +206,7 @@ UpdatePeCoffPermissions ( Base, SectionHeader.Misc.VirtualSize )); - ReadOnlyUpdater (Base, ALIGN_VALUE (SectionHeader.Misc.VirtualSize, SectionAlignment)); - NoExecUpdater (Base, ALIGN_VALUE (SectionHeader.Misc.VirtualSize, SectionAlignment)); + ReadOnlyExecUpdater (Base, ALIGN_VALUE (SectionHeader.Misc.VirtualSize, SectionAlignment)); } SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER); @@ -233,7 +233,8 @@ PeCoffLoaderRelocateImageExtraAction ( UpdatePeCoffPermissions ( ImageContext, ArmClearMemoryRegionNoExec, - ArmSetMemoryRegionReadOnly + ArmSetMemoryRegionReadOnlyPerm, + ArmSetMemoryRegionReadOnlyExecPerm ); } @@ -256,6 +257,7 @@ PeCoffLoaderUnloadImageExtraAction ( UpdatePeCoffPermissions ( ImageContext, ArmSetMemoryRegionNoExec, - ArmClearMemoryRegionReadOnly + ArmSetMemoryRegionReadWritePerm, + ArmSetMemoryRegionReadWritePerm ); }