Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize

["Ard Biesheuvel" <ardb@kernel.org>]

On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
<osde@linux.microsoft.com> 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
> > <osde@linux.microsoft.com> 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 <gaoliming@byosoft.com.cn>
> >> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >> Cc: Taylor Beebe <taylor.d.beebe@gmail.com>
> >>
> >> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
> >> ---
> >>   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]
-=-=-=-=-=-=-=-=-=-=-=-