From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 8CB677803D1 for ; Fri, 29 Mar 2024 20:21:29 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=DXSiGFokrslunimpu98I6/PvmLv0Ha3L2mwAQJcYmUo=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Transfer-Encoding; s=20240206; t=1711743688; v=1; b=QqIjRZ1C4vYcl6NkRlNAQTAaFQsufEhp94ZNUZBwCB5wlt1edhffFvTRDXmyirjuO+3TszFQ ELc8etxd+UG1Uh9BFxhqZ9BndkFZ0KDZCI88MQkBBhmk5LaWYJvIZlmWylcTz7HSvpSggALHa9i bNCN3BW6Cjb1xn6yllmgN9MFE2tQHTM6WJyAP1cajXZSNAbNS82Yji5NlICFycNgWDs4Sui0bG7 3oFE98+TYECqbX3gKg6AdJCRDjItoJB41/0ngGYZ1CYR2d7SmCZeQZkDw4+2gKjrYjrYdOZ4PlG BY1ZiWc6BXN6UbtFfXlu8Tbg7JZB5XGUmvFVskogn0vbA== X-Received: by 127.0.0.2 with SMTP id jTisYY7687511x3ly2uqGbDK; Fri, 29 Mar 2024 13:21:28 -0700 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.1033.1711743685734266798 for ; Fri, 29 Mar 2024 13:21:25 -0700 X-Received: from OSD-Desktop.redmond.corp.microsoft.com (unknown [131.107.159.43]) by linux.microsoft.com (Postfix) with ESMTPSA id 3C80520E6F5F; Fri, 29 Mar 2024 13:21:25 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3C80520E6F5F From: "Oliver Smith-Denny" To: devel@edk2.groups.io Cc: Liming Gao , Taylor Beebe , Michael D Kinney , Ard Biesheuvel Subject: [edk2-devel] [PATCH v3 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib Date: Fri, 29 Mar 2024 13:21:29 -0700 Message-Id: <20240329202129.12988-4-osde@linux.microsoft.com> In-Reply-To: <20240329202129.12988-1-osde@linux.microsoft.com> References: <20240329202129.12988-1-osde@linux.microsoft.com> MIME-Version: 1.0 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Fri, 29 Mar 2024 13:21:25 -0700 Reply-To: devel@edk2.groups.io,osde@linux.microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: JSdOUZY6aDx65Or97nmbmqNOx7686176AA= Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=QqIjRZ1C; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=linux.microsoft.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io The functionality to create and delete Image Records has been=0D consolidated in a library and ensured that MemoryProtection.c's=0D usage is encapsulated there.=0D =0D This patch moves MemoryProtection.c to reuse the code in the lib=0D and to prevent issues in the future where code is updated in one=0D place but not the other.=0D =0D Cc: Liming Gao =0D Cc: Taylor Beebe =0D =0D Acked-by: Michael D Kinney =0D Reviewed-by: Ard Biesheuvel =0D Signed-off-by: Oliver Smith-Denny =0D ---=0D MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++-----------------=0D 1 file changed, 28 insertions(+), 213 deletions(-)=0D =0D diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/C= ore/Dxe/Misc/MemoryProtection.c=0D index eb243a0137b1..2c069cc12c63 100644=0D --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c=0D +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c=0D @@ -281,80 +281,43 @@ SetUefiImageProtectionAttributes (=0D }=0D =0D /**=0D - Return if the PE image section is aligned.=0D + Return the section alignment requirement for the PE image section type.= =0D =0D - @param[in] SectionAlignment PE/COFF section alignment=0D - @param[in] MemoryType PE/COFF image memory type=0D + @param[in] MemoryType PE/COFF image memory type=0D +=0D + @retval The required section alignment for this memory type=0D =0D - @retval TRUE The PE image section is aligned.=0D - @retval FALSE The PE image section is not aligned.=0D **/=0D -BOOLEAN=0D -IsMemoryProtectionSectionAligned (=0D - IN UINT32 SectionAlignment,=0D +STATIC=0D +UINT32=0D +GetMemoryProtectionSectionAlignment (=0D IN EFI_MEMORY_TYPE MemoryType=0D )=0D {=0D - UINT32 PageAlignment;=0D + UINT32 SectionAlignment;=0D =0D switch (MemoryType) {=0D case EfiRuntimeServicesCode:=0D case EfiACPIMemoryNVS:=0D case EfiReservedMemoryType:=0D - PageAlignment =3D RUNTIME_PAGE_ALLOCATION_GRANULARITY;=0D + SectionAlignment =3D RUNTIME_PAGE_ALLOCATION_GRANULARITY;=0D break;=0D case EfiRuntimeServicesData:=0D ASSERT (FALSE);=0D - PageAlignment =3D RUNTIME_PAGE_ALLOCATION_GRANULARITY;=0D + SectionAlignment =3D RUNTIME_PAGE_ALLOCATION_GRANULARITY;=0D break;=0D case EfiBootServicesCode:=0D case EfiLoaderCode:=0D - PageAlignment =3D EFI_PAGE_SIZE;=0D + SectionAlignment =3D EFI_PAGE_SIZE;=0D break;=0D case EfiACPIReclaimMemory:=0D default:=0D ASSERT (FALSE);=0D - PageAlignment =3D EFI_PAGE_SIZE;=0D + SectionAlignment =3D EFI_PAGE_SIZE;=0D break;=0D }=0D =0D - if ((SectionAlignment & (PageAlignment - 1)) !=3D 0) {=0D - return FALSE;=0D - } else {=0D - return TRUE;=0D - }=0D -}=0D -=0D -/**=0D - Free Image record.=0D -=0D - @param[in] ImageRecord A UEFI image record=0D -**/=0D -VOID=0D -FreeImageRecord (=0D - IN IMAGE_PROPERTIES_RECORD *ImageRecord=0D - )=0D -{=0D - LIST_ENTRY *CodeSegmentListHead;=0D - IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;=0D -=0D - CodeSegmentListHead =3D &ImageRecord->CodeSegmentList;=0D - while (!IsListEmpty (CodeSegmentListHead)) {=0D - ImageRecordCodeSection =3D CR (=0D - CodeSegmentListHead->ForwardLink,=0D - IMAGE_PROPERTIES_RECORD_CODE_SECTION,=0D - Link,=0D - IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNAT= URE=0D - );=0D - RemoveEntryList (&ImageRecordCodeSection->Link);=0D - FreePool (ImageRecordCodeSection);=0D - }=0D -=0D - if (ImageRecord->Link.ForwardLink !=3D NULL) {=0D - RemoveEntryList (&ImageRecord->Link);=0D - }=0D -=0D - FreePool (ImageRecord);=0D + return SectionAlignment;=0D }=0D =0D /**=0D @@ -369,19 +332,10 @@ ProtectUefiImage (=0D IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath=0D )=0D {=0D - VOID *ImageAddress;=0D - EFI_IMAGE_DOS_HEADER *DosHdr;=0D - UINT32 PeCoffHeaderOffset;=0D - UINT32 SectionAlignment;=0D - EFI_IMAGE_SECTION_HEADER *Section;=0D - EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;=0D - UINT8 *Name;=0D - UINTN Index;=0D - IMAGE_PROPERTIES_RECORD *ImageRecord;=0D - CHAR8 *PdbPointer;=0D - IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;=0D - BOOLEAN IsAligned;=0D - UINT32 ProtectionPolicy;=0D + IMAGE_PROPERTIES_RECORD *ImageRecord;=0D + UINT32 ProtectionPolicy;=0D + EFI_STATUS Status;=0D + UINT32 RequiredAlignment;=0D =0D DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage));=0D DEBUG ((DEBUG_INFO, " - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(= UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize));=0D @@ -406,160 +360,21 @@ ProtectUefiImage (=0D return;=0D }=0D =0D - ImageRecord->Signature =3D IMAGE_PROPERTIES_RECORD_SIGNATURE;=0D + RequiredAlignment =3D GetMemoryProtectionSectionAlignment (LoadedImage->= ImageCodeType);=0D =0D - //=0D - // Step 1: record whole region=0D - //=0D - ImageRecord->ImageBase =3D (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->Ima= geBase;=0D - ImageRecord->ImageSize =3D LoadedImage->ImageSize;=0D -=0D - ImageAddress =3D LoadedImage->ImageBase;=0D -=0D - PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress);= =0D - if (PdbPointer !=3D NULL) {=0D - DEBUG ((DEBUG_VERBOSE, " Image - %a\n", PdbPointer));=0D - }=0D -=0D - //=0D - // Check PE/COFF image=0D - //=0D - DosHdr =3D (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress;=0D - PeCoffHeaderOffset =3D 0;=0D - if (DosHdr->e_magic =3D=3D EFI_IMAGE_DOS_SIGNATURE) {=0D - PeCoffHeaderOffset =3D DosHdr->e_lfanew;=0D - }=0D -=0D - Hdr.Pe32 =3D (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress + P= eCoffHeaderOffset);=0D - if (Hdr.Pe32->Signature !=3D EFI_IMAGE_NT_SIGNATURE) {=0D - DEBUG ((DEBUG_VERBOSE, "Hdr.Pe32->Signature invalid - 0x%x\n", Hdr.Pe3= 2->Signature));=0D - // It might be image in SMM.=0D - goto Finish;=0D - }=0D -=0D - //=0D - // Get SectionAlignment=0D - //=0D - if (Hdr.Pe32->OptionalHeader.Magic =3D=3D EFI_IMAGE_NT_OPTIONAL_HDR32_MA= GIC) {=0D - SectionAlignment =3D Hdr.Pe32->OptionalHeader.SectionAlignment;=0D - } else {=0D - SectionAlignment =3D Hdr.Pe32Plus->OptionalHeader.SectionAlignment;=0D - }=0D -=0D - IsAligned =3D IsMemoryProtectionSectionAligned (SectionAlignment, Loaded= Image->ImageCodeType);=0D - if (!IsAligned) {=0D - DEBUG ((=0D - DEBUG_VERBOSE,=0D - "!!!!!!!! ProtectUefiImageCommon - Section Alignment(0x%x) is incor= rect !!!!!!!!\n",=0D - SectionAlignment=0D - ));=0D - PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress)= ;=0D - if (PdbPointer !=3D NULL) {=0D - DEBUG ((DEBUG_VERBOSE, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointe= r));=0D - }=0D + Status =3D CreateImagePropertiesRecord (=0D + LoadedImage->ImageBase,=0D + LoadedImage->ImageSize,=0D + &RequiredAlignment,=0D + ImageRecord=0D + );=0D =0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((DEBUG_ERROR, "%a failed to create image properties record\n", = __func__));=0D + FreePool (ImageRecord);=0D goto Finish;=0D }=0D =0D - Section =3D (EFI_IMAGE_SECTION_HEADER *)(=0D - (UINT8 *)(UINTN)ImageAddress +=0D - PeCoffHeaderOffset +=0D - sizeof (UINT32) +=0D - sizeof (EFI_IMAGE_FILE_HEADER) += =0D - Hdr.Pe32->FileHeader.SizeOfOption= alHeader=0D - );=0D - ImageRecord->CodeSegmentCount =3D 0;=0D - InitializeListHead (&ImageRecord->CodeSegmentList);=0D - for (Index =3D 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++= ) {=0D - Name =3D Section[Index].Name;=0D - DEBUG ((=0D - DEBUG_VERBOSE,=0D - " Section - '%c%c%c%c%c%c%c%c'\n",=0D - Name[0],=0D - Name[1],=0D - Name[2],=0D - Name[3],=0D - Name[4],=0D - Name[5],=0D - Name[6],=0D - Name[7]=0D - ));=0D -=0D - //=0D - // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CN= T_CODE=0D - // can always be mapped read-only, classify a section as a code sectio= n only=0D - // if it has the executable attribute set and the writable attribute c= leared.=0D - //=0D - // This adheres more closely to the PE/COFF spec, and avoids issues wi= th=0D - // Linux OS loaders that may consist of a single read/write/execute se= ction.=0D - //=0D - if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE | EFI_I= MAGE_SCN_MEM_EXECUTE)) =3D=3D EFI_IMAGE_SCN_MEM_EXECUTE) {=0D - DEBUG ((DEBUG_VERBOSE, " VirtualSize - 0x%08x\n", Section[= Index].Misc.VirtualSize));=0D - DEBUG ((DEBUG_VERBOSE, " VirtualAddress - 0x%08x\n", Section[= Index].VirtualAddress));=0D - DEBUG ((DEBUG_VERBOSE, " SizeOfRawData - 0x%08x\n", Section[= Index].SizeOfRawData));=0D - DEBUG ((DEBUG_VERBOSE, " PointerToRawData - 0x%08x\n", Section[= Index].PointerToRawData));=0D - DEBUG ((DEBUG_VERBOSE, " PointerToRelocations - 0x%08x\n", Section[= Index].PointerToRelocations));=0D - DEBUG ((DEBUG_VERBOSE, " PointerToLinenumbers - 0x%08x\n", Section[= Index].PointerToLinenumbers));=0D - DEBUG ((DEBUG_VERBOSE, " NumberOfRelocations - 0x%08x\n", Section[= Index].NumberOfRelocations));=0D - DEBUG ((DEBUG_VERBOSE, " NumberOfLinenumbers - 0x%08x\n", Section[= Index].NumberOfLinenumbers));=0D - DEBUG ((DEBUG_VERBOSE, " Characteristics - 0x%08x\n", Section[= Index].Characteristics));=0D -=0D - //=0D - // Step 2: record code section=0D - //=0D - ImageRecordCodeSection =3D AllocatePool (sizeof (*ImageRecordCodeSec= tion));=0D - if (ImageRecordCodeSection =3D=3D NULL) {=0D - return;=0D - }=0D -=0D - ImageRecordCodeSection->Signature =3D IMAGE_PROPERTIES_RECORD_CODE_S= ECTION_SIGNATURE;=0D -=0D - ImageRecordCodeSection->CodeSegmentBase =3D (UINTN)ImageAddress + Se= ction[Index].VirtualAddress;=0D - ImageRecordCodeSection->CodeSegmentSize =3D ALIGN_VALUE (Section[Ind= ex].SizeOfRawData, SectionAlignment);=0D -=0D - DEBUG ((DEBUG_VERBOSE, "ImageCode: 0x%016lx - 0x%016lx\n", ImageReco= rdCodeSection->CodeSegmentBase, ImageRecordCodeSection->CodeSegmentSize));= =0D -=0D - InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSecti= on->Link);=0D - ImageRecord->CodeSegmentCount++;=0D - }=0D - }=0D -=0D - if (ImageRecord->CodeSegmentCount =3D=3D 0) {=0D - //=0D - // If a UEFI executable consists of a single read+write+exec PE/COFF=0D - // section, that isn't actually an error. The image can be launched=0D - // alright, only image protection cannot be applied to it fully.=0D - //=0D - // One example that elicits this is (some) Linux kernels (with the EFI= stub=0D - // of course).=0D - //=0D - DEBUG ((DEBUG_WARN, "!!!!!!!! ProtectUefiImageCommon - CodeSegmentCou= nt is 0 !!!!!!!!\n"));=0D - PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress)= ;=0D - if (PdbPointer !=3D NULL) {=0D - DEBUG ((DEBUG_WARN, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer))= ;=0D - }=0D -=0D - goto Finish;=0D - }=0D -=0D - //=0D - // Final=0D - //=0D - SortImageRecordCodeSection (ImageRecord);=0D - //=0D - // Check overlap all section in ImageBase/Size=0D - //=0D - if (!IsImageRecordCodeSectionValid (ImageRecord)) {=0D - DEBUG ((DEBUG_ERROR, "IsImageRecordCodeSectionValid - FAIL\n"));=0D - goto Finish;=0D - }=0D -=0D - //=0D - // Round up the ImageSize, some CPU arch may return EFI_UNSUPPORTED if I= mageSize is not aligned.=0D - // Given that the loader always allocates full pages, we know the space = after the image is not used.=0D - //=0D - ImageRecord->ImageSize =3D ALIGN_VALUE (LoadedImage->ImageSize, EFI_PAGE= _SIZE);=0D -=0D //=0D // CPU ARCH present. Update memory attribute directly.=0D //=0D @@ -607,7 +422,7 @@ UnprotectUefiImage (=0D ImageRecord->ImageSize,=0D 0=0D );=0D - FreeImageRecord (ImageRecord);=0D + DeleteImagePropertiesRecord (ImageRecord);=0D return;=0D }=0D }=0D -- =0D 2.40.1=0D =0D -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117242): https://edk2.groups.io/g/devel/message/117242 Mute This Topic: https://groups.io/mt/105223005/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-