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 B0835AC18F5 for ; Mon, 4 Mar 2024 18:55:04 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=3S7kZ8C/zqsDWyX5rxOgfy1jGqMcNirjYNH4Q7Fw85c=; 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:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1709578503; v=1; b=DpCorDRFWWrCUmAswXUoaHTjpM80SWDMOvHXCTwiqCFDjHPm1TYXK+YFp9L6TXP0tL9MSBSb /4Hw+zKlqAM01tVY1J7DW3oaThBxxAcUfidbuLISwe+yZeWK/xFZWgE4HNrA1RvfmoxfxgfnZkx Kv2m+qA7j4Lum7J8T5Q8d3GI= X-Received: by 127.0.0.2 with SMTP id 4CCEYY7687511xLfz4T5rgrp; Mon, 04 Mar 2024 10:55:03 -0800 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.3086.1709578502575777450 for ; Mon, 04 Mar 2024 10:55:02 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 08C026119F for ; Mon, 4 Mar 2024 18:55:02 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id A3057C43394 for ; Mon, 4 Mar 2024 18:55:01 +0000 (UTC) X-Received: by mail-lf1-f53.google.com with SMTP id 2adb3069b0e04-5132181d54bso5430320e87.3 for ; Mon, 04 Mar 2024 10:55:01 -0800 (PST) X-Gm-Message-State: 6dl9y9clT8hrVkbD9TcSrwjkx7686176AA= X-Google-Smtp-Source: AGHT+IEY0TEzYbGl5wxb/onyihuKdAfWCKM/+I2KsP1f3Fd+QlfKS/fHTp2PTWw/C40xXoF+EcXptN9HystGqBj6e70= X-Received: by 2002:a05:6512:1389:b0:512:f657:122d with SMTP id fc9-20020a056512138900b00512f657122dmr7760255lfb.12.1709578499817; Mon, 04 Mar 2024 10:54:59 -0800 (PST) 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> In-Reply-To: <5d95548d-d25f-4306-a271-a2960cc81ccf@linux.microsoft.com> From: "Ard Biesheuvel" Date: Mon, 4 Mar 2024 19:54:48 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize To: Oliver Smith-Denny Cc: devel@edk2.groups.io, 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 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=20140610 header.b=DpCorDRF; 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, 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 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. > >> > > > > 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 == > 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. > IIUC the SizeOfRawData is the file view not the memory view, and must always be aligned to the FileAlignment. > 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. > Yeah, no disagreement there. > >> 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/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> index e53ce086c54c..07ced0e54e38 100644 > >> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord ( > >> ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE; > >> > >> ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress; > >> - 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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116338): https://edk2.groups.io/g/devel/message/116338 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] -=-=-=-=-=-=-=-=-=-=-=-