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 39C16941370 for ; Tue, 12 Mar 2024 08:32:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=3LN0Ii3LcGXfCWBcfDJKf+egvKfqlCCcSfxqdCYwVvk=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1710232349; v=1; b=wMf7xW74GFK2Z0R56O1W0vn5D8ZUetU1iykEsvubyp7/uNIWGTKcV8r269A1SALXgkc3fnRV XoULWR+SuhSNRYEdvhYIWQCeXLpUuDsHJom0Xp+CJoDBTLbqgWh5vtvSs8VCVaP+Ty0atXQrBJ/ wXvVenqScZb+K5AJGUgl1tqLZxRHoWtxQxarGVxnxGg+NkK3E/zDo6M595DHGtXGOGOwunDPkY9 ik5vIXP7aHGrpU5OTCJDBznz9seKSOjJ1aeRMmT7R6oQYELXA+1KQlVTXRNUz/PJZ6goBrw2Hsm HuQdfSWNQ2vd/CMOkuPQj3Cxn5BbqZV/wV6EPBniFVO7Q== X-Received: by 127.0.0.2 with SMTP id E63kYY7687511x27nvlMXUxj; Tue, 12 Mar 2024 01:32:29 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.3906.1710232349011771185 for ; Tue, 12 Mar 2024 01:32:29 -0700 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 6966361113 for ; Tue, 12 Mar 2024 08:32:28 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 06067C43601 for ; Tue, 12 Mar 2024 08:32:28 +0000 (UTC) X-Received: by mail-lj1-f171.google.com with SMTP id 38308e7fff4ca-2d459a0462fso4145961fa.2 for ; Tue, 12 Mar 2024 01:32:27 -0700 (PDT) X-Gm-Message-State: hk7JOJzwQoVQ6xNJ6S7wJ4bwx7686176AA= X-Google-Smtp-Source: AGHT+IFbhoPVsMVNGaDZGKjLlEPaC4rLRgWcLP+/H1ci8ddvRCPKjTNHXqAmxV4dT1MdAHV00idAwm00vRd/xtAxYXk= X-Received: by 2002:a2e:8e8d:0:b0:2d3:7697:1ff5 with SMTP id z13-20020a2e8e8d000000b002d376971ff5mr6087464ljk.34.1710232346018; Tue, 12 Mar 2024 01:32:26 -0700 (PDT) MIME-Version: 1.0 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> <17B9B0CE1BDDC06B.30946@groups.io> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 12 Mar 2024 09:32:14 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize To: devel@edk2.groups.io, osde@linux.microsoft.com Cc: Liming Gao , Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe 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: Tue, 12 Mar 2024 01:32:29 -0700 Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=wMf7xW74; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (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 Mon, 11 Mar 2024 at 20:34, Oliver Smith-Denny wrote: > > On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote: > > 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: > >>>>>> > >>>>>> - ImageRecordCodeSection->CodeSegmentSize = > >>>>>> Section[Index].SizeOfRawData; > >>>>>> + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE > >>>>>> (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. > >>> > >> > >> 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/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/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/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 > >> > >> as ImageSize here is: > >> > >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/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. > >> > > > > 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. > > > > Following up on this: > > Not surprisingly, different toolchains do different things here. > > gcc obviously creates ELF files and ElfConvert*.c converts these to PE > images. However, when it does this, it always sets the section and file > alignment to the same value (I'm not as familiar with ELF images, I'm > not sure if there is the same concept as file vs section alignment). > GenFw could probably update this information to shrink the file > alignment, but that's a space optimization for gcc built binaries. > > In ElfConvert.c, the VirtualSize does get set to the section aligned > value, which is what I would expect from the spec. > > In MSVC PE images, VirtualSize is not section aligned and the > expectation appears to be that loaders will align the VirtualSize > to the section alignment. I am planning on reaching out to the MSVC > folks to learn why this is the case, is it intended, etc., as my > understanding is that the VirtualSize in the section headers should > be section aligned. > > We are stuck with this for existing MSVC toolchains, however, so we > will need to align either the VirtualSize or SizeOfRawData to the > section alignment when we create the image records. I don't have a > preference between the two, they end up being the same when we align > them, so I can send a v2 with aligning the VirtualSize. This will be > a no-op on gcc built binaries and will set it to the correct value for > MSVC. > I don't think this is correct. The VirtualSize could be much larger than the SizeOfRawData (to the extent where the delta exceeds the alignment padding), in cases where some of the memory is data-initialized and some of it is zero-initialized. We certainly make use of this in Linux when constructing the PE/COFF view of the kernel image. So for the memory view of the image, only VirtualSize is appropriate - SizeOfRawData just gives you the size of the data to copy to the start of this section. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116671): https://edk2.groups.io/g/devel/message/116671 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] -=-=-=-=-=-=-=-=-=-=-=-