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

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

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

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-