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

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

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.

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.

>> 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.

> 
>>         InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link);
>>         ImageRecord->CodeSegmentCount++;
>> --
>> 2.40.1
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116334): https://edk2.groups.io/g/devel/message/116334
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]
-=-=-=-=-=-=-=-=-=-=-=-