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 BD82D740035 for ; Mon, 4 Mar 2024 22:38:54 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+6n+Po3mQFtzlBotJ7pnzzQ88dZ914/4MCPc3N3fcr0=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:Message-ID:Date:MIME-Version:User-Agent:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709591933; v=1; b=vQZC63J4hU08LVzaCkxSBWLoam27x7oqmnaY2ZgnLlNNwTywsSQTv8u0lwufN7PItxc639Bz kt6kvbTP8+ZGHTMf1G8RO8Ev3sFc9SeRuFvWXX/DvnB46FV5WLZs5UW3aqierfwKK6TU3K+6mol Wlzm+v+mKPO8agIwSWJkvi5I= X-Received: by 127.0.0.2 with SMTP id XzHWYY7687511xSlK3xfLz6Y; Mon, 04 Mar 2024 14:38:53 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.6988.1709591932684061490 for ; Mon, 04 Mar 2024 14:38:52 -0800 X-Received: from [10.137.194.171] (unknown [131.107.160.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 1B49920B74C0; Mon, 4 Mar 2024 14:38:52 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1B49920B74C0 Message-ID: <3d4fc716-f0d7-47c8-9a1d-89da05049ce2@linux.microsoft.com> Date: Mon, 4 Mar 2024 14:38:51 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize From: "Oliver Smith-Denny" To: devel@edk2.groups.io, ardb@kernel.org Cc: Liming Gao , Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe Reply-To: devel@edk2.groups.io,osde@linux.microsoft.com References: <20240227202721.30070-1-osde@linux.microsoft.com> <20240227202721.30070-2-osde@linux.microsoft.com> <5d95548d-d25f-4306-a271-a2960cc81ccf@linux.microsoft.com> <17B9A63355E03AE5.30946@groups.io> In-Reply-To: <17B9A63355E03AE5.30946@groups.io> 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 List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: E7Ny9G9Euq7vGKlw0VmJbyQ2x7686176AA= 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=vQZC63J4; 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 11:24 AM, Oliver Smith-Denny wrote: > 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: >>>>> >>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ImageRecordCodeSection->CodeSegmentSi= ze =3D=20 >>>>> Section[Index].SizeOfRawData; >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ImageRecordCodeSection->CodeSegmentSi= ze =3D ALIGN_VALUE=20 >>>>> (Section[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. >>> >> >> No, I mean we should not be using SizeOfRawData here but VirtualSize. >> >> 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). >=20 > The other is that the image loader partially uses VirtualSize when > loading: >=20 > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2= c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 >=20 > However, when determining the size of a loaded image (and therefore the > number of pages to allocate) it will allocate an extra page: >=20 > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2= c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 >=20 > as ImageSize here is: >=20 > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2= c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 >=20 > Which according to the spec, SizeOfImage is the size of the image as > loaded into memory and must be a multiple of section alignment. >=20 > 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. >=20 > 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). >=20 > 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. >=20 Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. I am looking deeper into GenFw to make sure it is correctly getting set to align to SectionAlignment in the section headers. When I use dumpbin to dump the headers, it shows each section having VirtualSize as 64k aligned for a runtime image, but the same image doesn't show that in FW. I'll do some digging here. Thanks, Oliver -=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 (#116343): https://edk2.groups.io/g/devel/message/116343 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-