Hi Marvin,
Thanks for looking at this. Since sending out the v1, I have done a lot
more digging into our PE loader, others, the spec, and differences
between MSVC and our ElfConvert tool. I agree that my understanding was
flawed in my original comments.
On 3/14/2024 2:57 AM, Marvin Häuser wrote:
Good day everyone,
Sorry for interjecting, but I’d like to avoid premature changes to PE
code that would only make the current mess even worse.
On 4. Mar 2024, at 20:24, Oliver Smith-Denny <osde@...> wrote:
I relooked at the spec, you are correct, SizeOfRawData is aligned to
the FileAlignment. So this is only a bug for a file with
SectionAlignment != FileAlignment.
Still no, for some reasons you already mentioned, but also reasons you
didn’t:
- VirtualSize often is not aligned, so they can still trivially mismatch.
- VirtualSize includes zero-initialized data not part of the file.
This does not appear to be the case with MSVC built binaries. I am
seeing that VirtualSize is the size of the data-initialized part of the
section. Whereas on our ElfConverted binaries, what you say is true. The
difference concerns me.
- In the old days, VirtualSize = 0 was used to disable loading of debug
data, but SizeOfRawData remained intact, so it could be loaded on demand.
- SizeOfRawData may be unaligned due to bugs. In fact, there is no
reason to trust FileAlignment, it has no real meaning in an UEFI context
(I am not even sure it does on Windows).
I see, yes I do believe VirtualSize could be used here instead.
*Must*. As Ard said, SizeOfRawData is only interesting to know how many
bytes to copy from the file and for *nothing* else.
Agreed. I have done further research and v2 uses VirtualSize. I was
initially led astray here because the ElfConvert tool sets VirtualSize
and SizeOfRawData to the SectionAlignment universally:
https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136
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
Sorry, I don’t understand the “however”, this is sound.
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.
Side note, SizeOfImage may be borked, not only due to bugs, but also due
to malice. There is literally no reason whatsoever to use it - to
validate it, you need to calculate a sane value, and once you have that,
you obviously do not need SizeOfImage anymore. You could reject images
with malformed SizeOfImage, but there likely are various
legitimate-bugged ones out there. This is just an ancient artifact from
before memory safety was a thing. :)
Yep, the PE headers are definitely filled with information that can be
deduced from other sources and the spec is not very verbose, which is
why I definitely want to tread carefully here. For example, it says
SizeOfImage is the size of the image loaded into memory, which
current implementations seem to take to mean the sum of the headers
plus each section's VirtualSize rounded to the SectionAlignment (which
the spec does call out that SizeOfImage must be aligned to the
SectionAlignment). VirtualSize it says is also the size of the section
as loaded into memory, but current implementations (other than our
ElfConvert script which I'm currently investigating if we should
change that) seem to take this to mean the size of the initialized
data in memory, i.e. the "real" data from the image, not the zeroed
data to get to the SectionAlignment. But the language in the spec
is almost identical between the two. And again, we have a difference
in our MSVC and gcc built binaries here.
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.
No, it can simply be that FileAlignment = SectionAlignment and
SizeOfRawData is aligned and VirtualSize is not. Even when both are
aligned, the former may carry extra optional data (like previously
mentioned debug data), or it might be some weird I/O performance related
padding or something.
Agreed, this was misinformed by looking at gcc built binaries.
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).
NO! :) There are *many* issues surrounding this:
- Sections may overlap (observed with early macOS EFI images, could be
ignored, but needs to be validated across the landscape).
- Sections may have gaps (observed with “old" Linux EFI Shims iirc,
there’ll be plenty of broken images in the wild yet).
- The extra page is not pointless - as you can see, it is allocated when
exceeding EFI page size, which is the only granularity you can
page-allocate at. So, when you require > 4K alignment, you need the
extra page to be able to “shift” the image into proper alignment in the
allocated region as needed. In AUDK, I resolved this by abstracting
existing, buried aligned-allocation code into a MemoryAllocationLibEx
class:
https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70>
Without aligned-allocation, you cannot get rid of this extra page.
For reference, my image record code is here:
https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335>
Yep, this was me being dumb and not looking a few lines further ahead :)
After sending this I looked deeper at this offhand comment and realized
the purpose.
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.
Why does there need to be a protocol? Why should non-Core modules get
any information about image topology?
Agreed, only core code needs this. I think I said protocol originally
so that the existing ImagePropertiesRecordLib could use it without
depending on a core only function, but in a new implementation we
would drop the lib.
A few general words of caution when dealing with PE:
- Do not trust the spec.
- Do not trust the loader.
- Do not trust the converter.
- Do *not* trust your intuition.
*Everything* is broken, from the producers to the consumers, from the
signing to the validation tools, technically, security-wise, and
design-wise. Do *not* change things without as little as a collection of
Windows and Linux images from various versions and various OpROMs to
regression-test with. Otherwise, we will all be in a world of more pain. :)
I agree with you there. That being said, the current Image record code
is, unsurprisingly, broken, and does need to be fixed. Can you review
the v2 of this? The only difference is that I changed to VirtualSize.
Do you think the code should be updated for the case of VirtualSize
== 0? In the current implementation, it will round 0 up to the
SectionAlignment, which is what I believe will happen in the PE
loader, also, but want to get your thoughts on it.
This would still be a short term solution to fix the active bug
(ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or
we will boot without the benefit of the MAT, if we happen to have
an aligned total region but not each entry, testing shows). I do
think it is a broken pattern to try to determine the image records
after we've loaded them into memory, the PE loader should create
them with the factual data of what was loaded.
Thanks,
Oliver
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116756): https://edk2.groups.io/g/devel/message/116756
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]
-=-=-=-=-=-=-=-=-=-=-=-
