Re: [PATCH 1/2] mkimage: avoid copying relocations for sections that won't be copied.

[Vladimir 'phcoder' Serbinenko]

Patch looks good. I'm sick right now. I'll commit in few days when I feel better unless someone commits it before

Le mer. 31 janv. 2018 à 17:28, Peter Jones <address@hidden> a écrit :

Some versions of gcc include a plugin called "annobin", and in some
build systems this is enabled by default.  This plugin creates special
ELF note sections to track which ABI-breaking features are used by a
binary, as well as a series of relocations to annotate where.

If grub is compiled with this feature, then when grub-mkimage translates
the binary to another file format which does not strongly associate
relocation data with sections (i.e. when platform is *-efi), these
relocations appear to be against the .text section rather than the
original note section.  When the binary is loaded by the PE runtime
loader, hilarity ensues.

This issue is not necessarily limited to the annobin, but could arise
any time there are relocations in sections that are not represented in
grub-mkimage's output.

This patch seeks to avoid this issue by only including relocations that
refer to sections which will be included in the final binary.

As an aside, this should also obviate the need to avoid -funwind-tables,
-fasynchronous-unwind-tables, and any sections similar to .eh_frame in
the future.  I've tested it on x86-64-efi with the following gcc command
line options (as recorded by -grecord-gcc-flags), but I still need to
test the result on some other platforms that have been problematic in
the past (especially ARM Aarch64) before I feel comfortable making
changes to the configure.ac bits:

GNU C11 7.2.1 20180116 (Red Hat 7.2.1-7) -mno-mmx -mno-sse -mno-sse2 -mno-sse3 -mno-3dnow -msoft-float -mno-stack-arg-probe -mcmodel=large -mno-red-zone -m64 -mtune=generic -march=x86-64 -g3 -Os -freg-struct-return -fno-stack-protector -ffreestanding -funwind-tables -fasynchronous-unwind-tables -fno-strict-aliasing -fstack-clash-protection -fno-ident -fplugin=annobin

Signed-off-by: Peter Jones <address@hidden>
---
 util/grub-mkimagexx.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 73 insertions(+), 8 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index a2bb05439f0..b016b061c8c 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -708,6 +708,13 @@ arm_get_trampoline_size (Elf_Ehdr *e,
 }
 #endif

+static int
+SUFFIX (is_kept_section) (Elf_Shdr *s, const struct grub_install_image_target_desc *image_target);
+static int
+SUFFIX (is_kept_reloc_section) (Elf_Shdr *s, const struct grub_install_image_target_desc *image_target,
+                               Elf_Shdr *sections, Elf_Half section_entsize, Elf_Half num_sections,
+                               const char *strtab);
+
 /* Deal with relocation information. This function relocates addresses
    within the virtual address space starting from 0. So only relative
    addresses can be fully resolved. Absolute addresses must be relocated
@@ -747,6 +754,14 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
        Elf_Shdr *target_section;
        Elf_Word j;

+       if (!SUFFIX (is_kept_section) (s, image_target) &&
+           !SUFFIX (is_kept_reloc_section) (s, image_target, sections, section_entsize, num_sections, strtab))
+         {
+           grub_util_info ("not translating relocations for omitted section %s",
+                       strtab + grub_le_to_cpu32 (s->sh_name));
+           continue;
+         }
+
        symtab_section = (Elf_Shdr *) ((char *) sections
                                         + (grub_target_to_host32 (s->sh_link)
                                            * section_entsize));
@@ -1656,6 +1671,13 @@ make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
        Elf_Addr section_address;
        Elf_Word j;

+       if (!SUFFIX (is_kept_reloc_section) (s, image_target, sections, section_entsize, num_sections, strtab))
+         {
+           grub_util_info ("not translating the skipped relocation section %s",
+                           strtab + grub_le_to_cpu32 (s->sh_name));
+           continue;
+         }
+
        grub_util_info ("translating the relocation section %s",
                        strtab + grub_le_to_cpu32 (s->sh_name));

@@ -1731,6 +1753,56 @@ SUFFIX (is_bss_section) (Elf_Shdr *s, const struct grub_install_image_target_des
          == SHF_ALLOC) && (grub_target_to_host32 (s->sh_type) == SHT_NOBITS);
 }

+/* Determine if a section is going to be in the final output */
+static int
+SUFFIX (is_kept_section) (Elf_Shdr *s, const struct grub_install_image_target_desc *image_target)
+{
+  /* We keep .text and .data */
+  if (SUFFIX (is_text_section) (s, image_target)
+      || SUFFIX (is_data_section) (s, image_target))
+    return 1;
+
+  /* And we keep .bss if we're producing PE binaries  or the target doesn't
+   * have a relocating loader.  Platforms other than EFI and U-boot shouldn't
+   * have .bss in their binaries as we build with -Wl,-Ttext.
+   */
+  if (SUFFIX (is_bss_section) (s, image_target)
+      && (image_target->id == IMAGE_EFI || !is_relocatable (image_target)))
+    return 1;
+
+  /* Otherwise this is not a section we're keeping in the final output. */
+  return 0;
+}
+
+static int
+SUFFIX (is_kept_reloc_section) (Elf_Shdr *s, const struct grub_install_image_target_desc *image_target,
+                               Elf_Shdr *sections, Elf_Half section_entsize, Elf_Half num_sections,
+                               const char *strtab)
+{
+  int i;
+  int r = 0;
+  const char *name = strtab + grub_host_to_target32 (s->sh_name);
+
+  if (!strncmp (name, ".rela.", 6))
+    name += 5;
+  else if (!strncmp (name, ".rel.", 5))
+    name += 4;
+  else
+    return 1;
+
+  for (i = 0, s = sections; i < num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+    {
+      const char *sname = strtab + grub_host_to_target32 (s->sh_name);
+      if (strcmp (sname, name))
+       continue;
+
+      return SUFFIX (is_kept_section) (s, image_target);
+    }
+
+  return r;
+}
+
 /* Return if the ELF header is valid.  */
 static int
 SUFFIX (check_elf_header) (Elf_Ehdr *e, size_t size, const struct grub_install_image_target_desc *image_target)
@@ -2087,14 +2159,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
   for (i = 0, s = sections;
        i < num_sections;
        i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
-    if (SUFFIX (is_data_section) (s, image_target)
-       /* Explicitly initialize BSS
-          when producing PE32 to avoid a bug in EFI implementations.
-          Platforms other than EFI and U-boot shouldn't have .bss in
-          their binaries as we build with -Wl,-Ttext.
-       */
-       || (SUFFIX (is_bss_section) (s, image_target) && (image_target->id == IMAGE_EFI || !is_relocatable (image_target)))
-       || SUFFIX (is_text_section) (s, image_target))
+    if (SUFFIX (is_kept_section) (s, image_target))
       {
        if (grub_target_to_host32 (s->sh_type) == SHT_NOBITS)
          memset (out_img + section_addresses[i], 0,
--
2.15.0


_______________________________________________
Grub-devel mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel