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 4A3757803D0 for ; Mon, 4 Mar 2024 17:49:06 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=M7yKc+YqfCxkLYcJBZ+1XfJY2HQv1OZGlgXXSjeew5c=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709574545; v=1; b=LoC/O4/mdpQTh6jmDUTJJj27PXsfa7I2nsr5CvNUFNJYXxOapqlggWN0lQSXHnGMQ8UpSF5M l+1p7hd/j3SDjzUgWr0ROYfqOAJVAjaPcw/ijS8rAWKpscA3y3yr9jSYuhrfVF7/EL+vEItVA5Y bDVkLCHPA6QjlaqvrKhdYHjY= X-Received: by 127.0.0.2 with SMTP id 0iucYY7687511xFE0CIIgBcg; Mon, 04 Mar 2024 09:49:05 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.1244.1709574544456110604 for ; Mon, 04 Mar 2024 09:49:04 -0800 X-Received: from [10.137.194.171] (unknown [131.107.160.171]) by linux.microsoft.com (Postfix) with ESMTPSA id D511120B74C0; Mon, 4 Mar 2024 09:49:03 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D511120B74C0 Message-ID: <5d95548d-d25f-4306-a271-a2960cc81ccf@linux.microsoft.com> Date: Mon, 4 Mar 2024 09:49:03 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize To: Ard Biesheuvel Cc: devel@edk2.groups.io, Liming Gao , Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe References: <20240227202721.30070-1-osde@linux.microsoft.com> <20240227202721.30070-2-osde@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: 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 Reply-To: devel@edk2.groups.io,osde@linux.microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: evtSVxquiOBRFPA4IgfVBGTPx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="LoC/O4/m"; 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 Hi Ard, On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: > Hi Oliver, >=20 > On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny > wrote: >> >> When an ImageRecord is stored by ImagePropertiesRecordLib, it reports th= e >> CodeSegmentSize as the SizeOfRawData from the image. However, the image >> as loaded into memory is aligned to the SectionAlignment, so >> SizeOfRawData is under the actual size in memory. This is important, >> because the memory attributes table uses these image records to create >> its entries and it will report that the alignment of an image is >> incorrect, even though the actual image is correct. >> >> This was discovered on ARM64, which has a 64k runtime page granularity >> alignment, which is backed by a 64k section alignment for >> DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being >> loaded into memory, however the memory attribute table was incorrectly >> reporting misaligned ranges to the OS, causing attributes to be >> ignored for these sections for OSes using greater than 4k pages. >> >> This patch correctly aligns the CodeSegmentSize to the SectionAlignment >> and the corresponding memory attribute table entries are now correctly >> aligned and pointing to the right places in memory. >> >=20 > Can you explain how these can differ in the first place? Our flaky > ELF-to-PE/COFF converter should never generate such images to begin > with (which is probably how we ended up with this problem in the first > place), so I suppose this is native PE/COFF tooling emitting sections > either using a non-1:1 file:memory mapping, or with unallocated holes > in the file representation? >=20 This is an artifact of PE/COFF itself and is useful for having smaller FW images. In PE/COFF we have the section alignment (which is how we get loaded into memory) and the file alignment (how the actual file is aligned). It is valid for these two to be different. For example, these runtime DXE drivers, which are not XIP (which the case we do need the section and file alignment to be the same, as we are executing from the file image) can be a smaller size in the file, but when loaded into memory we will relocate them and do the proper rebasing to set these on 64k boundaries. Different toolchains have different ways of specifying the two things, on gcc I have seen common-page-size affect the section alignment and max-page-size affect both section and file alignment. For msvc there are /ALIGN and /FILEALIGN commands which directly manipulate these values. The issue here was not that we have different section and file alignment, in fact, the issue still exists if section alignement =3D=3D file alignment. This is because SizeOfRawData in the PE/COFF header is the raw size of bytes used, not even page aligned. So no matter what, the image records we were creating here had bad sizes being set which do not match what the image loader was actually doing. I do think there is a fair argument that would say the image loader should create the image records when it loads images, since in the end we want the record to match exactly what the image in memory is, creating after the fact is a poor pattern. >> Cc: Liming Gao >> Cc: Leif Lindholm >> Cc: Ard Biesheuvel >> Cc: Sami Mujawar >> Cc: Taylor Beebe >> >> Signed-off-by: Oliver Smith-Denny >> --- >> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib= .c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImageProperti= esRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImageProperti= esRecordLib.c >> index e53ce086c54c..07ced0e54e38 100644 >> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecor= dLib.c >> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecor= dLib.c >> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord ( >> ImageRecordCodeSection->Signature =3D IMAGE_PROPERTIES_RECORD_CO= DE_SECTION_SIGNATURE; >> >> ImageRecordCodeSection->CodeSegmentBase =3D (UINTN)ImageBase + S= ection[Index].VirtualAddress; >> - ImageRecordCodeSection->CodeSegmentSize =3D Section[Index].SizeOf= RawData; >> + ImageRecordCodeSection->CodeSegmentSize =3D ALIGN_VALUE (Section[= Index].SizeOfRawData, SectionAlignment); >> >=20 > This should be the virtual size, not the file size, right? Correct, SectionAlignment is the alignment of the image as loaded in memory, so in the case of a DXE runtime driver on ARM64, it will be 64k. >=20 >> InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeS= ection->Link); >> ImageRecord->CodeSegmentCount++; >> -- >> 2.40.1 >> -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116334): https://edk2.groups.io/g/devel/message/116334 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-