[v4,20/26] x86/build: Make generated PE more spec compliant

  Currently kernel image is not fully compliant PE image, so it may
fail to boot with stricter implementations of UEFI PE loaders.

Set minimal alignments and sizes specified by PE documentation [1]
referenced by UEFI specification [2]. Align PE header to 8 bytes.

Generate PE sections dynamically. This simplifies code, since with
current implementation all of the sections needs to be defined in
header.S, where most section header fields do not hold valid values,
except for their names. Before the change, it also held flags,
but now flags depend on kernel configuration and it is simpler
to set them from build.c too.

Setup sections protection. Since we cannot fit every needed section,
set a part of protection flags dynamically during initialization.
This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.

[1] https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
[2] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf

Tested-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/boot/Makefile                  |   2 +-
 arch/x86/boot/header.S                  |  96 +--------
 arch/x86/boot/tools/build.c             | 270 +++++++++++++-----------
 drivers/firmware/efi/libstub/x86-stub.c |   7 +-
 4 files changed, 161 insertions(+), 214 deletions(-)
  

On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> Currently kernel image is not fully compliant PE image, so it may
> fail to boot with stricter implementations of UEFI PE loaders.
>
> Set minimal alignments and sizes specified by PE documentation [1]
> referenced by UEFI specification [2]. Align PE header to 8 bytes.
>
> Generate PE sections dynamically. This simplifies code, since with
> current implementation all of the sections needs to be defined in
> header.S, where most section header fields do not hold valid values,
> except for their names. Before the change, it also held flags,
> but now flags depend on kernel configuration and it is simpler
> to set them from build.c too.
>
> Setup sections protection. Since we cannot fit every needed section,
> set a part of protection flags dynamically during initialization.
> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
>
> [1] https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
> [2] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
>
> Tested-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>

I would prefer it if we didn't rewrite the build tool this way.

Having the sections in header.S in the order they appear in the binary
is rather useful, and I don't think we should manipulate the section
flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
think we need more than .text / .,data (as discussed in the other
thread on linux-efi@)

Furthermore, I had a look at the audk PE loader [0], and I think it is
being overly pedantic.

The PE/COFF spec does not require that all sections are virtually
contiguous, and it does not require that the file content is
completely covered by either the header or by a section.

So what I would prefer to do is the following:

Sections:
Idx Name          Size     VMA              Type
  0 .reloc        00000200 0000000000002000 DATA
  1 .compat       00000200 0000000000003000 DATA
  2 .text         00bee000 0000000000004000 TEXT
  3 .data         00002200 0000000000bf2000 DATA

using 4k section alignment and 512 byte file alignment, and a header
size of 0x200 as before (This requires my patch that allows the setup
header to remain unmapped when running the stub [1])

The reloc and compat payloads are placed at the end of the setup data
as before, but increased in size to 512 bytes each, and then mapped
non-1:1 into the RVA space.

This works happily with both the existing PE loader as well as the
audk one, but with the pedantic flags disabled.



[0] https://github.com/acidanthera/audk
[1] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?id=84412899c16c65af13dac305aa01a5a85e08c69e

> ---
>  arch/x86/boot/Makefile                  |   2 +-
>  arch/x86/boot/header.S                  |  96 +--------
>  arch/x86/boot/tools/build.c             | 270 +++++++++++++-----------
>  drivers/firmware/efi/libstub/x86-stub.c |   7 +-
>  4 files changed, 161 insertions(+), 214 deletions(-)
>
> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> index 9e38ffaadb5d..bed78c82238e 100644
> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>
>  SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>
> -sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
> +sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|_rodata\|z_.*\)$$/\#define ZO_\2 0x\1/p'
>
>  quiet_cmd_zoffset = ZOFFSET $@
>        cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 9fec80bc504b..07e31ddb074f 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -94,6 +94,7 @@ bugger_off_msg:
>         .set    bugger_off_msg_size, . - bugger_off_msg
>
>  #ifdef CONFIG_EFI_STUB
> +       .align 8
>  pe_header:
>         .long   PE_MAGIC
>
> @@ -107,7 +108,7 @@ coff_header:
>         .set    pe_opt_magic, PE_OPT_MAGIC_PE32PLUS
>         .word   IMAGE_FILE_MACHINE_AMD64
>  #endif
> -       .word   section_count                   # nr_sections
> +       .word   0                               # nr_sections
>         .long   0                               # TimeDateStamp
>         .long   0                               # PointerToSymbolTable
>         .long   1                               # NumberOfSymbols
> @@ -131,7 +132,7 @@ optional_header:
>         # Filled in by build.c
>         .long   0x0000                          # AddressOfEntryPoint
>
> -       .long   0x0200                          # BaseOfCode
> +       .long   0x1000                          # BaseOfCode
>  #ifdef CONFIG_X86_32
>         .long   0                               # data
>  #endif
> @@ -144,8 +145,8 @@ extra_header_fields:
>  #else
>         .quad   image_base                      # ImageBase
>  #endif
> -       .long   0x20                            # SectionAlignment
> -       .long   0x20                            # FileAlignment
> +       .long   0x1000                          # SectionAlignment
> +       .long   0x200                           # FileAlignment
>         .word   0                               # MajorOperatingSystemVersion
>         .word   0                               # MinorOperatingSystemVersion
>         .word   LINUX_EFISTUB_MAJOR_VERSION     # MajorImageVersion
> @@ -188,91 +189,14 @@ extra_header_fields:
>         .quad   0                               # CertificationTable
>         .quad   0                               # BaseRelocationTable
>
> -       # Section table
> -section_table:
> -       #
> -       # The offset & size fields are filled in by build.c.
> -       #
> -       .ascii  ".setup"
> -       .byte   0
> -       .byte   0
> -       .long   0
> -       .long   0x0                             # startup_{32,64}
> -       .long   0                               # Size of initialized data
> -                                               # on disk
> -       .long   0x0                             # startup_{32,64}
> -       .long   0                               # PointerToRelocations
> -       .long   0                               # PointerToLineNumbers
> -       .word   0                               # NumberOfRelocations
> -       .word   0                               # NumberOfLineNumbers
> -       .long   IMAGE_SCN_CNT_CODE              | \
> -               IMAGE_SCN_MEM_READ              | \
> -               IMAGE_SCN_MEM_EXECUTE           | \
> -               IMAGE_SCN_ALIGN_16BYTES         # Characteristics
> -
> -       #
> -       # The EFI application loader requires a relocation section
> -       # because EFI applications must be relocatable. The .reloc
> -       # offset & size fields are filled in by build.c.
>         #
> -       .ascii  ".reloc"
> -       .byte   0
> -       .byte   0
> -       .long   0
> -       .long   0
> -       .long   0                               # SizeOfRawData
> -       .long   0                               # PointerToRawData
> -       .long   0                               # PointerToRelocations
> -       .long   0                               # PointerToLineNumbers
> -       .word   0                               # NumberOfRelocations
> -       .word   0                               # NumberOfLineNumbers
> -       .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
> -               IMAGE_SCN_MEM_READ              | \
> -               IMAGE_SCN_MEM_DISCARDABLE       | \
> -               IMAGE_SCN_ALIGN_1BYTES          # Characteristics
> -
> -#ifdef CONFIG_EFI_MIXED
> -       #
> -       # The offset & size fields are filled in by build.c.
> +       # Section table
> +       # It is generated by build.c and here we just need
> +       # to reserve some space for sections
>         #
> -       .asciz  ".compat"
> -       .long   0
> -       .long   0x0
> -       .long   0                               # Size of initialized data
> -                                               # on disk
> -       .long   0x0
> -       .long   0                               # PointerToRelocations
> -       .long   0                               # PointerToLineNumbers
> -       .word   0                               # NumberOfRelocations
> -       .word   0                               # NumberOfLineNumbers
> -       .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
> -               IMAGE_SCN_MEM_READ              | \
> -               IMAGE_SCN_MEM_DISCARDABLE       | \
> -               IMAGE_SCN_ALIGN_1BYTES          # Characteristics
> -#endif
> +section_table:
> +       .fill 40*5, 1, 0
>
> -       #
> -       # The offset & size fields are filled in by build.c.
> -       #
> -       .ascii  ".text"
> -       .byte   0
> -       .byte   0
> -       .byte   0
> -       .long   0
> -       .long   0x0                             # startup_{32,64}
> -       .long   0                               # Size of initialized data
> -                                               # on disk
> -       .long   0x0                             # startup_{32,64}
> -       .long   0                               # PointerToRelocations
> -       .long   0                               # PointerToLineNumbers
> -       .word   0                               # NumberOfRelocations
> -       .word   0                               # NumberOfLineNumbers
> -       .long   IMAGE_SCN_CNT_CODE              | \
> -               IMAGE_SCN_MEM_READ              | \
> -               IMAGE_SCN_MEM_EXECUTE           | \
> -               IMAGE_SCN_ALIGN_16BYTES         # Characteristics
> -
> -       .set    section_count, (. - section_table) / 40
>  #endif /* CONFIG_EFI_STUB */
>
>         # Kernel attributes; used by setup.  This is part 1 of the
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index fbc5315af032..ac6159b76a13 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -61,8 +61,10 @@ uint8_t buf[SETUP_SECT_MAX*SECTOR_SIZE];
>
>  #ifdef CONFIG_EFI_MIXED
>  #define PECOFF_COMPAT_RESERVE 0x20
> +#define COMPAT_SECTION_SIZE 0x8
>  #else
>  #define PECOFF_COMPAT_RESERVE 0x0
> +#define COMPAT_SECTION_SIZE 0x0
>  #endif
>
>  #define RELOC_SECTION_SIZE 10
> @@ -117,6 +119,7 @@ static unsigned long efi_pe_entry;
>  static unsigned long efi32_pe_entry;
>  static unsigned long kernel_info;
>  static unsigned long startup_64;
> +static unsigned long _rodata;
>  static unsigned long _ehead;
>  static unsigned long _end;
>
> @@ -258,122 +261,177 @@ static void *map_output_file(const char *path, size_t size)
>
>  #ifdef CONFIG_EFI_STUB
>
> -static void update_pecoff_section_header_fields(char *section_name, uint32_t vma,
> -                                               uint32_t size, uint32_t datasz,
> -                                               uint32_t offset)
> +static unsigned int reloc_offset;
> +static unsigned int compat_offset;
> +
> +#define MAX_SECTIONS 5
> +
> +static void emit_pecoff_section(const char *section_name, unsigned int size,
> +                               unsigned int bss, unsigned int *file_offset,
> +                               unsigned int *mem_offset, uint32_t flags)
>  {
> -       unsigned int pe_header;
> +       unsigned int section_memsz, section_filesz;
> +       unsigned int name_len;
>         unsigned short num_sections;
> +       struct pe_hdr *hdr = get_pe_header(buf);
>         struct section_header *section;
>
> -       struct pe_hdr *hdr = get_pe_header(buf);
>         num_sections = get_unaligned_le16(&hdr->sections);
> -       section = get_sections(buf);
> +       if (num_sections >= MAX_SECTIONS)
> +               die("Not enough space to generate all sections");
>
> -       while (num_sections > 0) {
> -               if (strncmp(section->name, section_name, 8) == 0) {
> -                       /* section header size field */
> -                       put_unaligned_le32(size, &section->virtual_size);
> +       section = get_sections(buf) + num_sections;
>
> -                       /* section header vma field */
> -                       put_unaligned_le32(vma, &section->virtual_address);
> +       if ((size & (FILE_ALIGNMENT - 1)) || (bss & (FILE_ALIGNMENT - 1)))
> +               die("Section '%s' is improperly aligned", section_name);
>
> -                       /* section header 'size of initialised data' field */
> -                       put_unaligned_le32(datasz, &section->raw_data_size);
> +       section_memsz = round_up(size + bss, SECTION_ALIGNMENT);
> +       section_filesz = round_up(size, FILE_ALIGNMENT);
>
> -                       /* section header 'file offset' field */
> -                       put_unaligned_le32(offset, &section->data_addr);
> +       /* Zero out all section fields */
> +       memset(section, 0, sizeof(*section));
>
> -                       break;
> -               }
> -               section++;
> -               num_sections--;
> -       }
> -}
> +       name_len = strlen(section_name);
> +       if (name_len > sizeof(section->name))
> +               name_len = sizeof(section_name);
>
> -static void update_pecoff_section_header(char *section_name, uint32_t offset, uint32_t size)
> -{
> -       update_pecoff_section_header_fields(section_name, offset, size, size, offset);
> +       /* Section header size field */
> +       memcpy(section->name, section_name, name_len);
> +
> +       put_unaligned_le32(section_memsz, &section->virtual_size);
> +       put_unaligned_le32(*mem_offset, &section->virtual_address);
> +       put_unaligned_le32(section_filesz, &section->raw_data_size);
> +       put_unaligned_le32(*file_offset, &section->data_addr);
> +       put_unaligned_le32(flags, &section->flags);
> +
> +       put_unaligned_le16(num_sections + 1, &hdr->sections);
> +
> +       *mem_offset += section_memsz;
> +       *file_offset += section_filesz;
>  }
>
> -static void update_pecoff_setup_and_reloc(unsigned int size)
> +#define BASE_RVA 0x1000
> +
> +static unsigned int text_rva;
> +
> +static unsigned int update_pecoff_sections(unsigned int setup_size,
> +                                          unsigned int file_size,
> +                                          unsigned int virt_size,
> +                                          unsigned int text_size)
>  {
> -       uint32_t setup_offset = SECTOR_SIZE;
> -       uint32_t reloc_offset = size - PECOFF_RELOC_RESERVE - PECOFF_COMPAT_RESERVE;
> -#ifdef CONFIG_EFI_MIXED
> -       uint32_t compat_offset = reloc_offset + PECOFF_RELOC_RESERVE;
> -#endif
> -       uint32_t setup_size = reloc_offset - setup_offset;
> +       /* First section starts at 512 byes, after PE header */
> +       unsigned int mem_offset = BASE_RVA, file_offset = SECTOR_SIZE;
> +       unsigned int compat_size, reloc_size;
> +       unsigned int bss_size, text_rva_diff, reloc_rva;
> +       pe_opt_hdr  *opt_hdr = get_pe_opt_header(buf);
> +       struct pe_hdr *hdr = get_pe_header(buf);
> +       struct data_dirent *base_reloc;
> +
> +       if (get_unaligned_le32(&hdr->sections))
> +               die("Some sections present in PE file");
>
> -       update_pecoff_section_header(".setup", setup_offset, setup_size);
> -       update_pecoff_section_header(".reloc", reloc_offset, PECOFF_RELOC_RESERVE);
> +       reloc_size = round_up(RELOC_SECTION_SIZE, FILE_ALIGNMENT);
> +       compat_size = round_up(COMPAT_SECTION_SIZE, FILE_ALIGNMENT);
> +       virt_size = round_up(virt_size, SECTION_ALIGNMENT);
>
>         /*
> -        * Modify .reloc section contents with a single entry. The
> -        * relocation is applied to offset 10 of the relocation section.
> +        * Update sections offsets.
> +        * NOTE: Order is important
>          */
> -       put_unaligned_le32(reloc_offset + RELOC_SECTION_SIZE, &buf[reloc_offset]);
> -       put_unaligned_le32(RELOC_SECTION_SIZE, &buf[reloc_offset + 4]);
>
> +       bss_size = virt_size - file_size;
> +
> +       emit_pecoff_section(".setup", setup_size - SECTOR_SIZE, 0,
> +                           &file_offset, &mem_offset, SCN_RO |
> +                           IMAGE_SCN_CNT_INITIALIZED_DATA);
> +
> +       text_rva_diff = mem_offset - file_offset;
> +       text_rva = mem_offset;
> +       emit_pecoff_section(".text", text_size, 0,
> +                           &file_offset, &mem_offset, SCN_RX |
> +                           IMAGE_SCN_CNT_CODE);
> +
> +       /* Check that kernel sections mapping is contiguous */
> +       if (text_rva_diff != mem_offset - file_offset)
> +               die("Kernel sections mapping is wrong: %#x != %#x",
> +                   mem_offset - file_offset, text_rva_diff);
> +
> +       emit_pecoff_section(".data", file_size - text_size, bss_size,
> +                           &file_offset, &mem_offset, SCN_RW |
> +                           IMAGE_SCN_CNT_INITIALIZED_DATA);
> +
> +       reloc_offset = file_offset;
> +       reloc_rva = mem_offset;
> +       emit_pecoff_section(".reloc", reloc_size, 0,
> +                           &file_offset, &mem_offset, SCN_RW |
> +                           IMAGE_SCN_CNT_INITIALIZED_DATA |
> +                           IMAGE_SCN_MEM_DISCARDABLE);
> +
> +       compat_offset = file_offset;
>  #ifdef CONFIG_EFI_MIXED
> -       update_pecoff_section_header(".compat", compat_offset, PECOFF_COMPAT_RESERVE);
> +       emit_pecoff_section(".comat", compat_size, 0,
> +                           &file_offset, &mem_offset, SCN_RW |
> +                           IMAGE_SCN_CNT_INITIALIZED_DATA |
> +                           IMAGE_SCN_MEM_DISCARDABLE);
> +#endif
>
> +       if (file_size + setup_size + reloc_size + compat_size != file_offset)
> +               die("file_size(%#x) != filesz(%#x)",
> +                   file_size + setup_size + reloc_size + compat_size, file_offset);
> +
> +       /* Size of code. */
> +       put_unaligned_le32(round_up(text_size, SECTION_ALIGNMENT), &opt_hdr->text_size);
>         /*
> -        * Put the IA-32 machine type (0x14c) and the associated entry point
> -        * address in the .compat section, so loaders can figure out which other
> -        * execution modes this image supports.
> +        * Size of data.
> +        * Exclude text size and first sector, which contains PE header.
>          */
> -       buf[compat_offset] = 0x1;
> -       buf[compat_offset + 1] = 0x8;
> -       put_unaligned_le16(IMAGE_FILE_MACHINE_I386, &buf[compat_offset + 2]);
> -       put_unaligned_le32(efi32_pe_entry + size, &buf[compat_offset + 4]);
> -#endif
> -}
> +       put_unaligned_le32(mem_offset - round_up(text_size, SECTION_ALIGNMENT),
> +                          &opt_hdr->data_size);
>
> -static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int text_sz,
> -                              unsigned int init_sz)
> -{
> -       unsigned int file_sz = text_start + text_sz;
> -       unsigned int bss_sz = init_sz - file_sz;
> -       pe_opt_hdr *hdr = get_pe_opt_header(buf);
> +       /* Size of image. */
> +       put_unaligned_le32(mem_offset, &opt_hdr->image_size);
>
>         /*
> -        * The PE/COFF loader may load the image at an address which is
> -        * misaligned with respect to the kernel_alignment field in the setup
> -        * header.
> -        *
> -        * In order to avoid relocating the kernel to correct the misalignment,
> -        * add slack to allow the buffer to be aligned within the declared size
> -        * of the image.
> +        * Address of entry point for PE/COFF executable
>          */
> -       bss_sz  += CONFIG_PHYSICAL_ALIGN;
> -       init_sz += CONFIG_PHYSICAL_ALIGN;
> +       put_unaligned_le32(text_rva + efi_pe_entry, &opt_hdr->entry_point);
>
>         /*
> -        * Size of code: Subtract the size of the first sector (512 bytes)
> -        * which includes the header.
> +        * BaseOfCode for PE/COFF executable
>          */
> -       put_unaligned_le32(file_sz - SECTOR_SIZE + bss_sz, &hdr->text_size);
> -
> -       /* Size of image */
> -       put_unaligned_le32(init_sz, &hdr->image_size);
> +       put_unaligned_le32(text_rva, &opt_hdr->code_base);
>
>         /*
> -        * Address of entry point for PE/COFF executable
> +        * Since we have generated .reloc section, we need to
> +        * fill-in Reloc directory
>          */
> -       put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
> +       base_reloc = &get_data_dirs(buf)->base_relocations;
> +       put_unaligned_le32(reloc_rva, &base_reloc->virtual_address);
> +       put_unaligned_le32(RELOC_SECTION_SIZE, &base_reloc->size);
>
> -       update_pecoff_section_header_fields(".text", text_start, text_sz + bss_sz,
> -                                           text_sz, text_start);
> -
> -       return text_start + file_sz;
> +       return file_offset;
>  }
>
> -static int reserve_pecoff_reloc_section(int c)
> +static void generate_pecoff_section_data(uint8_t *output)
>  {
> -       /* Reserve 0x20 bytes for .reloc section */
> -       memset(buf+c, 0, PECOFF_RELOC_RESERVE);
> -       return PECOFF_RELOC_RESERVE;
> +       /*
> +        * Modify .reloc section contents with a two entries. The
> +        * relocation is applied to offset 10 of the relocation section.
> +        */
> +       put_unaligned_le32(reloc_offset + RELOC_SECTION_SIZE, &output[reloc_offset]);
> +       put_unaligned_le32(RELOC_SECTION_SIZE, &output[reloc_offset + 4]);
> +
> +#ifdef CONFIG_EFI_MIXED
> +       /*
> +        * Put the IA-32 machine type (0x14c) and the associated entry point
> +        * address in the .compat section, so loaders can figure out which other
> +        * execution modes this image supports.
> +        */
> +       output[compat_offset] = 0x1;
> +       output[compat_offset + 1] = 0x8;
> +       put_unaligned_le16(IMAGE_FILE_MACHINE_I386, &output[compat_offset + 2]);
> +       put_unaligned_le32(efi32_pe_entry + text_rva, &output[compat_offset + 4]);
> +#endif
>  }
>
>  static void efi_stub_update_defaults(void)
> @@ -407,26 +465,10 @@ static void efi_stub_entry_update(void)
>
>  #else
>
> -static inline void update_pecoff_setup_and_reloc(unsigned int size) {}
> -static inline void update_pecoff_text(unsigned int text_start,
> -                                     unsigned int file_sz,
> -                                     unsigned int init_sz) {}
> -static inline void efi_stub_update_defaults(void) {}
> -static inline void efi_stub_entry_update(void) {}
> +static void efi_stub_update_defaults(void) {}
>
> -static inline int reserve_pecoff_reloc_section(int c)
> -{
> -       return 0;
> -}
>  #endif /* CONFIG_EFI_STUB */
>
> -static int reserve_pecoff_compat_section(int c)
> -{
> -       /* Reserve 0x20 bytes for .compat section */
> -       memset(buf+c, 0, PECOFF_COMPAT_RESERVE);
> -       return PECOFF_COMPAT_RESERVE;
> -}
> -
>  /*
>   * Parse zoffset.h and find the entry points. We could just #include zoffset.h
>   * but that would mean tools/build would have to be rebuilt every time. It's
> @@ -456,6 +498,7 @@ static void parse_zoffset(char *fname)
>                 PARSE_ZOFS(p, efi32_pe_entry);
>                 PARSE_ZOFS(p, kernel_info);
>                 PARSE_ZOFS(p, startup_64);
> +               PARSE_ZOFS(p, _rodata);
>                 PARSE_ZOFS(p, _ehead);
>                 PARSE_ZOFS(p, _end);
>
> @@ -489,10 +532,6 @@ static unsigned int read_setup(char *path)
>
>         fclose(file);
>
> -       /* Reserve space for PE sections */
> -       file_size += reserve_pecoff_compat_section(file_size);
> -       file_size += reserve_pecoff_reloc_section(file_size);
> -
>         /* Pad unused space with zeros */
>
>         setup_size = round_up(file_size, SECTOR_SIZE);
> @@ -515,7 +554,6 @@ int main(int argc, char **argv)
>         size_t kern_file_size;
>         unsigned int setup_size;
>         unsigned int setup_sectors;
> -       unsigned int init_size;
>         unsigned int total_size;
>         unsigned int kern_size;
>         void *kernel;
> @@ -540,8 +578,7 @@ int main(int argc, char **argv)
>
>  #ifdef CONFIG_EFI_STUB
>         /* PE specification require 512-byte minimum section file alignment */
> -       kern_size = round_up(kern_file_size + 4, SECTOR_SIZE);
> -       update_pecoff_setup_and_reloc(setup_size);
> +       kern_size = round_up(kern_file_size + 4, FILE_ALIGNMENT);
>  #else
>         /* Number of 16-byte paragraphs, including space for a 4-byte CRC */
>         kern_size = round_up(kern_file_size + 4, PARAGRAPH_SIZE);
> @@ -554,33 +591,12 @@ int main(int argc, char **argv)
>         /* Update kernel_info offset. */
>         put_unaligned_le32(kernel_info, &buf[0x268]);
>
> -       init_size = get_unaligned_le32(&buf[0x260]);
> -
>  #ifdef CONFIG_EFI_STUB
> -       /*
> -        * The decompression buffer will start at ImageBase. When relocating
> -        * the compressed kernel to its end, we must ensure that the head
> -        * section does not get overwritten.  The head section occupies
> -        * [i, i + _ehead), and the destination is [init_sz - _end, init_sz).
> -        *
> -        * At present these should never overlap, because 'i' is at most 32k
> -        * because of SETUP_SECT_MAX, '_ehead' is less than 1k, and the
> -        * calculation of INIT_SIZE in boot/header.S ensures that
> -        * 'init_sz - _end' is at least 64k.
> -        *
> -        * For future-proofing, increase init_sz if necessary.
> -        */
> -
> -       if (init_size - _end < setup_size + _ehead) {
> -               init_size = round_up(setup_size + _ehead + _end, SECTION_ALIGNMENT);
> -               put_unaligned_le32(init_size, &buf[0x260]);
> -       }
>
> -       total_size = update_pecoff_sections(setup_size, kern_size, init_size);
> +       total_size = update_pecoff_sections(setup_size, kern_size, _end, _rodata);
>
>         efi_stub_entry_update();
>  #else
> -       (void)init_size;
>         total_size = setup_size + kern_size;
>  #endif
>
> @@ -590,6 +606,10 @@ int main(int argc, char **argv)
>         memcpy(output + setup_size, kernel, kern_file_size);
>         memset(output + setup_size + kern_file_size, 0, kern_size - kern_file_size);
>
> +#ifdef CONFIG_EFI_STUB
> +       generate_pecoff_section_data(output);
> +#endif
> +
>         /* Calculate and write kernel checksum. */
>         crc = partial_crc32(output, total_size - 4, crc);
>         put_unaligned_le32(crc, &output[total_size - 4]);
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 1d1ab1911fd3..1f0a2e7075c3 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -389,8 +389,11 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>
>         hdr = &boot_params->hdr;
>
> -       /* Copy the setup header from the second sector to boot_params */
> -       memcpy(&hdr->jump, image_base + 512,
> +       /*
> +        * Copy the setup header from the second sector
> +        * (mapped to image_base + 0x1000) to boot_params
> +        */
> +       memcpy(&hdr->jump, image_base + 0x1000,
>                sizeof(struct setup_header) - offsetof(struct setup_header, jump));
>
>         /*
> --
> 2.37.4
>
  

On 2023-03-10 18:17, Ard Biesheuvel wrote:
> On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> 
>> Currently kernel image is not fully compliant PE image, so it may
>> fail to boot with stricter implementations of UEFI PE loaders.
>> 
>> Set minimal alignments and sizes specified by PE documentation [1]
>> referenced by UEFI specification [2]. Align PE header to 8 bytes.
>> 
>> Generate PE sections dynamically. This simplifies code, since with
>> current implementation all of the sections needs to be defined in
>> header.S, where most section header fields do not hold valid values,
>> except for their names. Before the change, it also held flags,
>> but now flags depend on kernel configuration and it is simpler
>> to set them from build.c too.
>> 
>> Setup sections protection. Since we cannot fit every needed section,
>> set a part of protection flags dynamically during initialization.
>> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
>> 
>> [1] 
>> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
>> [2] 
>> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
>> 
>> Tested-by: Peter Jones <pjones@redhat.com>
>> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> 
> I would prefer it if we didn't rewrite the build tool this way.
> 
> Having the sections in header.S in the order they appear in the binary
> is rather useful, and I don't think we should manipulate the section
> flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
> think we need more than .text / .,data (as discussed in the other
> thread on linux-efi@)
> 
> Furthermore, I had a look at the audk PE loader [0], and I think it is
> being overly pedantic.
> 
> The PE/COFF spec does not require that all sections are virtually
> contiguous, and it does not require that the file content is
> completely covered by either the header or by a section.
> 
> So what I would prefer to do is the following:
> 
> Sections:
> Idx Name          Size     VMA              Type
>   0 .reloc        00000200 0000000000002000 DATA
>   1 .compat       00000200 0000000000003000 DATA
>   2 .text         00bee000 0000000000004000 TEXT
>   3 .data         00002200 0000000000bf2000 DATA
> 
> using 4k section alignment and 512 byte file alignment, and a header
> size of 0x200 as before (This requires my patch that allows the setup
> header to remain unmapped when running the stub [1])
> 
> The reloc and compat payloads are placed at the end of the setup data
> as before, but increased in size to 512 bytes each, and then mapped
> non-1:1 into the RVA space.
> 
> This works happily with both the existing PE loader as well as the
> audk one, but with the pedantic flags disabled.
> 

This makes sense. I'll change this patch to use this layout and
to keep sections in headers.S before sending v5. (and I guess I'll
make the compressed kernel a part of .text). I have a few questions
though:

This layout assumes having the local copy of the bootparams as
in your RFC patches, right?

Can I keep the .rodata -- 5th section fits in the section table
without much work?

Also, why .reloc is at offset 0x2000 and not just 0x1000, is there
anything important I am missing? I understand that is cannot be 0
and should be aligned on page size, but nothing else comes to my
mind...

Thanks!
> 
> 
> [0] https://github.com/acidanthera/audk
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?id=84412899c16c65af13dac305aa01a5a85e08c69e
> 
>> ---
>>  arch/x86/boot/Makefile                  |   2 +-
>>  arch/x86/boot/header.S                  |  96 +--------
>>  arch/x86/boot/tools/build.c             | 270 
>> +++++++++++++-----------
>>  drivers/firmware/efi/libstub/x86-stub.c |   7 +-
>>  4 files changed, 161 insertions(+), 214 deletions(-)
>> 
>> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
>> index 9e38ffaadb5d..bed78c82238e 100644
>> --- a/arch/x86/boot/Makefile
>> +++ b/arch/x86/boot/Makefile
>> @@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>> 
>>  SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>> 
>> -sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] 
>> \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define 
>> ZO_\2 0x\1/p'
>> +sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] 
>> \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|_rodata\|z_.*\)$$/\#define 
>> ZO_\2 0x\1/p'
>> 
>>  quiet_cmd_zoffset = ZOFFSET $@
>>        cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
>> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>> index 9fec80bc504b..07e31ddb074f 100644
>> --- a/arch/x86/boot/header.S
>> +++ b/arch/x86/boot/header.S
>> @@ -94,6 +94,7 @@ bugger_off_msg:
>>         .set    bugger_off_msg_size, . - bugger_off_msg
>> 
>>  #ifdef CONFIG_EFI_STUB
>> +       .align 8
>>  pe_header:
>>         .long   PE_MAGIC
>> 
>> @@ -107,7 +108,7 @@ coff_header:
>>         .set    pe_opt_magic, PE_OPT_MAGIC_PE32PLUS
>>         .word   IMAGE_FILE_MACHINE_AMD64
>>  #endif
>> -       .word   section_count                   # nr_sections
>> +       .word   0                               # nr_sections
>>         .long   0                               # TimeDateStamp
>>         .long   0                               # PointerToSymbolTable
>>         .long   1                               # NumberOfSymbols
>> @@ -131,7 +132,7 @@ optional_header:
>>         # Filled in by build.c
>>         .long   0x0000                          # AddressOfEntryPoint
>> 
>> -       .long   0x0200                          # BaseOfCode
>> +       .long   0x1000                          # BaseOfCode
>>  #ifdef CONFIG_X86_32
>>         .long   0                               # data
>>  #endif
>> @@ -144,8 +145,8 @@ extra_header_fields:
>>  #else
>>         .quad   image_base                      # ImageBase
>>  #endif
>> -       .long   0x20                            # SectionAlignment
>> -       .long   0x20                            # FileAlignment
>> +       .long   0x1000                          # SectionAlignment
>> +       .long   0x200                           # FileAlignment
>>         .word   0                               # 
>> MajorOperatingSystemVersion
>>         .word   0                               # 
>> MinorOperatingSystemVersion
>>         .word   LINUX_EFISTUB_MAJOR_VERSION     # MajorImageVersion
>> @@ -188,91 +189,14 @@ extra_header_fields:
>>         .quad   0                               # CertificationTable
>>         .quad   0                               # BaseRelocationTable
>> 
>> -       # Section table
>> -section_table:
>> -       #
>> -       # The offset & size fields are filled in by build.c.
>> -       #
>> -       .ascii  ".setup"
>> -       .byte   0
>> -       .byte   0
>> -       .long   0
>> -       .long   0x0                             # startup_{32,64}
>> -       .long   0                               # Size of initialized 
>> data
>> -                                               # on disk
>> -       .long   0x0                             # startup_{32,64}
>> -       .long   0                               # PointerToRelocations
>> -       .long   0                               # PointerToLineNumbers
>> -       .word   0                               # NumberOfRelocations
>> -       .word   0                               # NumberOfLineNumbers
>> -       .long   IMAGE_SCN_CNT_CODE              | \
>> -               IMAGE_SCN_MEM_READ              | \
>> -               IMAGE_SCN_MEM_EXECUTE           | \
>> -               IMAGE_SCN_ALIGN_16BYTES         # Characteristics
>> -
>> -       #
>> -       # The EFI application loader requires a relocation section
>> -       # because EFI applications must be relocatable. The .reloc
>> -       # offset & size fields are filled in by build.c.
>>         #
>> -       .ascii  ".reloc"
>> -       .byte   0
>> -       .byte   0
>> -       .long   0
>> -       .long   0
>> -       .long   0                               # SizeOfRawData
>> -       .long   0                               # PointerToRawData
>> -       .long   0                               # PointerToRelocations
>> -       .long   0                               # PointerToLineNumbers
>> -       .word   0                               # NumberOfRelocations
>> -       .word   0                               # NumberOfLineNumbers
>> -       .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
>> -               IMAGE_SCN_MEM_READ              | \
>> -               IMAGE_SCN_MEM_DISCARDABLE       | \
>> -               IMAGE_SCN_ALIGN_1BYTES          # Characteristics
>> -
>> -#ifdef CONFIG_EFI_MIXED
>> -       #
>> -       # The offset & size fields are filled in by build.c.
>> +       # Section table
>> +       # It is generated by build.c and here we just need
>> +       # to reserve some space for sections
>>         #
>> -       .asciz  ".compat"
>> -       .long   0
>> -       .long   0x0
>> -       .long   0                               # Size of initialized 
>> data
>> -                                               # on disk
>> -       .long   0x0
>> -       .long   0                               # PointerToRelocations
>> -       .long   0                               # PointerToLineNumbers
>> -       .word   0                               # NumberOfRelocations
>> -       .word   0                               # NumberOfLineNumbers
>> -       .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
>> -               IMAGE_SCN_MEM_READ              | \
>> -               IMAGE_SCN_MEM_DISCARDABLE       | \
>> -               IMAGE_SCN_ALIGN_1BYTES          # Characteristics
>> -#endif
>> +section_table:
>> +       .fill 40*5, 1, 0
>> 
>> -       #
>> -       # The offset & size fields are filled in by build.c.
>> -       #
>> -       .ascii  ".text"
>> -       .byte   0
>> -       .byte   0
>> -       .byte   0
>> -       .long   0
>> -       .long   0x0                             # startup_{32,64}
>> -       .long   0                               # Size of initialized 
>> data
>> -                                               # on disk
>> -       .long   0x0                             # startup_{32,64}
>> -       .long   0                               # PointerToRelocations
>> -       .long   0                               # PointerToLineNumbers
>> -       .word   0                               # NumberOfRelocations
>> -       .word   0                               # NumberOfLineNumbers
>> -       .long   IMAGE_SCN_CNT_CODE              | \
>> -               IMAGE_SCN_MEM_READ              | \
>> -               IMAGE_SCN_MEM_EXECUTE           | \
>> -               IMAGE_SCN_ALIGN_16BYTES         # Characteristics
>> -
>> -       .set    section_count, (. - section_table) / 40
>>  #endif /* CONFIG_EFI_STUB */
>> 
>>         # Kernel attributes; used by setup.  This is part 1 of the
>> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
>> index fbc5315af032..ac6159b76a13 100644
>> --- a/arch/x86/boot/tools/build.c
>> +++ b/arch/x86/boot/tools/build.c
>> @@ -61,8 +61,10 @@ uint8_t buf[SETUP_SECT_MAX*SECTOR_SIZE];
>> 
>>  #ifdef CONFIG_EFI_MIXED
>>  #define PECOFF_COMPAT_RESERVE 0x20
>> +#define COMPAT_SECTION_SIZE 0x8
>>  #else
>>  #define PECOFF_COMPAT_RESERVE 0x0
>> +#define COMPAT_SECTION_SIZE 0x0
>>  #endif
>> 
>>  #define RELOC_SECTION_SIZE 10
>> @@ -117,6 +119,7 @@ static unsigned long efi_pe_entry;
>>  static unsigned long efi32_pe_entry;
>>  static unsigned long kernel_info;
>>  static unsigned long startup_64;
>> +static unsigned long _rodata;
>>  static unsigned long _ehead;
>>  static unsigned long _end;
>> 
>> @@ -258,122 +261,177 @@ static void *map_output_file(const char *path, 
>> size_t size)
>> 
>>  #ifdef CONFIG_EFI_STUB
>> 
>> -static void update_pecoff_section_header_fields(char *section_name, 
>> uint32_t vma,
>> -                                               uint32_t size, 
>> uint32_t datasz,
>> -                                               uint32_t offset)
>> +static unsigned int reloc_offset;
>> +static unsigned int compat_offset;
>> +
>> +#define MAX_SECTIONS 5
>> +
>> +static void emit_pecoff_section(const char *section_name, unsigned 
>> int size,
>> +                               unsigned int bss, unsigned int 
>> *file_offset,
>> +                               unsigned int *mem_offset, uint32_t 
>> flags)
>>  {
>> -       unsigned int pe_header;
>> +       unsigned int section_memsz, section_filesz;
>> +       unsigned int name_len;
>>         unsigned short num_sections;
>> +       struct pe_hdr *hdr = get_pe_header(buf);
>>         struct section_header *section;
>> 
>> -       struct pe_hdr *hdr = get_pe_header(buf);
>>         num_sections = get_unaligned_le16(&hdr->sections);
>> -       section = get_sections(buf);
>> +       if (num_sections >= MAX_SECTIONS)
>> +               die("Not enough space to generate all sections");
>> 
>> -       while (num_sections > 0) {
>> -               if (strncmp(section->name, section_name, 8) == 0) {
>> -                       /* section header size field */
>> -                       put_unaligned_le32(size, 
>> &section->virtual_size);
>> +       section = get_sections(buf) + num_sections;
>> 
>> -                       /* section header vma field */
>> -                       put_unaligned_le32(vma, 
>> &section->virtual_address);
>> +       if ((size & (FILE_ALIGNMENT - 1)) || (bss & (FILE_ALIGNMENT - 
>> 1)))
>> +               die("Section '%s' is improperly aligned", 
>> section_name);
>> 
>> -                       /* section header 'size of initialised data' 
>> field */
>> -                       put_unaligned_le32(datasz, 
>> &section->raw_data_size);
>> +       section_memsz = round_up(size + bss, SECTION_ALIGNMENT);
>> +       section_filesz = round_up(size, FILE_ALIGNMENT);
>> 
>> -                       /* section header 'file offset' field */
>> -                       put_unaligned_le32(offset, 
>> &section->data_addr);
>> +       /* Zero out all section fields */
>> +       memset(section, 0, sizeof(*section));
>> 
>> -                       break;
>> -               }
>> -               section++;
>> -               num_sections--;
>> -       }
>> -}
>> +       name_len = strlen(section_name);
>> +       if (name_len > sizeof(section->name))
>> +               name_len = sizeof(section_name);
>> 
>> -static void update_pecoff_section_header(char *section_name, uint32_t 
>> offset, uint32_t size)
>> -{
>> -       update_pecoff_section_header_fields(section_name, offset, 
>> size, size, offset);
>> +       /* Section header size field */
>> +       memcpy(section->name, section_name, name_len);
>> +
>> +       put_unaligned_le32(section_memsz, &section->virtual_size);
>> +       put_unaligned_le32(*mem_offset, &section->virtual_address);
>> +       put_unaligned_le32(section_filesz, &section->raw_data_size);
>> +       put_unaligned_le32(*file_offset, &section->data_addr);
>> +       put_unaligned_le32(flags, &section->flags);
>> +
>> +       put_unaligned_le16(num_sections + 1, &hdr->sections);
>> +
>> +       *mem_offset += section_memsz;
>> +       *file_offset += section_filesz;
>>  }
>> 
>> -static void update_pecoff_setup_and_reloc(unsigned int size)
>> +#define BASE_RVA 0x1000
>> +
>> +static unsigned int text_rva;
>> +
>> +static unsigned int update_pecoff_sections(unsigned int setup_size,
>> +                                          unsigned int file_size,
>> +                                          unsigned int virt_size,
>> +                                          unsigned int text_size)
>>  {
>> -       uint32_t setup_offset = SECTOR_SIZE;
>> -       uint32_t reloc_offset = size - PECOFF_RELOC_RESERVE - 
>> PECOFF_COMPAT_RESERVE;
>> -#ifdef CONFIG_EFI_MIXED
>> -       uint32_t compat_offset = reloc_offset + PECOFF_RELOC_RESERVE;
>> -#endif
>> -       uint32_t setup_size = reloc_offset - setup_offset;
>> +       /* First section starts at 512 byes, after PE header */
>> +       unsigned int mem_offset = BASE_RVA, file_offset = SECTOR_SIZE;
>> +       unsigned int compat_size, reloc_size;
>> +       unsigned int bss_size, text_rva_diff, reloc_rva;
>> +       pe_opt_hdr  *opt_hdr = get_pe_opt_header(buf);
>> +       struct pe_hdr *hdr = get_pe_header(buf);
>> +       struct data_dirent *base_reloc;
>> +
>> +       if (get_unaligned_le32(&hdr->sections))
>> +               die("Some sections present in PE file");
>> 
>> -       update_pecoff_section_header(".setup", setup_offset, 
>> setup_size);
>> -       update_pecoff_section_header(".reloc", reloc_offset, 
>> PECOFF_RELOC_RESERVE);
>> +       reloc_size = round_up(RELOC_SECTION_SIZE, FILE_ALIGNMENT);
>> +       compat_size = round_up(COMPAT_SECTION_SIZE, FILE_ALIGNMENT);
>> +       virt_size = round_up(virt_size, SECTION_ALIGNMENT);
>> 
>>         /*
>> -        * Modify .reloc section contents with a single entry. The
>> -        * relocation is applied to offset 10 of the relocation 
>> section.
>> +        * Update sections offsets.
>> +        * NOTE: Order is important
>>          */
>> -       put_unaligned_le32(reloc_offset + RELOC_SECTION_SIZE, 
>> &buf[reloc_offset]);
>> -       put_unaligned_le32(RELOC_SECTION_SIZE, &buf[reloc_offset + 
>> 4]);
>> 
>> +       bss_size = virt_size - file_size;
>> +
>> +       emit_pecoff_section(".setup", setup_size - SECTOR_SIZE, 0,
>> +                           &file_offset, &mem_offset, SCN_RO |
>> +                           IMAGE_SCN_CNT_INITIALIZED_DATA);
>> +
>> +       text_rva_diff = mem_offset - file_offset;
>> +       text_rva = mem_offset;
>> +       emit_pecoff_section(".text", text_size, 0,
>> +                           &file_offset, &mem_offset, SCN_RX |
>> +                           IMAGE_SCN_CNT_CODE);
>> +
>> +       /* Check that kernel sections mapping is contiguous */
>> +       if (text_rva_diff != mem_offset - file_offset)
>> +               die("Kernel sections mapping is wrong: %#x != %#x",
>> +                   mem_offset - file_offset, text_rva_diff);
>> +
>> +       emit_pecoff_section(".data", file_size - text_size, bss_size,
>> +                           &file_offset, &mem_offset, SCN_RW |
>> +                           IMAGE_SCN_CNT_INITIALIZED_DATA);
>> +
>> +       reloc_offset = file_offset;
>> +       reloc_rva = mem_offset;
>> +       emit_pecoff_section(".reloc", reloc_size, 0,
>> +                           &file_offset, &mem_offset, SCN_RW |
>> +                           IMAGE_SCN_CNT_INITIALIZED_DATA |
>> +                           IMAGE_SCN_MEM_DISCARDABLE);
>> +
>> +       compat_offset = file_offset;
>>  #ifdef CONFIG_EFI_MIXED
>> -       update_pecoff_section_header(".compat", compat_offset, 
>> PECOFF_COMPAT_RESERVE);
>> +       emit_pecoff_section(".comat", compat_size, 0,
>> +                           &file_offset, &mem_offset, SCN_RW |
>> +                           IMAGE_SCN_CNT_INITIALIZED_DATA |
>> +                           IMAGE_SCN_MEM_DISCARDABLE);
>> +#endif
>> 
>> +       if (file_size + setup_size + reloc_size + compat_size != 
>> file_offset)
>> +               die("file_size(%#x) != filesz(%#x)",
>> +                   file_size + setup_size + reloc_size + compat_size, 
>> file_offset);
>> +
>> +       /* Size of code. */
>> +       put_unaligned_le32(round_up(text_size, SECTION_ALIGNMENT), 
>> &opt_hdr->text_size);
>>         /*
>> -        * Put the IA-32 machine type (0x14c) and the associated entry 
>> point
>> -        * address in the .compat section, so loaders can figure out 
>> which other
>> -        * execution modes this image supports.
>> +        * Size of data.
>> +        * Exclude text size and first sector, which contains PE 
>> header.
>>          */
>> -       buf[compat_offset] = 0x1;
>> -       buf[compat_offset + 1] = 0x8;
>> -       put_unaligned_le16(IMAGE_FILE_MACHINE_I386, &buf[compat_offset 
>> + 2]);
>> -       put_unaligned_le32(efi32_pe_entry + size, &buf[compat_offset + 
>> 4]);
>> -#endif
>> -}
>> +       put_unaligned_le32(mem_offset - round_up(text_size, 
>> SECTION_ALIGNMENT),
>> +                          &opt_hdr->data_size);
>> 
>> -static unsigned int update_pecoff_sections(unsigned int text_start, 
>> unsigned int text_sz,
>> -                              unsigned int init_sz)
>> -{
>> -       unsigned int file_sz = text_start + text_sz;
>> -       unsigned int bss_sz = init_sz - file_sz;
>> -       pe_opt_hdr *hdr = get_pe_opt_header(buf);
>> +       /* Size of image. */
>> +       put_unaligned_le32(mem_offset, &opt_hdr->image_size);
>> 
>>         /*
>> -        * The PE/COFF loader may load the image at an address which 
>> is
>> -        * misaligned with respect to the kernel_alignment field in 
>> the setup
>> -        * header.
>> -        *
>> -        * In order to avoid relocating the kernel to correct the 
>> misalignment,
>> -        * add slack to allow the buffer to be aligned within the 
>> declared size
>> -        * of the image.
>> +        * Address of entry point for PE/COFF executable
>>          */
>> -       bss_sz  += CONFIG_PHYSICAL_ALIGN;
>> -       init_sz += CONFIG_PHYSICAL_ALIGN;
>> +       put_unaligned_le32(text_rva + efi_pe_entry, 
>> &opt_hdr->entry_point);
>> 
>>         /*
>> -        * Size of code: Subtract the size of the first sector (512 
>> bytes)
>> -        * which includes the header.
>> +        * BaseOfCode for PE/COFF executable
>>          */
>> -       put_unaligned_le32(file_sz - SECTOR_SIZE + bss_sz, 
>> &hdr->text_size);
>> -
>> -       /* Size of image */
>> -       put_unaligned_le32(init_sz, &hdr->image_size);
>> +       put_unaligned_le32(text_rva, &opt_hdr->code_base);
>> 
>>         /*
>> -        * Address of entry point for PE/COFF executable
>> +        * Since we have generated .reloc section, we need to
>> +        * fill-in Reloc directory
>>          */
>> -       put_unaligned_le32(text_start + efi_pe_entry, 
>> &hdr->entry_point);
>> +       base_reloc = &get_data_dirs(buf)->base_relocations;
>> +       put_unaligned_le32(reloc_rva, &base_reloc->virtual_address);
>> +       put_unaligned_le32(RELOC_SECTION_SIZE, &base_reloc->size);
>> 
>> -       update_pecoff_section_header_fields(".text", text_start, 
>> text_sz + bss_sz,
>> -                                           text_sz, text_start);
>> -
>> -       return text_start + file_sz;
>> +       return file_offset;
>>  }
>> 
>> -static int reserve_pecoff_reloc_section(int c)
>> +static void generate_pecoff_section_data(uint8_t *output)
>>  {
>> -       /* Reserve 0x20 bytes for .reloc section */
>> -       memset(buf+c, 0, PECOFF_RELOC_RESERVE);
>> -       return PECOFF_RELOC_RESERVE;
>> +       /*
>> +        * Modify .reloc section contents with a two entries. The
>> +        * relocation is applied to offset 10 of the relocation 
>> section.
>> +        */
>> +       put_unaligned_le32(reloc_offset + RELOC_SECTION_SIZE, 
>> &output[reloc_offset]);
>> +       put_unaligned_le32(RELOC_SECTION_SIZE, &output[reloc_offset + 
>> 4]);
>> +
>> +#ifdef CONFIG_EFI_MIXED
>> +       /*
>> +        * Put the IA-32 machine type (0x14c) and the associated entry 
>> point
>> +        * address in the .compat section, so loaders can figure out 
>> which other
>> +        * execution modes this image supports.
>> +        */
>> +       output[compat_offset] = 0x1;
>> +       output[compat_offset + 1] = 0x8;
>> +       put_unaligned_le16(IMAGE_FILE_MACHINE_I386, 
>> &output[compat_offset + 2]);
>> +       put_unaligned_le32(efi32_pe_entry + text_rva, 
>> &output[compat_offset + 4]);
>> +#endif
>>  }
>> 
>>  static void efi_stub_update_defaults(void)
>> @@ -407,26 +465,10 @@ static void efi_stub_entry_update(void)
>> 
>>  #else
>> 
>> -static inline void update_pecoff_setup_and_reloc(unsigned int size) 
>> {}
>> -static inline void update_pecoff_text(unsigned int text_start,
>> -                                     unsigned int file_sz,
>> -                                     unsigned int init_sz) {}
>> -static inline void efi_stub_update_defaults(void) {}
>> -static inline void efi_stub_entry_update(void) {}
>> +static void efi_stub_update_defaults(void) {}
>> 
>> -static inline int reserve_pecoff_reloc_section(int c)
>> -{
>> -       return 0;
>> -}
>>  #endif /* CONFIG_EFI_STUB */
>> 
>> -static int reserve_pecoff_compat_section(int c)
>> -{
>> -       /* Reserve 0x20 bytes for .compat section */
>> -       memset(buf+c, 0, PECOFF_COMPAT_RESERVE);
>> -       return PECOFF_COMPAT_RESERVE;
>> -}
>> -
>>  /*
>>   * Parse zoffset.h and find the entry points. We could just #include 
>> zoffset.h
>>   * but that would mean tools/build would have to be rebuilt every 
>> time. It's
>> @@ -456,6 +498,7 @@ static void parse_zoffset(char *fname)
>>                 PARSE_ZOFS(p, efi32_pe_entry);
>>                 PARSE_ZOFS(p, kernel_info);
>>                 PARSE_ZOFS(p, startup_64);
>> +               PARSE_ZOFS(p, _rodata);
>>                 PARSE_ZOFS(p, _ehead);
>>                 PARSE_ZOFS(p, _end);
>> 
>> @@ -489,10 +532,6 @@ static unsigned int read_setup(char *path)
>> 
>>         fclose(file);
>> 
>> -       /* Reserve space for PE sections */
>> -       file_size += reserve_pecoff_compat_section(file_size);
>> -       file_size += reserve_pecoff_reloc_section(file_size);
>> -
>>         /* Pad unused space with zeros */
>> 
>>         setup_size = round_up(file_size, SECTOR_SIZE);
>> @@ -515,7 +554,6 @@ int main(int argc, char **argv)
>>         size_t kern_file_size;
>>         unsigned int setup_size;
>>         unsigned int setup_sectors;
>> -       unsigned int init_size;
>>         unsigned int total_size;
>>         unsigned int kern_size;
>>         void *kernel;
>> @@ -540,8 +578,7 @@ int main(int argc, char **argv)
>> 
>>  #ifdef CONFIG_EFI_STUB
>>         /* PE specification require 512-byte minimum section file 
>> alignment */
>> -       kern_size = round_up(kern_file_size + 4, SECTOR_SIZE);
>> -       update_pecoff_setup_and_reloc(setup_size);
>> +       kern_size = round_up(kern_file_size + 4, FILE_ALIGNMENT);
>>  #else
>>         /* Number of 16-byte paragraphs, including space for a 4-byte 
>> CRC */
>>         kern_size = round_up(kern_file_size + 4, PARAGRAPH_SIZE);
>> @@ -554,33 +591,12 @@ int main(int argc, char **argv)
>>         /* Update kernel_info offset. */
>>         put_unaligned_le32(kernel_info, &buf[0x268]);
>> 
>> -       init_size = get_unaligned_le32(&buf[0x260]);
>> -
>>  #ifdef CONFIG_EFI_STUB
>> -       /*
>> -        * The decompression buffer will start at ImageBase. When 
>> relocating
>> -        * the compressed kernel to its end, we must ensure that the 
>> head
>> -        * section does not get overwritten.  The head section 
>> occupies
>> -        * [i, i + _ehead), and the destination is [init_sz - _end, 
>> init_sz).
>> -        *
>> -        * At present these should never overlap, because 'i' is at 
>> most 32k
>> -        * because of SETUP_SECT_MAX, '_ehead' is less than 1k, and 
>> the
>> -        * calculation of INIT_SIZE in boot/header.S ensures that
>> -        * 'init_sz - _end' is at least 64k.
>> -        *
>> -        * For future-proofing, increase init_sz if necessary.
>> -        */
>> -
>> -       if (init_size - _end < setup_size + _ehead) {
>> -               init_size = round_up(setup_size + _ehead + _end, 
>> SECTION_ALIGNMENT);
>> -               put_unaligned_le32(init_size, &buf[0x260]);
>> -       }
>> 
>> -       total_size = update_pecoff_sections(setup_size, kern_size, 
>> init_size);
>> +       total_size = update_pecoff_sections(setup_size, kern_size, 
>> _end, _rodata);
>> 
>>         efi_stub_entry_update();
>>  #else
>> -       (void)init_size;
>>         total_size = setup_size + kern_size;
>>  #endif
>> 
>> @@ -590,6 +606,10 @@ int main(int argc, char **argv)
>>         memcpy(output + setup_size, kernel, kern_file_size);
>>         memset(output + setup_size + kern_file_size, 0, kern_size - 
>> kern_file_size);
>> 
>> +#ifdef CONFIG_EFI_STUB
>> +       generate_pecoff_section_data(output);
>> +#endif
>> +
>>         /* Calculate and write kernel checksum. */
>>         crc = partial_crc32(output, total_size - 4, crc);
>>         put_unaligned_le32(crc, &output[total_size - 4]);
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c 
>> b/drivers/firmware/efi/libstub/x86-stub.c
>> index 1d1ab1911fd3..1f0a2e7075c3 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -389,8 +389,11 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t 
>> handle,
>> 
>>         hdr = &boot_params->hdr;
>> 
>> -       /* Copy the setup header from the second sector to boot_params 
>> */
>> -       memcpy(&hdr->jump, image_base + 512,
>> +       /*
>> +        * Copy the setup header from the second sector
>> +        * (mapped to image_base + 0x1000) to boot_params
>> +        */
>> +       memcpy(&hdr->jump, image_base + 0x1000,
>>                sizeof(struct setup_header) - offsetof(struct 
>> setup_header, jump));
>> 
>>         /*
>> --
>> 2.37.4
>>
  

On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-03-10 18:17, Ard Biesheuvel wrote:
> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
> >>
> >> Currently kernel image is not fully compliant PE image, so it may
> >> fail to boot with stricter implementations of UEFI PE loaders.
> >>
> >> Set minimal alignments and sizes specified by PE documentation [1]
> >> referenced by UEFI specification [2]. Align PE header to 8 bytes.
> >>
> >> Generate PE sections dynamically. This simplifies code, since with
> >> current implementation all of the sections needs to be defined in
> >> header.S, where most section header fields do not hold valid values,
> >> except for their names. Before the change, it also held flags,
> >> but now flags depend on kernel configuration and it is simpler
> >> to set them from build.c too.
> >>
> >> Setup sections protection. Since we cannot fit every needed section,
> >> set a part of protection flags dynamically during initialization.
> >> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
> >>
> >> [1]
> >> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
> >> [2]
> >> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
> >>
> >> Tested-by: Peter Jones <pjones@redhat.com>
> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> >
> > I would prefer it if we didn't rewrite the build tool this way.
> >
> > Having the sections in header.S in the order they appear in the binary
> > is rather useful, and I don't think we should manipulate the section
> > flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
> > think we need more than .text / .,data (as discussed in the other
> > thread on linux-efi@)
> >
> > Furthermore, I had a look at the audk PE loader [0], and I think it is
> > being overly pedantic.
> >
> > The PE/COFF spec does not require that all sections are virtually
> > contiguous, and it does not require that the file content is
> > completely covered by either the header or by a section.
> >
> > So what I would prefer to do is the following:
> >
> > Sections:
> > Idx Name          Size     VMA              Type
> >   0 .reloc        00000200 0000000000002000 DATA
> >   1 .compat       00000200 0000000000003000 DATA
> >   2 .text         00bee000 0000000000004000 TEXT
> >   3 .data         00002200 0000000000bf2000 DATA
> >
> > using 4k section alignment and 512 byte file alignment, and a header
> > size of 0x200 as before (This requires my patch that allows the setup
> > header to remain unmapped when running the stub [1])
> >
> > The reloc and compat payloads are placed at the end of the setup data
> > as before, but increased in size to 512 bytes each, and then mapped
> > non-1:1 into the RVA space.
> >
> > This works happily with both the existing PE loader as well as the
> > audk one, but with the pedantic flags disabled.
> >
>
> This makes sense. I'll change this patch to use this layout and
> to keep sections in headers.S before sending v5. (and I guess I'll
> make the compressed kernel a part of .text). I have a few questions
> though:
>
> This layout assumes having the local copy of the bootparams as
> in your RFC patches, right?
>

Indeed. Otherwise, the setup header may not have been copied to memory
by the loader.

> Can I keep the .rodata -- 5th section fits in the section table
> without much work?
>

You could, but at least the current PE/COFF loader in EDK2 will map it
read/write, as it only distinguishes between executable sections and
non-executable sections.

> Also, why .reloc is at offset 0x2000 and not just 0x1000, is there
> anything important I am missing? I understand that is cannot be 0
> and should be aligned on page size, but nothing else comes to my
> mind...
>

That was just arbitrary, because the raw allocations of reloc and
compat are also allocated towards the end. But I guess starting at
0x1000 for .reloc makes more sense so feel free to change that.
  

On 2023-03-11 20:31, Ard Biesheuvel wrote:
> On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> 
>> On 2023-03-10 18:17, Ard Biesheuvel wrote:
>> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> >>
>> >> Currently kernel image is not fully compliant PE image, so it may
>> >> fail to boot with stricter implementations of UEFI PE loaders.
>> >>
>> >> Set minimal alignments and sizes specified by PE documentation [1]
>> >> referenced by UEFI specification [2]. Align PE header to 8 bytes.
>> >>
>> >> Generate PE sections dynamically. This simplifies code, since with
>> >> current implementation all of the sections needs to be defined in
>> >> header.S, where most section header fields do not hold valid values,
>> >> except for their names. Before the change, it also held flags,
>> >> but now flags depend on kernel configuration and it is simpler
>> >> to set them from build.c too.
>> >>
>> >> Setup sections protection. Since we cannot fit every needed section,
>> >> set a part of protection flags dynamically during initialization.
>> >> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
>> >>
>> >> [1]
>> >> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
>> >> [2]
>> >> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
>> >>
>> >> Tested-by: Peter Jones <pjones@redhat.com>
>> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
>> >
>> > I would prefer it if we didn't rewrite the build tool this way.
>> >
>> > Having the sections in header.S in the order they appear in the binary
>> > is rather useful, and I don't think we should manipulate the section
>> > flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
>> > think we need more than .text / .,data (as discussed in the other
>> > thread on linux-efi@)
>> >
>> > Furthermore, I had a look at the audk PE loader [0], and I think it is
>> > being overly pedantic.
>> >
>> > The PE/COFF spec does not require that all sections are virtually
>> > contiguous, and it does not require that the file content is
>> > completely covered by either the header or by a section.
>> >
>> > So what I would prefer to do is the following:
>> >
>> > Sections:
>> > Idx Name          Size     VMA              Type
>> >   0 .reloc        00000200 0000000000002000 DATA
>> >   1 .compat       00000200 0000000000003000 DATA
>> >   2 .text         00bee000 0000000000004000 TEXT
>> >   3 .data         00002200 0000000000bf2000 DATA
>> >
>> > using 4k section alignment and 512 byte file alignment, and a header
>> > size of 0x200 as before (This requires my patch that allows the setup
>> > header to remain unmapped when running the stub [1])
>> >
>> > The reloc and compat payloads are placed at the end of the setup data
>> > as before, but increased in size to 512 bytes each, and then mapped
>> > non-1:1 into the RVA space.
>> >
>> > This works happily with both the existing PE loader as well as the
>> > audk one, but with the pedantic flags disabled.
>> >
>> 
>> This makes sense. I'll change this patch to use this layout and
>> to keep sections in headers.S before sending v5. (and I guess I'll
>> make the compressed kernel a part of .text). I have a few questions
>> though:
>> 
>> This layout assumes having the local copy of the bootparams as
>> in your RFC patches, right?
>> 
> 
> Indeed. Otherwise, the setup header may not have been copied to memory
> by the loader.
> 
>> Can I keep the .rodata -- 5th section fits in the section table
>> without much work?
>> 
> 
> You could, but at least the current PE/COFF loader in EDK2 will map it
> read/write, as it only distinguishes between executable sections and
> non-executable sections.
> 

At least it will slightly improve security for some implementations
(e.g. audk, while being overly strict support RO sections)

>> Also, why .reloc is at offset 0x2000 and not just 0x1000, is there
>> anything important I am missing? I understand that is cannot be 0
>> and should be aligned on page size, but nothing else comes to my
>> mind...
>> 
> 
> That was just arbitrary, because the raw allocations of reloc and
> compat are also allocated towards the end. But I guess starting at
> 0x1000 for .reloc makes more sense so feel free to change that.

Thanks for clarifications!
  

On Sun, 12 Mar 2023 at 13:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-03-11 20:31, Ard Biesheuvel wrote:
> > On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
> >>
> >> On 2023-03-10 18:17, Ard Biesheuvel wrote:
> >> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
> >> >>
> >> >> Currently kernel image is not fully compliant PE image, so it may
> >> >> fail to boot with stricter implementations of UEFI PE loaders.
> >> >>
> >> >> Set minimal alignments and sizes specified by PE documentation [1]
> >> >> referenced by UEFI specification [2]. Align PE header to 8 bytes.
> >> >>
> >> >> Generate PE sections dynamically. This simplifies code, since with
> >> >> current implementation all of the sections needs to be defined in
> >> >> header.S, where most section header fields do not hold valid values,
> >> >> except for their names. Before the change, it also held flags,
> >> >> but now flags depend on kernel configuration and it is simpler
> >> >> to set them from build.c too.
> >> >>
> >> >> Setup sections protection. Since we cannot fit every needed section,
> >> >> set a part of protection flags dynamically during initialization.
> >> >> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
> >> >>
> >> >> [1]
> >> >> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
> >> >> [2]
> >> >> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
> >> >>
> >> >> Tested-by: Peter Jones <pjones@redhat.com>
> >> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> >> >
> >> > I would prefer it if we didn't rewrite the build tool this way.
> >> >
> >> > Having the sections in header.S in the order they appear in the binary
> >> > is rather useful, and I don't think we should manipulate the section
> >> > flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
> >> > think we need more than .text / .,data (as discussed in the other
> >> > thread on linux-efi@)
> >> >
> >> > Furthermore, I had a look at the audk PE loader [0], and I think it is
> >> > being overly pedantic.
> >> >
> >> > The PE/COFF spec does not require that all sections are virtually
> >> > contiguous, and it does not require that the file content is
> >> > completely covered by either the header or by a section.
> >> >
> >> > So what I would prefer to do is the following:
> >> >
> >> > Sections:
> >> > Idx Name          Size     VMA              Type
> >> >   0 .reloc        00000200 0000000000002000 DATA
> >> >   1 .compat       00000200 0000000000003000 DATA
> >> >   2 .text         00bee000 0000000000004000 TEXT
> >> >   3 .data         00002200 0000000000bf2000 DATA
> >> >
> >> > using 4k section alignment and 512 byte file alignment, and a header
> >> > size of 0x200 as before (This requires my patch that allows the setup
> >> > header to remain unmapped when running the stub [1])
> >> >
> >> > The reloc and compat payloads are placed at the end of the setup data
> >> > as before, but increased in size to 512 bytes each, and then mapped
> >> > non-1:1 into the RVA space.
> >> >
> >> > This works happily with both the existing PE loader as well as the
> >> > audk one, but with the pedantic flags disabled.
> >> >
> >>
> >> This makes sense. I'll change this patch to use this layout and
> >> to keep sections in headers.S before sending v5. (and I guess I'll
> >> make the compressed kernel a part of .text). I have a few questions
> >> though:
> >>
> >> This layout assumes having the local copy of the bootparams as
> >> in your RFC patches, right?
> >>
> >
> > Indeed. Otherwise, the setup header may not have been copied to memory
> > by the loader.
> >
> >> Can I keep the .rodata -- 5th section fits in the section table
> >> without much work?
> >>
> >
> > You could, but at least the current PE/COFF loader in EDK2 will map it
> > read/write, as it only distinguishes between executable sections and
> > non-executable sections.
> >
>
> At least it will slightly improve security for some implementations
> (e.g. audk, while being overly strict support RO sections)
>

Yeah, but more common loaders will put the compressed data in a
writable region. I'd prefer to have a simple and common baseline where
we always just use R-X for all text and rodata, and RW- for everything
else.
  

On 2023-03-12 16:09, Ard Biesheuvel wrote:
> On Sun, 12 Mar 2023 at 13:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> 
>> On 2023-03-11 20:31, Ard Biesheuvel wrote:
>> > On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> >>
>> >> On 2023-03-10 18:17, Ard Biesheuvel wrote:
>> >> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> >> >>
>> >> >> Currently kernel image is not fully compliant PE image, so it may
>> >> >> fail to boot with stricter implementations of UEFI PE loaders.
>> >> >>
>> >> >> Set minimal alignments and sizes specified by PE documentation [1]
>> >> >> referenced by UEFI specification [2]. Align PE header to 8 bytes.
>> >> >>
>> >> >> Generate PE sections dynamically. This simplifies code, since with
>> >> >> current implementation all of the sections needs to be defined in
>> >> >> header.S, where most section header fields do not hold valid values,
>> >> >> except for their names. Before the change, it also held flags,
>> >> >> but now flags depend on kernel configuration and it is simpler
>> >> >> to set them from build.c too.
>> >> >>
>> >> >> Setup sections protection. Since we cannot fit every needed section,
>> >> >> set a part of protection flags dynamically during initialization.
>> >> >> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set.
>> >> >>
>> >> >> [1]
>> >> >> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx
>> >> >> [2]
>> >> >> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf
>> >> >>
>> >> >> Tested-by: Peter Jones <pjones@redhat.com>
>> >> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
>> >> >
>> >> > I would prefer it if we didn't rewrite the build tool this way.
>> >> >
>> >> > Having the sections in header.S in the order they appear in the binary
>> >> > is rather useful, and I don't think we should manipulate the section
>> >> > flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't
>> >> > think we need more than .text / .,data (as discussed in the other
>> >> > thread on linux-efi@)
>> >> >
>> >> > Furthermore, I had a look at the audk PE loader [0], and I think it is
>> >> > being overly pedantic.
>> >> >
>> >> > The PE/COFF spec does not require that all sections are virtually
>> >> > contiguous, and it does not require that the file content is
>> >> > completely covered by either the header or by a section.
>> >> >
>> >> > So what I would prefer to do is the following:
>> >> >
>> >> > Sections:
>> >> > Idx Name          Size     VMA              Type
>> >> >   0 .reloc        00000200 0000000000002000 DATA
>> >> >   1 .compat       00000200 0000000000003000 DATA
>> >> >   2 .text         00bee000 0000000000004000 TEXT
>> >> >   3 .data         00002200 0000000000bf2000 DATA
>> >> >
>> >> > using 4k section alignment and 512 byte file alignment, and a header
>> >> > size of 0x200 as before (This requires my patch that allows the setup
>> >> > header to remain unmapped when running the stub [1])
>> >> >
>> >> > The reloc and compat payloads are placed at the end of the setup data
>> >> > as before, but increased in size to 512 bytes each, and then mapped
>> >> > non-1:1 into the RVA space.
>> >> >
>> >> > This works happily with both the existing PE loader as well as the
>> >> > audk one, but with the pedantic flags disabled.
>> >> >
>> >>
>> >> This makes sense. I'll change this patch to use this layout and
>> >> to keep sections in headers.S before sending v5. (and I guess I'll
>> >> make the compressed kernel a part of .text). I have a few questions
>> >> though:
>> >>
>> >> This layout assumes having the local copy of the bootparams as
>> >> in your RFC patches, right?
>> >>
>> >
>> > Indeed. Otherwise, the setup header may not have been copied to memory
>> > by the loader.
>> >
>> >> Can I keep the .rodata -- 5th section fits in the section table
>> >> without much work?
>> >>
>> >
>> > You could, but at least the current PE/COFF loader in EDK2 will map it
>> > read/write, as it only distinguishes between executable sections and
>> > non-executable sections.
>> >
>> 
>> At least it will slightly improve security for some implementations
>> (e.g. audk, while being overly strict support RO sections)
>> 
> 
> Yeah, but more common loaders will put the compressed data in a
> writable region. I'd prefer to have a simple and common baseline where
> we always just use R-X for all text and rodata, and RW- for everything
> else.

Hmm... I'll remove the .rodata for now then. If anything changes I can
always submit it as a separate patch later anyways.