Actually this may be incomplete. When trying to boot shimx64.efi, I realised the binary's relocations are malformed, i.e. it is not ensured that relocation blocks have a size aligned by 4 Bytes as per the PE spec. While I did not have any error with my generated PE iPXE binary, I do not quite see (yet?) where in the code this condition is ensured. Should it not be around here? If the logic is indeed missing, I can probably help writing it, but testing will be hard with no malformed binary discovered so far.

Thank you.

Thank you. Some issues observed from a quick initial check of the patch:

• Please reformat code to remain within an 80-character width

• Please reflow commit messages to remain within an 80-character width

• Running objdump -x bin-x86_64-efi/ipxe.efi no longer shows meaningful information about the PE sections.

  Before:

  Sections:
  Idx Name          Size      VMA               LMA               File off  Algn
    0 .text         000a0148  0000000000001000  0000000000001000  000002c0  2**4
                    CONTENTS, ALLOC, LOAD, READONLY, CODE
    1 .rodata       0002c10e  00000000000a1160  00000000000a1160  000a0420  2**2
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    2 .data         00023f40  00000000000cd280  00000000000cd280  000cc540  2**4
                    CONTENTS, ALLOC, LOAD, DATA
    3 .bss          000a3e0c  00000000000f1200  00000000000f1200  00000000  2**4
                    ALLOC
    4 .reloc        00001a9c  0000000000195020  0000000000195020  000f0480  2**2
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    5 .debug        00000040  0000000000196ac0  0000000000196ac0  000f1f20  2**0
                    CONTENTS, READONLY, DEBUGGING
  

  After:

  Sections:
  Idx Name          Size      VMA               LMA               File off  Algn
    0               000f01a0  0000000000001000  0000000000001000  00000240  2**2
                    CONTENTS, ALLOC, LOAD, CODE
    1 .reloc        00001ac0  0000000000196000  0000000000196000  000f03e0  2**2
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    2 .debug        00000040  0000000000198000  0000000000198000  000f1ea0  2**0
                    CONTENTS, READONLY, DEBUGGING
  

Actually this may be incomplete. When trying to boot shimx64.efi, I realised the binary's relocations are malformed, i.e. it is not ensured that relocation blocks have a size aligned by 4 Bytes as per the PE spec. While I did not have any error with my generated PE iPXE binary, I do not quite see (yet?) where in the code this condition is ensured. Should it not be around here? If the logic is indeed missing, I can probably help writing it, but testing will be hard with no malformed binary discovered so far.

The existing code will pad the section filesize to 32-byte alignment and zero-fill any unused space.

PE relocation records are two bytes each, so if the PE spec requires 4-byte alignment of the size of relocation records then that seems to be a bug in the PE spec. Do you have a direct link to the relevant requirement?

From a closer look at the code, it looks as though we already ensure that the relocation section size is 4-byte aligned:

num_relocs = ( ( pe_rel->used_relocs + 1 ) & ~1 );

Thank you. Some issues observed from a quick initial check of the patch:

• Please reformat code to remain within an 80-character width
• Please reflow commit messages to remain within an 80-character width

OK, sorry.

• Running objdump -x bin-x86_64-efi/ipxe.efi no longer shows meaningful information about the PE sections.
  Before:

  Sections:
  Idx Name          Size      VMA               LMA               File off  Algn
    0 .text         000a0148  0000000000001000  0000000000001000  000002c0  2**4
                    CONTENTS, ALLOC, LOAD, READONLY, CODE
    1 .rodata       0002c10e  00000000000a1160  00000000000a1160  000a0420  2**2
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    2 .data         00023f40  00000000000cd280  00000000000cd280  000cc540  2**4
                    CONTENTS, ALLOC, LOAD, DATA
    3 .bss          000a3e0c  00000000000f1200  00000000000f1200  00000000  2**4
                    ALLOC
    4 .reloc        00001a9c  0000000000195020  0000000000195020  000f0480  2**2
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    5 .debug        00000040  0000000000196ac0  0000000000196ac0  000f1f20  2**0
                    CONTENTS, READONLY, DEBUGGING
  

  After:

  Sections:
  Idx Name          Size      VMA               LMA               File off  Algn
    0               000f01a0  0000000000001000  0000000000001000  00000240  2**2
                    CONTENTS, ALLOC, LOAD, CODE
    1 .reloc        00001ac0  0000000000196000  0000000000196000  000f03e0  2**2
                    CONTENTS, ALLOC, LOAD, READONLY, DATA
    2 .debug        00000040  0000000000198000  0000000000198000  000f1ea0  2**0
                    CONTENTS, READONLY, DEBUGGING
  

ELF segments do not have names, so it's sort of expected. I suppose we could have heuristics for ".text", ".data", and other common ones? I dislike heuristics and lack of information equally, so I am fine with whatever you prefer.

I also just realised that this change alone will not be acceptable as the source ELF file has one rwx segment and applies more granular permissions per section. For this change to be extensive, this needs to be fixed too somehow. I will change the state to Draft and hope you have a good suggestion at hand, otherwise I will have to go back to the ELF docs. :)

The existing code will pad the section filesize to 32-byte alignment and zero-fill any unused space.

PE relocation records are two bytes each, so if the PE spec requires 4-byte alignment of the size of relocation records then that seems to be a bug in the PE spec. Do you have a direct link to the relevant requirement?

This one is funny because it's heavily implied and basically the only sensical way I see to implement it, but not strictly spelled out. The two relevant pieces are these:

• "Each block must start on a 32-bit boundary." [1]
• "IMAGE_REL_BASED_ABSOLUTE": "The base relocation is skipped. This type can be used to pad a block." [2]

While the records themselves are 2 bytes each as you say and thus only require a 2 byte alignment, the block headers have 2 records of 4 bytes each and hence require a 4 byte alignment. This can be accomplished by inserting an absolute relocation to reach an even amount of entries in a block.

The spec is very vague on this, but let's be honest, the spec is very vague on everything. I do not think there is an alternative interpretation that makes sense in context of other, similar examples.

EDIT:

From a closer look at the code, it looks as though we already ensure that the relocation section size is 4-byte aligned:

num_relocs = ( ( pe_rel->used_relocs + 1 ) & ~1 );

Aha, totally missed it because I was looking for explicit initialisation. ABSOLUTE is 0, so it is implicit. I think this resolves this matter, thank you a lot.

[1] PE format, "The .reloc Section (Image Only)" (https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#the-reloc-section-image-only)
[2] PE format, "The .reloc Section (Image Only)", "Base Relocation Types" (https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#base-relocation-block)

ELF segments do not have names, so it's sort of expected. I suppose we could have heuristics for ".text", ".data", and other common ones? I dislike heuristics and lack of information equally, so I am fine with whatever you prefer.

I would like the existing section names (as defined deliberately in efi/script.lds) to be preserved. If a future patch modifies efi/script.lds to add a new section, then that section should appear automatically in the final EFI binary with no modification to elf2efi.c required.

This avoids any kind of heuristics, and matches the existing behaviour.

As far as I'm concerned, the linker scripts are definitive to describe the intended layouts of the final binaries across all platforms (not just UEFI). The function of elf2efi.c should be to merely provide the mechanism that transcribes the policy defined in scripts/efi.lds from ELF format into PE format.

I also just realised that this change alone will not be acceptable as the source ELF file has one rwx segment and applies more granular permissions per section. For this change to be extensive, this needs to be fixed too somehow. I will change the state to Draft and hope you have a good suggestion at hand, otherwise I will have to go back to the ELF docs. :)

I would probably suggest retaining the existing behaviour of copying sections rather than segments, since that seems to already more closely match what you are aiming to achieve. Would it suffice to simply change the section alignment (_max_align) to 4kB rather than 32 bytes?

I would probably suggest retaining the existing behaviour of copying sections rather than segments, since that seems to already more closely match what you are aiming to achieve. Would it suffice to simply change the section alignment (_max_align) to 4kB rather than 32 bytes?

That would be fine by me, just I researched the ELF format a little and like the Mach-O format, a loader should not be concerned with sections (linker info) but segments. Another concern is that the PE sections must be adjacent. If both conditions are fulfilled however, I see no reason to not take your route.

I will do the edits you suggested locally and verify with fully constrained loading. If it works, I will submit a new PR. Sorry, I'm just not familiar with ELF or GNU utilities. :)

@mcb30 Sorry to bother again, but I faced an unaligned ".pci_dev" section in the resulting PE. It is neither declared nor
discarded in the LDS. Is it needed?

@mcb30 Sorry to bother again, but I faced an unaligned ".pci_dev" section in the resulting PE. It is neither declared nor
discarded in the LDS. Is it needed?

Good catch! That must be .pci_devlist truncated to the 8 characters available for PE section names. It's part of the mechanism used for BIOS PCI ROMs to build up a list of other PCI IDs supported by the same ROM image (and linked to as part of the PCI ROM header).

It's irrelevant for UEFI, where we don't have any such capability to mark a ROM as supporting multple PCI IDs. Doesn't show up in bin-x86_64-efi/ipxe.efi since "ipxe" is not a PCI vendor:device ID, but it does currently (and erroneously) show up as a section in e.g. bin-x86_64-efi/8086100e.efi.

It should be safe to add it to the discard list. If that doesn't work (e.g. because the link-time check complains about an explicitly requested symbol being discarded), then just add it in to .rodata.

@mcb30 Sorry, misclicked. Confirmed working, will submit a V2. Tyvm!

@mcb30 Sorry for the hassle, I believe I corrected everything. Thanks for the fast review!

@mhaeuser Only the author email still needed fixing, so I just did that one myself. Thanks for the improvements!