From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout.perfora.net (mout.perfora.net [74.208.4.194]) by mx.groups.io with SMTP id smtpd.web10.6003.1679005279881420280 for ; Thu, 16 Mar 2023 15:21:20 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: smith-denny.com, ip: 74.208.4.194, mailfrom: osd@smith-denny.com) Received: from [10.137.194.171] ([131.107.8.107]) by mrelay.perfora.net (mreueus002 [74.208.5.2]) with ESMTPSA (Nemesis) id 0MKXsv-1pdCg13Kid-001wic; Thu, 16 Mar 2023 23:20:34 +0100 Message-ID: <7dae7e51-a7d7-d49a-0d37-732b1518431e@smith-denny.com> Date: Thu, 16 Mar 2023 15:20:31 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [edk2-devel] [PATCH v5 18/38] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch To: devel@edk2.groups.io, ardb@kernel.org Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-19-ardb@kernel.org> From: osd@smith-denny.com In-Reply-To: <20230313171714.3866151-19-ardb@kernel.org> X-Provags-ID: V03:K1:Fh97zhTW/s3ztZA7ET3dThJ7+77C4+CAFfvGGfxsuGzFmZxvSzx gFFvSsSqeB3Lce+IKqEMPzXJo3zIQVGDqnAmCXz3szEAKnPKMM7vwVaNGky24yCPU9aM/Ut fNwZbRN0m1vXE5TUXXCm9qMHBVd+1lu1BZEB5gbjOKdSHz02qwCGv/P4ShydpYgSWkkWmQB Yo1/cbXdZp893j/nj5Okg== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:vmdm5skeUaQ=;TnZYUQv1OS+Ha1oAhJ9OLBAeKV4 q0Ldwf9cwlvHCch0QTVD5LgqN/EFJd9Bvvd0caVsY8HNxeyRhgpWq1+eIxt6VZLvjFPeuFhPO 5/kZf4A7kfnzdmVX4ozPP8vHWarLhrgBz94jrEcyf548IUQlT39uBInrSjeEk4S+6NVLysY1f p7q3aASLS5JF1f9CmHJG6jCn9hSX91vaQylawjvPMDNXotZKpuu7404GQbudmjn/mhMvdhy7x rHvz1BbjRI5vvrdmxPuIL+mJvndAe+f5xxczvQi3YmLFai5cIKzXwDERTGBK6/UtVsLWlfuro oSIiSfRaVnozAlCP5mEPL5EzMvq3EnDk5G2Wc4ZVqa6g8E2OuKJXtrkh8kKwCAfTW66q/SuJH xiYbzyHb9nfjRSCfsM8k9Lxg1CYFPVl6GgmJj++CY/j+b6CfnSRpj0tb4kDWnUyt1a7JJz/Kl G/87mc9EdnVjUApiBWLvzBNlab+gcoo5EY6q3+O12UUtPu/sGmV619QaNr6dntcUz8JYWbeJc tWjrtoL3qgXVQmAoWwCPTXw8Qr9618DsIuGES4pd3Ye3myOABXIZJ13dZO7zQtOO6fG451Jec 1Hwn3cohVulQBLLm10hX7AoRuBhYasHLK4MlRjEkULbsqkNGCaMMY4T75ezxUfXp2kzJ4ehd/ YJlm0kwukiH+G9YKQfHjfsUs6HXBXLwd6IU8hoVbLw== Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 3/13/2023 10:16 AM, Ard Biesheuvel wrote: One nitpick below. > To permit the platform to adopt a stricter policy when it comes to > memory protections, and map all memory XP by default, add the necessary > handling to the DXE IPL PEIM to ensure that the DXE core code section is > mapped executable before invoking the DXE core. > > It is up to the DXE core itself to manage the executable permissions on > other DXE and UEFI drivers and applications that it dispatches. > > Note that this requires that the DXE IPL executes non-shadowed from a FV > that is mapped executable. > > Signed-off-by: Ard Biesheuvel > --- > MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 73 ++++++++++++++++++++ > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 1 + > 2 files changed, 74 insertions(+) > > diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c > index f62b6dcb38a7..c57ffa87e30f 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c > +++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c > @@ -11,6 +11,73 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "DxeIpl.h" > > > > #include > > +#include > > + > > +/** > > + Discover the code sections of the DXE core, and remap them read-only > > + and executable. > > + > > + @param DxeCoreEntryPoint The entrypoint of the DXE core executable. > > + @param HobList The list of HOBs passed to the DXE core from PEI. > > +**/ > > +STATIC > > +VOID > > +RemapDxeCoreCodeReadOnly ( nit: should this be called RemapDxeCoreCodeReadExecute or something? The naming seems confusing here to say map read only when we are mapping it read and execute. > > + IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint, > > + IN EFI_PEI_HOB_POINTERS HobList > > + ) > > +{ > > + EFI_PEI_HOB_POINTERS Hob; > > + PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; > > + RETURN_STATUS Status; > > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr; > > + EFI_IMAGE_SECTION_HEADER *Section; > > + UINTN Index; > > + > > + ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory; > > + ImageContext.Handle = NULL; > > + > > + // > > + // Find the module HOB for the DXE core > > + // > > + for (Hob.Raw = HobList.Raw; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) { > > + if ((GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_MEMORY_ALLOCATION) && > > + (CompareGuid (&Hob.MemoryAllocation->AllocDescriptor.Name, &gEfiHobMemoryAllocModuleGuid)) && > > + (Hob.MemoryAllocationModule->EntryPoint == DxeCoreEntryPoint)) > > + { > > + ImageContext.Handle = (VOID *)(UINTN)Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress; > > + break; > > + } > > + } > > + > > + ASSERT (ImageContext.Handle != NULL); > > + > > + Status = PeCoffLoaderGetImageInfo (&ImageContext); > > + ASSERT_RETURN_ERROR (Status); > > + > > + Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext.Handle + > > + ImageContext.PeCoffHeaderOffset); > > + ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE); > > + > > + Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) + > > + sizeof (EFI_IMAGE_FILE_HEADER) + > > + Hdr.Pe32->FileHeader.SizeOfOptionalHeader > > + ); > > + > > + for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) { > > + if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) { > > + ArmSetMemoryRegionReadOnly ( > > + (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress), > > + Section[Index].Misc.VirtualSize > > + ); > > + > > + ArmClearMemoryRegionNoExec ( > > + (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress), > > + Section[Index].Misc.VirtualSize > > + ); > > + } > > + } > > +} > > > > /** > > Transfers control to DxeCore. > > @@ -33,6 +100,12 @@ HandOffToDxeCore ( > VOID *TopOfStack; > > EFI_STATUS Status; > > > > + // > > + // DRAM may be mapped with non-executable permissions by default, so > > + // we'll need to map the DXE core code region executable explicitly. > > + // > > + RemapDxeCoreCodeReadOnly (DxeCoreEntryPoint, HobList); > > + > > // > > // Allocate 128KB for the Stack > > // > > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > index 62821477d012..d85ca79dc0c3 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > @@ -82,6 +82,7 @@ [LibraryClasses] > > > [LibraryClasses.ARM, LibraryClasses.AARCH64] > > ArmMmuLib > > + PeCoffLib > > > > [Ppis] > > gEfiDxeIplPpiGuid ## PRODUCES >