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 CAC20941D61 for ; Mon, 4 Mar 2024 19:24:34 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=rjQASYwiqa68cfNwaAsqf6fnU9si55lmUGZFp1Qa7Hk=; 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=1709580273; v=1; b=Jf7DWaKBevisvHHdyB3dpRoHn14rhGH8+Ln4ZzmdXPcypT7bqA8pIkkVzsU7GofHctz32sz0 bnnuw+b0dN1TmaIj+7EoLKRXGr+yHUKnRdgxNTIlKpJjRDnCtA+pMs8qFeFZ+383LQ7vhVUTcOz Wh5g4stCLKRZ6bMjgpEAV3M0= X-Received: by 127.0.0.2 with SMTP id lozQYY7687511xEapfniIb45; Mon, 04 Mar 2024 11:24:33 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.634.1709580272845193454 for ; Mon, 04 Mar 2024 11:24:32 -0800 X-Received: from [10.137.194.171] (unknown [131.107.160.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 1714920B74C0; Mon, 4 Mar 2024 11:24:32 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1714920B74C0 Message-ID: <531b3cfe-b57c-4b5a-97fd-45e428f97853@linux.microsoft.com> Date: Mon, 4 Mar 2024 11:24:31 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize To: devel@edk2.groups.io, ardb@kernel.org Cc: 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> <5d95548d-d25f-4306-a271-a2960cc81ccf@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: NyKAmfAybCMcuX9jRuvXrWb5x7686176AA= 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=Jf7DWaKB; 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 On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: > On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny > wrote: >> >> Hi Ard, >> >> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: >>> Hi Oliver, >>> >>> On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny >>> wrote: >>>> >>>> When an ImageRecord is stored by ImagePropertiesRecordLib, it reports = the >>>> CodeSegmentSize as the SizeOfRawData from the image. However, the imag= e >>>> 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 SectionAlignmen= t >>>> and the corresponding memory attribute table entries are now correctly >>>> aligned and pointing to the right places in memory. >>>> >>> >>> 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? >>> >> >> 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. >> >=20 > IIUC the SizeOfRawData is the file view not the memory view, and must > always be aligned to the FileAlignment. I relooked at the spec, you are correct, SizeOfRawData is aligned to the FileAlignment. So this is only a bug for a file with SectionAlignment !=3D FileAlignment. >=20 >> 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. >> >=20 > Yeah, no disagreement there. >=20 >>>> Cc: Liming Gao >>>> Cc: Leif Lindholm >>>> Cc: Ard Biesheuvel >>>> Cc: Sami Mujawar >>>> Cc: Taylor Beebe >>>> >>>> Signed-off-by: Oliver Smith-Denny >>>> --- >>>> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecord= Lib.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImageProper= tiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImageProper= tiesRecordLib.c >>>> index e53ce086c54c..07ced0e54e38 100644 >>>> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRec= ordLib.c >>>> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRec= ordLib.c >>>> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord ( >>>> ImageRecordCodeSection->Signature =3D IMAGE_PROPERTIES_RECORD= _CODE_SECTION_SIGNATURE; >>>> >>>> ImageRecordCodeSection->CodeSegmentBase =3D (UINTN)ImageBase = + Section[Index].VirtualAddress; >>>> - ImageRecordCodeSection->CodeSegmentSize =3D Section[Index].Size= OfRawData; >>>> + ImageRecordCodeSection->CodeSegmentSize =3D ALIGN_VALUE (Sectio= n[Index].SizeOfRawData, SectionAlignment); >>>> >>> >>> 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 > No, I mean we should not be using SizeOfRawData here but VirtualSize. >=20 > I understand this is unlikely to make a difference in practice, but > VirtualSize may exceed SizeOfRawData by a wide margin, and we need the > whole region to be covered. >=20 I see, yes I do believe VirtualSize could be used here instead. Two things give me pause. One is that the PE spec states that SizeOfRawData is rounded and VirtualSize is not, so that SizeOfRawData may be greater than the VirtualSize in some cases (which seems incorrect). The other is that the image loader partially uses VirtualSize when loading: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7= dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 However, when determining the size of a loaded image (and therefore the number of pages to allocate) it will allocate an extra page: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7= dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 as ImageSize here is: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7= dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 Which according to the spec, SizeOfImage is the size of the image as loaded into memory and must be a multiple of section alignment. So, reflecting on this, let me test with VirtualSize here, I think that is the right value to use, the only time we would have a SizeOfRawData that is greater than the VirtualSize would be if our FileAlignment is greater than our SectionAlignment, which would be a misconfiguration. I do think the ImageLoader should also be fixed to only allocate ImageSize number of pages (which should be the sum of the section VirtualSizes + any headers that aren't included). Might as well not waste an extra page for each image and then our image record code is simpler as well (ensuring we are protecting the right pages). I think this patch series should go in, as it is fixing an active bug, but I will also take a look at the image loader creating the image records and having a protocol it produces to retrieve the list, to attempt to avoid issues like this in the future. -=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 (#116340): https://edk2.groups.io/g/devel/message/116340 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-