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
<o...@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
<o...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
