From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.18028.1685035295287232246 for ; Thu, 25 May 2023 10:21:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=OR4wldQo; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id B98EF20FBE8C; Thu, 25 May 2023 10:21:34 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com B98EF20FBE8C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1685035294; bh=gGuchtPS6OMniefS15dG57n1B5WRMMom9fqe0+oFkwI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=OR4wldQom050de6PxW7sbreRGYmWVGhxheB+/dnI0PHTMDQE3cfMEUK4w9JeyYmGN lqux/pPTefJhmRTUMbqfI3HW8/xU21O7Ah0hsVWGtFaJJ0fzR5uyPG8Pvg0rGvuE1M gZSlJ74b19kDNnNi/yCMblenq/P2UDPRNRogBZ/U= Message-ID: Date: Thu, 25 May 2023 10:21:34 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader To: devel@edk2.groups.io, ardb@kernel.org Cc: Ray Ni , Jiewen Yao , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , Dandan Bi , Liming Gao , "Kinney, Michael D" , Leif Lindholm , Sunil V L , Andrei Warkentin References: <20230525143041.1172989-1-ardb@kernel.org> <20230525143041.1172989-8-ardb@kernel.org> From: "Oliver Smith-Denny" In-Reply-To: <20230525143041.1172989-8-ardb@kernel.org> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 5/25/2023 7:30 AM, Ard Biesheuvel wrote: > Add a notification callback to the PEI core to grab a reference to the > memory attributes PPI as soon as it is registered, and use it in the > image loader to set restricted memory permissions after loading the > image if the image was loaded into memory. > > There are two use cases for this: > - when the DXE IPL loads the DXE core using the PEI image services, its > mappings will be set according to the PE section permission attributes > if the image was built with 4k section alignment; this means DXE core > will never run with mappings that are both writable and executable. > - when PEIMs are shadowed to memory, they will not only be mapped > read-only, but any non-exec permissions will also be removed. (Note > that this requires the component that installs PEI permanent memory to > depex on the memory attributes PPI, to ensure that it is available to > manage permissions on permanent memory before it is used to load > images) > > With this logic in place *, there is no longer a need for system memory > to be mapped with both write and execute permissions out of reset. > Instead, all memory can be mapped with non-executable permissions by > default, which means that the stack and other allocations used in PEI or > early in DXE will no longer need to be mapped non-exec explicitly. > > * the DXE core will also need its own method for clearing non-exec > permissions on memory ranges, but this is being addressed in a > separate series. > > Signed-off-by: Ard Biesheuvel > --- > MdeModulePkg/Core/Pei/Image/Image.c | 160 ++++++++++++++++++++ > MdeModulePkg/Core/Pei/PeiMain.h | 6 + > MdeModulePkg/Core/Pei/PeiMain.inf | 1 + > 3 files changed, 167 insertions(+) > > diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c > index cee9f09c6ea61e31..3a7de45014b8f772 100644 > --- a/MdeModulePkg/Core/Pei/Image/Image.c > +++ b/MdeModulePkg/Core/Pei/Image/Image.c > @@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = { > &mPeiLoadImagePpi > > }; > > > > +/** > > + Provide a callback for when the memory attributes PPI is installed. > > + > > + @param PeiServices An indirect pointer to the EFI_PEI_SERVICES table > > + published by the PEI Foundation. > > + @param NotifyDescriptor The descriptor for the notification event. > > + @param Ppi Pointer to the PPI in question. > > + > > + @return Always success > > + > > +**/ > > +STATIC > > +EFI_STATUS > > +EFIAPI > > +MemoryAttributePpiNotifyCallback ( > > + IN EFI_PEI_SERVICES **PeiServices, > > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > > + IN VOID *Ppi > > + ) > > +{ > > + PEI_CORE_INSTANCE *PrivateData; > > + > > + // > > + // Get PEI Core private data > > + // > > + PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices); > > + > > + // > > + // If there isn't a memory attribute PPI installed, use the one from > > + // notification > > + // > > + if (PrivateData->MemoryAttributePpi == NULL) { > > + PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi; > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = { > > + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > > + &gEdkiiMemoryAttributePpiGuid, > > + MemoryAttributePpiNotifyCallback > > +}; > > + > > /** > > > > Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file. > > @@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress ( > return Status; > > } > > > > +/** > > + Remap the memory region covering a loaded image so it can be executed. > > + > > + @param ImageContext Pointer to the image context structure that describes the > > + PE/COFF image that needs to be examined by this function. > > + @param FileType The FFS file type of the image > > + @param ImageAddress The start of the memory region covering the image > > + @param ImageSize The size of the memory region covering the image > > + > > + @retval EFI_SUCCESS The image is ready to be executed > > + @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the memory > > + mapping > > + > > +**/ > > +STATIC > > +EFI_STATUS > > +RemapLoadedImageForExecution ( > > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext, > > + IN EFI_FV_FILETYPE FileType, > > + IN PHYSICAL_ADDRESS ImageAddress, > > + IN UINT64 ImageSize > > + ) > > +{ > > + PEI_CORE_INSTANCE *Private; > > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr; > > + EFI_IMAGE_SECTION_HEADER *Section; > > + PHYSICAL_ADDRESS SectionAddress; > > + EFI_STATUS Status; > > + UINT64 Permissions; > > + UINTN Index; > > + > > + Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ()); > > + > > + if (Private->MemoryAttributePpi == NULL) { > > + return EFI_SUCCESS; We will have a gap here before the MemoryAttributePpi is installed, obviously, when CpuPei is installed. Is the expectation that only dependencies for CpuPei will be launched before and everything else will have a dependency on CpuPei? Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious how big or small of a gap this really is. > > + } > > + > > + // > > + // PEI phase executables must be able to execute in place from read-only NOR > > + // flash, and so they can be mapped read-only in their entirety. > > + // > > + if ((FileType == EFI_FV_FILETYPE_PEI_CORE) || > > + (FileType == EFI_FV_FILETYPE_PEIM) || > > + (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER)) > > + { We are calling this from PEI Core, will we have more images of type PEI Core? I understand if this is for completeness, but just making sure I understand the flow. Thanks, Oliver > > + return Private->MemoryAttributePpi->SetPermissions ( > > + Private->MemoryAttributePpi, > > + ImageAddress, > > + ImageSize, > > + EFI_MEMORY_RO, > > + EFI_MEMORY_XP > > + ); > > + } > > + > > + // > > + // Only PE images with minimum 4k section alignment can be remapped with > > + // restricted permissions. > > + // > > + if (ImageContext->IsTeImage || > > + (ImageContext->SectionAlignment < EFI_PAGE_SIZE)) > > + { > > + return EFI_UNSUPPORTED; > > + } > > + > > + 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++) { > > + SectionAddress = ImageContext->ImageAddress + Section[Index].VirtualAddress; > > + Permissions = 0; > > + > > + if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) { > > + Permissions |= EFI_MEMORY_RO; > > + } > > + > > + if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) { > > + Permissions |= EFI_MEMORY_XP; > > + } > > + > > + Status = Private->MemoryAttributePpi->SetPermissions ( > > + Private->MemoryAttributePpi, > > + SectionAddress, > > + Section[Index].Misc.VirtualSize, > > + Permissions, > > + Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP > > + ); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > /** > > > > Loads and relocates a PE/COFF image into memory. > > @@ -456,9 +600,24 @@ LoadAndRelocatePeCoffImage ( > > > // > > // Flush the instruction cache so the image data is written before we execute it > > + // Also ensure that the pages are mapped for execution > > // > > if (ImageContext.ImageAddress != (EFI_PHYSICAL_ADDRESS)(UINTN)Pe32Data) { > > InvalidateInstructionCacheRange ((VOID *)(UINTN)ImageContext.ImageAddress, (UINTN)ImageContext.ImageSize); > > + > > + Status = RemapLoadedImageForExecution ( > > + &ImageContext, > > + FileInfo.FileType, > > + ImageContext.ImageAddress & ~(UINT64)EFI_PAGE_MASK, > > + ALIGN_VALUE ( > > + AlignImageSize + (ImageContext.ImageAddress & EFI_PAGE_MASK), > > + EFI_PAGE_SIZE > > + ) > > + ); > > + if (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + return Status; > > + } > > } > > > > *ImageAddress = ImageContext.ImageAddress; > > @@ -972,6 +1131,7 @@ InitializeImageServices ( > // > > PrivateData->XipLoadFile = &gPpiLoadFilePpiList; > > PeiServicesInstallPpi (PrivateData->XipLoadFile); > > + PeiServicesNotifyPpi (&mNotifyList); > > } else { > > // > > // 2nd time we are running from memory so replace the XIP version with the > > diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h > index 556beddad533989f..5499d53b0bbaf641 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.h > +++ b/MdeModulePkg/Core/Pei/PeiMain.h > @@ -26,6 +26,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -302,6 +303,11 @@ struct _PEI_CORE_INSTANCE { > // > > EFI_GUID *TempFileGuid; > > > > + // > > + // Pointer to the memory attribute PPI > > + // > > + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryAttributePpi; > > + > > // > > // Temp Memory Range is not covered by PeiTempMem and Stack. > > // Those Memory Range will be migrated into physical memory. > > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf > index 0cf357371a16d872..55d8eb3e862d6418 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.inf > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf > @@ -100,6 +100,7 @@ [Ppis] > gEfiPeiReset2PpiGuid ## SOMETIMES_CONSUMES > > gEfiSecHobDataPpiGuid ## SOMETIMES_CONSUMES > > gEfiPeiCoreFvLocationPpiGuid ## SOMETIMES_CONSUMES > > + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES > > > > [Pcd] > > gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeiStackSize ## CONSUMES >