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

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 6092 bytes --]

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

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

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

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

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

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

Thanks!

Best regards,
Marvin

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



[-- Attachment #2: Type: text/html, Size: 7900 bytes --]