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

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

[Peter Jones]

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 <pjones@redhat.com>
---
 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

[PATCH 2/2] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Peter Jones]

This way debuginfo built from the .module will still include this
information, but the final result won't have the data we don't actually
need in the modules, either on-disk, loaded at runtime, or in prebuilt
images.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 grub-core/genmod.sh.in | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index 3de06ee018f..1250589b3f5 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -58,6 +58,10 @@ if test x@TARGET_APPLE_LINKER@ != x1; then
 		-K grub_mod_init -K grub_mod_fini \
 		-K _grub_mod_init -K _grub_mod_fini \
 		-R .note.gnu.gold-version -R .note.GNU-stack \
+		-R .gnu.build.attributes \
+		-R .rel.gnu.build.attributes \
+		-R .rela.gnu.build.attributes \
+		-R .eh_frame -R .rela.eh_frame -R .rel.eh_frame \
 		-R .note -R .comment -R .ARM.exidx $tmpfile || exit 1
 	fi
 	if ! test -z "${TARGET_OBJ2ELF}"; then
-- 
2.15.0

Re: [PATCH 2/2] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Vladimir 'phcoder' Serbinenko]

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

LGTM

Le mer. 31 janv. 2018 à 17:28, Peter Jones <pjones@redhat.com> a écrit :

> This way debuginfo built from the .module will still include this
> information, but the final result won't have the data we don't actually
> need in the modules, either on-disk, loaded at runtime, or in prebuilt
> images.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  grub-core/genmod.sh.in | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
> index 3de06ee018f..1250589b3f5 100644
> --- a/grub-core/genmod.sh.in
> +++ b/grub-core/genmod.sh.in
> @@ -58,6 +58,10 @@ if test x@TARGET_APPLE_LINKER@ != x1; then
>                 -K grub_mod_init -K grub_mod_fini \
>                 -K _grub_mod_init -K _grub_mod_fini \
>                 -R .note.gnu.gold-version -R .note.GNU-stack \
> +               -R .gnu.build.attributes \
> +               -R .rel.gnu.build.attributes \
> +               -R .rela.gnu.build.attributes \
> +               -R .eh_frame -R .rela.eh_frame -R .rel.eh_frame \
>                 -R .note -R .comment -R .ARM.exidx $tmpfile || exit 1
>         fi
>         if ! test -z "${TARGET_OBJ2ELF}"; then
> --
> 2.15.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

Re: [PATCH 2/2] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Daniel Kiper]

On Wed, Jan 31, 2018 at 11:27:00AM -0500, Peter Jones wrote:
> This way debuginfo built from the .module will still include this
> information, but the final result won't have the data we don't actually
> need in the modules, either on-disk, loaded at runtime, or in prebuilt
> images.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

I suppose that I can apply this patch without patch 1
but I would like to be sure... So?

Daniel

Re: [PATCH 2/2] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Peter Jones]

On Tue, Feb 20, 2018 at 03:51:59PM +0100, Daniel Kiper wrote:
> On Wed, Jan 31, 2018 at 11:27:00AM -0500, Peter Jones wrote:
> > This way debuginfo built from the .module will still include this
> > information, but the final result won't have the data we don't actually
> > need in the modules, either on-disk, loaded at runtime, or in prebuilt
> > images.
> >
> > Signed-off-by: Peter Jones <pjones@redhat.com>
> 
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> I suppose that I can apply this patch without patch 1
> but I would like to be sure... So?

Yes, it should be completely safe.

-- 
  Peter

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

[Peter Jones]

Anyone want to have a look at this patchset to make gcc > 7.2.1 (with
optional plugins enabled) work?

On Wed, Jan 31, 2018 at 11:26:59AM -0500, Peter Jones wrote:
> 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 <pjones@redhat.com>
> ---
>  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
> 

-- 
  Peter

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

[Vladimir 'phcoder' Serbinenko]

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

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 <pjones@redhat.com> 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 <pjones@redhat.com>
> ---
>  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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

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

[Daniel Kiper]

On Wed, Jan 31, 2018 at 11:26:59AM -0500, Peter Jones wrote:
> 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 <pjones@redhat.com>

In general I am OK with the idea but...

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

Ugh... Could not you create a struct and pass the pointer to it here?

Daniel

PS Please CC me on the patches next time.

[PATCH v2 1/3] mkimage: refactor a bunch of section data into a struct.

[Peter Jones]

This basically moves a bunch of the section information we pass around a
lot into a struct, and passes a pointer to a single one of those
instead.

Because it's more convenient, it also puts the section_vaddresses
calculations in locate_section(), which no longer returns the allocation
for section_addresses directly either.

This shouldn't change the binary file output or the "grub-mkimage -v"
output in any way.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 util/grub-mkimagexx.c | 282 ++++++++++++++++++++++----------------------------
 1 file changed, 123 insertions(+), 159 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index a2bb05439f0..5c787ec56bf 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -84,6 +84,17 @@ struct fixup_block_list
 
 #define ALIGN_ADDR(x) (ALIGN_UP((x), image_target->voidp_sizeof))
 
+struct section_metadata
+{
+  Elf_Half num_sections;
+  Elf_Shdr *sections;
+  Elf_Addr *addresses;
+  Elf_Addr *vaddresses;
+  Elf_Half section_entsize;
+  Elf_Shdr *symtab;
+  const char *strtab;
+};
+
 static int
 is_relocatable (const struct grub_install_image_target_desc *image_target)
 {
@@ -499,9 +510,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
 /* Relocate symbols; note that this function overwrites the symbol table.
    Return the address of a start symbol.  */
 static Elf_Addr
-SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
-			   Elf_Shdr *symtab_section, Elf_Addr *section_addresses,
-			   Elf_Half section_entsize, Elf_Half num_sections,
+SUFFIX (relocate_symbols) (Elf_Ehdr *e, struct section_metadata *sdata,
 			   void *jumpers, Elf_Addr jumpers_addr,
 			   Elf_Addr bss_start, Elf_Addr end,
 			   const struct grub_install_image_target_desc *image_target)
@@ -511,19 +520,18 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
   Elf_Addr start_address = (Elf_Addr) -1;
   Elf_Sym *sym;
   Elf_Word i;
-  Elf_Shdr *strtab_section;
-  const char *strtab;
+  Elf_Shdr *symtab_section;
+  const char *symtab;
   grub_uint64_t *jptr = jumpers;
 
-  strtab_section
-    = (Elf_Shdr *) ((char *) sections
-		      + (grub_target_to_host32 (symtab_section->sh_link)
-			 * section_entsize));
-  strtab = (char *) e + grub_target_to_host (strtab_section->sh_offset);
+  symtab_section = (Elf_Shdr *) ((char *) sdata->sections
+				 + grub_target_to_host32 (sdata->symtab->sh_link)
+				   * sdata->section_entsize);
+  symtab = (char *) e + grub_target_to_host (symtab_section->sh_offset);
 
-  symtab_size = grub_target_to_host (symtab_section->sh_size);
-  sym_size = grub_target_to_host (symtab_section->sh_entsize);
-  symtab_offset = grub_target_to_host (symtab_section->sh_offset);
+  symtab_size = grub_target_to_host (sdata->symtab->sh_size);
+  sym_size = grub_target_to_host (sdata->symtab->sh_entsize);
+  symtab_offset = grub_target_to_host (sdata->symtab->sh_offset);
   num_syms = symtab_size / sym_size;
 
   for (i = 0, sym = (Elf_Sym *) ((char *) e + symtab_offset);
@@ -533,7 +541,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
       Elf_Section cur_index;
       const char *name;
 
-      name = strtab + grub_target_to_host32 (sym->st_name);
+      name = symtab + grub_target_to_host32 (sym->st_name);
 
       cur_index = grub_target_to_host16 (sym->st_shndx);
       if (cur_index == STN_ABS)
@@ -551,12 +559,12 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
 	  else
 	    continue;
 	}
-      else if (cur_index >= num_sections)
+      else if (cur_index >= sdata->num_sections)
 	grub_util_error ("section %d does not exist", cur_index);
       else
 	{
 	  sym->st_value = (grub_target_to_host (sym->st_value)
-			   + section_addresses[cur_index]);
+			   + sdata->vaddresses[cur_index]);
 	}
 
       if (image_target->elf_target == EM_IA_64 && ELF_ST_TYPE (sym->st_info)
@@ -571,7 +579,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
       grub_util_info ("locating %s at 0x%"  GRUB_HOST_PRIxLONG_LONG
 		      " (0x%"  GRUB_HOST_PRIxLONG_LONG ")", name,
 		      (unsigned long long) sym->st_value,
-		      (unsigned long long) section_addresses[cur_index]);
+		      (unsigned long long) sdata->vaddresses[cur_index]);
 
       if (start_address == (Elf_Addr)-1)
 	if (strcmp (name, "_start") == 0 || strcmp (name, "start") == 0)
@@ -713,12 +721,8 @@ arm_get_trampoline_size (Elf_Ehdr *e,
    addresses can be fully resolved. Absolute addresses must be relocated
    again by a PE32 relocator when loaded.  */
 static void
-SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
-			     Elf_Addr *section_addresses,
-			     Elf_Half section_entsize, Elf_Half num_sections,
-			     const char *strtab,
-			     char *pe_target, Elf_Addr tramp_off,
-			     Elf_Addr got_off,
+SUFFIX (relocate_addresses) (Elf_Ehdr *e, struct section_metadata *sdata,
+			     char *pe_target, Elf_Addr tramp_off, Elf_Addr got_off,
 			     const struct grub_install_image_target_desc *image_target)
 {
   Elf_Half i;
@@ -732,33 +736,29 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
   grub_uint32_t *tr = (void *) (pe_target + tramp_off);
 #endif
 
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = sdata->sections;
+       i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
     if ((s->sh_type == grub_host_to_target32 (SHT_REL)) ||
         (s->sh_type == grub_host_to_target32 (SHT_RELA)))
       {
 	Elf_Rela *r;
 	Elf_Word rtab_size, r_size, num_rs;
 	Elf_Off rtab_offset;
-	Elf_Shdr *symtab_section;
 	Elf_Word target_section_index;
 	Elf_Addr target_section_addr;
 	Elf_Shdr *target_section;
 	Elf_Word j;
 
-	symtab_section = (Elf_Shdr *) ((char *) sections
-					 + (grub_target_to_host32 (s->sh_link)
-					    * section_entsize));
 	target_section_index = grub_target_to_host32 (s->sh_info);
-	target_section_addr = section_addresses[target_section_index];
-	target_section = (Elf_Shdr *) ((char *) sections
+	target_section_addr = sdata->addresses[target_section_index];
+	target_section = (Elf_Shdr *) ((char *) sdata->sections
 					 + (target_section_index
-					    * section_entsize));
+					    * sdata->section_entsize));
 
 	grub_util_info ("dealing with the relocation section %s for %s",
-			strtab + grub_target_to_host32 (s->sh_name),
-			strtab + grub_target_to_host32 (target_section->sh_name));
+			sdata->strtab + grub_target_to_host32 (s->sh_name),
+			sdata->strtab + grub_target_to_host32 (target_section->sh_name));
 
 	rtab_size = grub_target_to_host (s->sh_size);
 	r_size = grub_target_to_host (s->sh_entsize);
@@ -779,7 +779,7 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
 	    target = SUFFIX (get_target_address) (e, target_section,
 						  offset, image_target);
 	    info = grub_target_to_host (r->r_info);
-	    sym_addr = SUFFIX (get_symbol_address) (e, symtab_section,
+	    sym_addr = SUFFIX (get_symbol_address) (e, sdata->symtab,
 						    ELF_R_SYM (info), image_target);
 
             addend = (s->sh_type == grub_target_to_host32 (SHT_RELA)) ?
@@ -909,8 +909,8 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
 		    Elf_Sym *sym;
 
 		    sym = (Elf_Sym *) ((char *) e
-				       + grub_target_to_host (symtab_section->sh_offset)
-				       + ELF_R_SYM (info) * grub_target_to_host (symtab_section->sh_entsize));
+				       + grub_target_to_host (sdata->symtab->sh_offset)
+				       + ELF_R_SYM (info) * grub_target_to_host (sdata->symtab->sh_entsize));
 		    if (ELF_ST_TYPE (sym->st_info) == STT_FUNC)
 		      sym_addr = grub_target_to_host64 (*(grub_uint64_t *) (pe_target
 									    + sym->st_value
@@ -1095,8 +1095,8 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
 							- (char *) e),
 				       sym_addr);
 		       sym = (Elf_Sym *) ((char *) e
-					  + grub_target_to_host (symtab_section->sh_offset)
-					  + ELF_R_SYM (info) * grub_target_to_host (symtab_section->sh_entsize));
+					  + grub_target_to_host (sdata->symtab->sh_offset)
+					  + ELF_R_SYM (info) * grub_target_to_host (sdata->symtab->sh_entsize));
 		       if (ELF_ST_TYPE (sym->st_info) != STT_FUNC)
 			 sym_addr |= 1;
 		       if (!(sym_addr & 1))
@@ -1634,9 +1634,7 @@ create_u64_fixups (struct translate_context *ctx,
 /* Make a .reloc section.  */
 static void
 make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
-		    Elf_Addr *section_addresses, Elf_Shdr *sections,
-		    Elf_Half section_entsize, Elf_Half num_sections,
-		    const char *strtab,
+		    struct section_metadata *sdata,
 		    const struct grub_install_image_target_desc *image_target)
 {
   unsigned i;
@@ -1645,8 +1643,8 @@ make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
 
   translate_reloc_start (&ctx, image_target);
 
-  for (i = 0, s = sections; i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = sdata->sections; i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
     if ((grub_target_to_host32 (s->sh_type) == SHT_REL) ||
         (grub_target_to_host32 (s->sh_type) == SHT_RELA))
       {
@@ -1657,14 +1655,14 @@ make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
 	Elf_Word j;
 
 	grub_util_info ("translating the relocation section %s",
-			strtab + grub_le_to_cpu32 (s->sh_name));
+			sdata->strtab + grub_le_to_cpu32 (s->sh_name));
 
 	rtab_size = grub_target_to_host (s->sh_size);
 	r_size = grub_target_to_host (s->sh_entsize);
 	rtab_offset = grub_target_to_host (s->sh_offset);
 	num_rs = rtab_size / r_size;
 
-	section_address = section_addresses[grub_le_to_cpu32 (s->sh_info)];
+	section_address = sdata->vaddresses[grub_le_to_cpu32 (s->sh_info)];
 
 	for (j = 0, r = (Elf_Rel *) ((char *) e + rtab_offset);
 	     j < num_rs;
@@ -1751,12 +1749,11 @@ SUFFIX (check_elf_header) (Elf_Ehdr *e, size_t size, const struct grub_install_i
 static Elf_Addr
 SUFFIX (put_section) (Elf_Shdr *s, int i,
 		      Elf_Addr current_address,
-		      Elf_Addr *section_addresses,
-		      const char *strtab,
+		      struct section_metadata *sdata,
 		      const struct grub_install_image_target_desc *image_target)
 {
 	Elf_Word align = grub_host_to_target_addr (s->sh_addralign);
-	const char *name = strtab + grub_host_to_target32 (s->sh_name);
+	const char *name = sdata->strtab + grub_host_to_target32 (s->sh_name);
 
 	if (align)
 	  current_address = ALIGN_UP (current_address + image_target->vaddr_offset,
@@ -1768,24 +1765,23 @@ SUFFIX (put_section) (Elf_Shdr *s, int i,
 			name, (unsigned long long) current_address);
 	if (!is_relocatable (image_target))
 	  current_address = grub_host_to_target_addr (s->sh_addr)
-	    - image_target->link_addr;
-	section_addresses[i] = current_address;
+			    - image_target->link_addr;
+	sdata->addresses[i] = current_address;
 	current_address += grub_host_to_target_addr (s->sh_size);
 	return current_address;
 }
 
-/* Locate section addresses by merging code sections and data sections
-   into .text and .data, respectively. Return the array of section
-   addresses.  */
-static Elf_Addr *
+/*
+ * Locate section addresses by merging code sections and data sections
+ * into .text and .data, respectively.
+ */
+static void
 SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
-			  Elf_Shdr *sections, Elf_Half section_entsize,
-			  Elf_Half num_sections, const char *strtab,
+			  struct section_metadata *sdata,
 			  struct grub_mkimage_layout *layout,
 			  const struct grub_install_image_target_desc *image_target)
 {
   int i;
-  Elf_Addr *section_addresses;
   Elf_Shdr *s;
 
   layout->align = 1;
@@ -1793,30 +1789,23 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   if (image_target->elf_target == EM_AARCH64)
     layout->align = 4096;
 
-  section_addresses = xmalloc (sizeof (*section_addresses) * num_sections);
-  memset (section_addresses, 0, sizeof (*section_addresses) * num_sections);
-
   layout->kernel_size = 0;
 
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
-    if ((grub_target_to_host (s->sh_flags) & SHF_ALLOC) 
+  for (i = 0, s = sdata->sections;
+       i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
+    if ((grub_target_to_host (s->sh_flags) & SHF_ALLOC)
 	&& grub_host_to_target32 (s->sh_addralign) > layout->align)
       layout->align = grub_host_to_target32 (s->sh_addralign);
 
-
   /* .text */
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = sdata->sections;
+       i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
     if (SUFFIX (is_text_section) (s, image_target))
       {
-	layout->kernel_size = SUFFIX (put_section) (s, i,
-						layout->kernel_size,
-						section_addresses,
-						strtab,
-						image_target);
+	layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size,
+						sdata, image_target);
 	if (!is_relocatable (image_target) &&
 	    grub_host_to_target_addr (s->sh_addr) != image_target->link_addr)
 	  {
@@ -1836,15 +1825,12 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   layout->exec_size = layout->kernel_size;
 
   /* .data */
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = sdata->sections;
+       i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
     if (SUFFIX (is_data_section) (s, image_target))
-      layout->kernel_size = SUFFIX (put_section) (s, i,
-					      layout->kernel_size,
-					      section_addresses,
-					      strtab,
-					      image_target);
+      layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, sdata,
+						  image_target);
 
 #ifdef MKIMAGE_ELF32
   if (image_target->elf_target == EM_ARM)
@@ -1855,8 +1841,8 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
 
       layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
 
-      tramp = arm_get_trampoline_size (e, sections, section_entsize,
-				       num_sections, image_target);
+      tramp = arm_get_trampoline_size (e, sdata->sections, sdata->section_entsize,
+				       sdata->num_sections, image_target);
 
       layout->tramp_off = layout->kernel_size;
       layout->kernel_size += ALIGN_UP (tramp, 16);
@@ -1867,15 +1853,16 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   layout->end = layout->kernel_size;
   
   /* .bss */
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
-    if (SUFFIX (is_bss_section) (s, image_target))
-      layout->end = SUFFIX (put_section) (s, i,
-					  layout->end,
-					  section_addresses,
-					  strtab,
-					  image_target);
+  for (i = 0, s = sdata->sections;
+       i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
+    {
+      if (SUFFIX (is_bss_section) (s, image_target))
+	layout->end = SUFFIX (put_section) (s, i, layout->end, sdata, image_target);
+
+      /* This is here just to save us another pass through the loop... */
+      sdata->vaddresses[i] = sdata->addresses[i] + image_target->vaddr_offset;
+    }
 
   layout->end = ALIGN_UP (layout->end + image_target->vaddr_offset,
 			      image_target->section_align) - image_target->vaddr_offset;
@@ -1886,8 +1873,6 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   */
   if (image_target->id == IMAGE_EFI || !is_relocatable (image_target))
     layout->kernel_size = layout->end;
-
-  return section_addresses;
 }
 
 char *
@@ -1897,18 +1882,12 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 				  const struct grub_install_image_target_desc *image_target)
 {
   char *kernel_img, *out_img;
-  const char *strtab;
+  struct section_metadata sdata = { 0, };
   Elf_Ehdr *e;
-  Elf_Shdr *sections;
-  Elf_Addr *section_addresses;
-  Elf_Addr *section_vaddresses;
   int i;
   Elf_Shdr *s;
-  Elf_Half num_sections;
   Elf_Off section_offset;
-  Elf_Half section_entsize;
   grub_size_t kernel_size;
-  Elf_Shdr *symtab_section = 0;
 
   grub_memset (layout, 0, sizeof (*layout));
 
@@ -1923,48 +1902,45 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
     grub_util_error ("invalid ELF header");
 
   section_offset = grub_target_to_host (e->e_shoff);
-  section_entsize = grub_target_to_host16 (e->e_shentsize);
-  num_sections = grub_target_to_host16 (e->e_shnum);
+  sdata.section_entsize = grub_target_to_host16 (e->e_shentsize);
+  sdata.num_sections = grub_target_to_host16 (e->e_shnum);
 
-  if (kernel_size < section_offset + (grub_uint32_t) section_entsize * num_sections)
+  if (kernel_size < section_offset
+		    + (grub_uint32_t) sdata.section_entsize * sdata.num_sections)
     grub_util_error (_("premature end of file %s"), kernel_path);
 
-  sections = (Elf_Shdr *) (kernel_img + section_offset);
+  sdata.sections = (Elf_Shdr *) (kernel_img + section_offset);
 
   /* Relocate sections then symbols in the virtual address space.  */
-  s = (Elf_Shdr *) ((char *) sections
-		      + grub_host_to_target16 (e->e_shstrndx) * section_entsize);
-  strtab = (char *) e + grub_host_to_target_addr (s->sh_offset);
+  s = (Elf_Shdr *) ((char *) sdata.sections
+		      + grub_host_to_target16 (e->e_shstrndx) * sdata.section_entsize);
+  sdata.strtab = (char *) e + grub_host_to_target_addr (s->sh_offset);
 
-  section_addresses = SUFFIX (locate_sections) (e, kernel_path,
-						sections, section_entsize,
-						num_sections, strtab,
-						layout,
-						image_target);
+  sdata.addresses = xmalloc (sizeof (*sdata.addresses) * sdata.num_sections);
+  memset (sdata.addresses, 0, sizeof (*sdata.addresses) * sdata.num_sections);
+  sdata.vaddresses = xmalloc (sizeof (*sdata.vaddresses) * sdata.num_sections);
+  memset (sdata.vaddresses, 0, sizeof (*sdata.addresses) * sdata.num_sections);
 
-  section_vaddresses = xmalloc (sizeof (*section_addresses) * num_sections);
-
-  for (i = 0; i < num_sections; i++)
-    section_vaddresses[i] = section_addresses[i] + image_target->vaddr_offset;
+  SUFFIX (locate_sections) (e, kernel_path, &sdata, layout, image_target);
 
   if (!is_relocatable (image_target))
     {
       Elf_Addr current_address = layout->kernel_size;
 
-      for (i = 0, s = sections;
-	   i < num_sections;
-	   i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+      for (i = 0, s = sdata.sections;
+	   i < sdata.num_sections;
+	   i++, s = (Elf_Shdr *) ((char *) s + sdata.section_entsize))
 	if (grub_target_to_host32 (s->sh_type) == SHT_NOBITS)
 	  {
 	    Elf_Word sec_align = grub_host_to_target_addr (s->sh_addralign);
-	    const char *name = strtab + grub_host_to_target32 (s->sh_name);
+	    const char *name = sdata.strtab + grub_host_to_target32 (s->sh_name);
 
 	    if (sec_align)
 	      current_address = ALIGN_UP (current_address
 					  + image_target->vaddr_offset,
 					  sec_align)
 		- image_target->vaddr_offset;
-	
+
 	    grub_util_info ("locating the section %s at 0x%"
 			    GRUB_HOST_PRIxLONG_LONG,
 			    name, (unsigned long long) current_address);
@@ -1972,7 +1948,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 	      current_address = grub_host_to_target_addr (s->sh_addr)
 		- image_target->link_addr;
 
-	    section_vaddresses[i] = current_address
+	    sdata.vaddresses[i] = current_address
 	      + image_target->vaddr_offset;
 	    current_address += grub_host_to_target_addr (s->sh_size);
 	  }
@@ -1993,16 +1969,16 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 
   if (is_relocatable (image_target))
     {
-      symtab_section = NULL;
-      for (i = 0, s = sections;
-	   i < num_sections;
-	   i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+      sdata.symtab = NULL;
+      for (i = 0, s = sdata.sections;
+	   i < sdata.num_sections;
+	   i++, s = (Elf_Shdr *) ((char *) s + sdata.section_entsize))
 	if (s->sh_type == grub_host_to_target32 (SHT_SYMTAB))
 	  {
-	    symtab_section = s;
+	    sdata.symtab = s;
 	    break;
 	  }
-      if (! symtab_section)
+      if (! sdata.symtab)
 	grub_util_error ("%s", _("no symbol table"));
 #ifdef MKIMAGE_ELF64
       if (image_target->elf_target == EM_IA_64)
@@ -2017,7 +1993,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 	  layout->kernel_size += ALIGN_UP (tramp, 16);
 
 	  layout->ia64jmp_off = layout->kernel_size;
-	  layout->ia64jmpnum = SUFFIX (count_funcs) (e, symtab_section,
+	  layout->ia64jmpnum = SUFFIX (count_funcs) (e, sdata.symtab,
 						     image_target);
 	  layout->kernel_size += 16 * layout->ia64jmpnum;
 
@@ -2048,31 +2024,19 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 
   if (is_relocatable (image_target))
     {
-      layout->start_address = SUFFIX (relocate_symbols) (e, sections, symtab_section,
-					  section_vaddresses, section_entsize,
-					  num_sections, 
-					  (char *) out_img + layout->ia64jmp_off, 
-					  layout->ia64jmp_off 
-					  + image_target->vaddr_offset,
-							 layout->bss_start,
-							 layout->end,
-					  image_target);
+      layout->start_address = SUFFIX (relocate_symbols) (e, &sdata,
+				  (char *) out_img + layout->ia64jmp_off,
+				  layout->ia64jmp_off + image_target->vaddr_offset,
+				  layout->bss_start, layout->end, image_target);
+
       if (layout->start_address == (Elf_Addr) -1)
 	grub_util_error ("start symbol is not defined");
 
       /* Resolve addresses in the virtual address space.  */
-      SUFFIX (relocate_addresses) (e, sections, section_addresses, 
-				   section_entsize,
-				   num_sections, strtab,
-				   out_img, layout->tramp_off,
-				   layout->got_off,
-				   image_target);
+      SUFFIX (relocate_addresses) (e, &sdata, out_img, layout->tramp_off,
+				   layout->got_off, image_target);
 
-      make_reloc_section (e, layout,
-			  section_vaddresses, sections,
-			  section_entsize, num_sections,
-			  strtab,
-			  image_target);
+      make_reloc_section (e, layout, &sdata, image_target);
       if (image_target->id != IMAGE_EFI)
 	{
 	  out_img = xrealloc (out_img, layout->kernel_size + total_module_size
@@ -2084,9 +2048,9 @@ 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))
+  for (i = 0, s = sdata.sections;
+       i < sdata.num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata.section_entsize))
     if (SUFFIX (is_data_section) (s, image_target)
 	/* Explicitly initialize BSS
 	   when producing PE32 to avoid a bug in EFI implementations.
@@ -2097,17 +2061,17 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 	|| SUFFIX (is_text_section) (s, image_target))
       {
 	if (grub_target_to_host32 (s->sh_type) == SHT_NOBITS)
-	  memset (out_img + section_addresses[i], 0,
+	  memset (out_img + sdata.addresses[i], 0,
 		  grub_host_to_target_addr (s->sh_size));
 	else
-	  memcpy (out_img + section_addresses[i],
+	  memcpy (out_img + sdata.addresses[i],
 		  kernel_img + grub_host_to_target_addr (s->sh_offset),
 		  grub_host_to_target_addr (s->sh_size));
       }
   free (kernel_img);
 
-  free (section_vaddresses);
-  free (section_addresses);
+  free (sdata.vaddresses);
+  free (sdata.addresses);
 
   return out_img;
 }
-- 
2.15.0

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

[Peter Jones]

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 <pjones@redhat.com>
---
 util/grub-mkimagexx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 5c787ec56bf..c15079c96ba 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -716,6 +716,12 @@ 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,
+				struct section_metadata *sdata);
+
 /* 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
@@ -750,6 +756,14 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, struct section_metadata *sdata,
 	Elf_Shdr *target_section;
 	Elf_Word j;
 
+	if (!SUFFIX (is_kept_section) (s, image_target) &&
+	    !SUFFIX (is_kept_reloc_section) (s, image_target, sdata))
+	  {
+	    grub_util_info ("not translating relocations for omitted section %s",
+			sdata->strtab + grub_le_to_cpu32 (s->sh_name));
+	    continue;
+	  }
+
 	target_section_index = grub_target_to_host32 (s->sh_info);
 	target_section_addr = sdata->addresses[target_section_index];
 	target_section = (Elf_Shdr *) ((char *) sdata->sections
@@ -1654,6 +1668,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, sdata))
+	  {
+	    grub_util_info ("not translating the skipped relocation section %s",
+			    sdata->strtab + grub_le_to_cpu32 (s->sh_name));
+	    continue;
+	  }
+
 	grub_util_info ("translating the relocation section %s",
 			sdata->strtab + grub_le_to_cpu32 (s->sh_name));
 
@@ -1729,6 +1750,55 @@ 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,
+				struct section_metadata *sdata)
+{
+  int i;
+  int r = 0;
+  const char *name = sdata->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 = sdata->sections; i < sdata->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + sdata->section_entsize))
+    {
+      const char *sname = sdata->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)
@@ -2051,14 +2121,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
   for (i = 0, s = sdata.sections;
        i < sdata.num_sections;
        i++, s = (Elf_Shdr *) ((char *) s + sdata.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 + sdata.addresses[i], 0,
-- 
2.15.0

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

[Daniel Kiper]

On Tue, Feb 20, 2018 at 06:25:32PM -0500, Peter Jones wrote:
> 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 <pjones@redhat.com>

One nit pick below...

Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

And +/- sdata name change...

> ---
>  util/grub-mkimagexx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 71 insertions(+), 8 deletions(-)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 5c787ec56bf..c15079c96ba 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -716,6 +716,12 @@ 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,
> +				struct section_metadata *sdata);
> +
>  /* 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
> @@ -750,6 +756,14 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, struct section_metadata *sdata,
>  	Elf_Shdr *target_section;
>  	Elf_Word j;
>
> +	if (!SUFFIX (is_kept_section) (s, image_target) &&
> +	    !SUFFIX (is_kept_reloc_section) (s, image_target, sdata))
> +	  {
> +	    grub_util_info ("not translating relocations for omitted section %s",
> +			sdata->strtab + grub_le_to_cpu32 (s->sh_name));
> +	    continue;
> +	  }
> +
>  	target_section_index = grub_target_to_host32 (s->sh_info);
>  	target_section_addr = sdata->addresses[target_section_index];
>  	target_section = (Elf_Shdr *) ((char *) sdata->sections
> @@ -1654,6 +1668,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, sdata))
> +	  {
> +	    grub_util_info ("not translating the skipped relocation section %s",
> +			    sdata->strtab + grub_le_to_cpu32 (s->sh_name));
> +	    continue;
> +	  }
> +
>  	grub_util_info ("translating the relocation section %s",
>  			sdata->strtab + grub_le_to_cpu32 (s->sh_name));
>
> @@ -1729,6 +1750,55 @@ 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

     /*
      * 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.
> +   */

Daniel

[PATCH v2 3/3] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Peter Jones]

This way debuginfo built from the .module will still include this
information, but the final result won't have the data we don't actually
need in the modules, either on-disk, loaded at runtime, or in prebuilt
images.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 grub-core/genmod.sh.in | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index 3de06ee018f..1250589b3f5 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -58,6 +58,10 @@ if test x@TARGET_APPLE_LINKER@ != x1; then
 		-K grub_mod_init -K grub_mod_fini \
 		-K _grub_mod_init -K _grub_mod_fini \
 		-R .note.gnu.gold-version -R .note.GNU-stack \
+		-R .gnu.build.attributes \
+		-R .rel.gnu.build.attributes \
+		-R .rela.gnu.build.attributes \
+		-R .eh_frame -R .rela.eh_frame -R .rel.eh_frame \
 		-R .note -R .comment -R .ARM.exidx $tmpfile || exit 1
 	fi
 	if ! test -z "${TARGET_OBJ2ELF}"; then
-- 
2.15.0

Re: [PATCH v2 3/3] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Daniel Kiper]

On Tue, Feb 20, 2018 at 06:25:33PM -0500, Peter Jones wrote:
> This way debuginfo built from the .module will still include this
> information, but the final result won't have the data we don't actually
> need in the modules, either on-disk, loaded at runtime, or in prebuilt
> images.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

I will apply this patch with other ones at once by the end of this week.

Daniel

Re: [PATCH v2 1/3] mkimage: refactor a bunch of section data into a struct.

[Daniel Kiper]

On Tue, Feb 20, 2018 at 06:25:31PM -0500, Peter Jones wrote:
> This basically moves a bunch of the section information we pass around a
> lot into a struct, and passes a pointer to a single one of those
> instead.
>
> Because it's more convenient, it also puts the section_vaddresses
> calculations in locate_section(), which no longer returns the allocation
> for section_addresses directly either.

Could not you make this change in separate patch?

> This shouldn't change the binary file output or the "grub-mkimage -v"
> output in any way.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  util/grub-mkimagexx.c | 282 ++++++++++++++++++++++----------------------------
>  1 file changed, 123 insertions(+), 159 deletions(-)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index a2bb05439f0..5c787ec56bf 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -84,6 +84,17 @@ struct fixup_block_list
>
>  #define ALIGN_ADDR(x) (ALIGN_UP((x), image_target->voidp_sizeof))
>
> +struct section_metadata
> +{
> +  Elf_Half num_sections;
> +  Elf_Shdr *sections;
> +  Elf_Addr *addresses;

s/addresses/addr/

> +  Elf_Addr *vaddresses;

s/vaddresses/vaddr/

> +  Elf_Half section_entsize;
> +  Elf_Shdr *symtab;
> +  const char *strtab;
> +};
> +
>  static int
>  is_relocatable (const struct grub_install_image_target_desc *image_target)
>  {
> @@ -499,9 +510,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>  /* Relocate symbols; note that this function overwrites the symbol table.
>     Return the address of a start symbol.  */
>  static Elf_Addr
> -SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
> -			   Elf_Shdr *symtab_section, Elf_Addr *section_addresses,
> -			   Elf_Half section_entsize, Elf_Half num_sections,
> +SUFFIX (relocate_symbols) (Elf_Ehdr *e, struct section_metadata *sdata,

I would do s/sdata/smdata/ or even s/sdata/smd/...

>  			   void *jumpers, Elf_Addr jumpers_addr,
>  			   Elf_Addr bss_start, Elf_Addr end,
>  			   const struct grub_install_image_target_desc *image_target)
> @@ -511,19 +520,18 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
>    Elf_Addr start_address = (Elf_Addr) -1;
>    Elf_Sym *sym;
>    Elf_Word i;
> -  Elf_Shdr *strtab_section;
> -  const char *strtab;
> +  Elf_Shdr *symtab_section;
> +  const char *symtab;

This change does not seem to be logical part of this patch.

Daniel

Re: [PATCH v2 1/3] mkimage: refactor a bunch of section data into a struct.

[Peter Jones]

On Wed, Feb 21, 2018 at 12:07:42PM +0100, Daniel Kiper wrote:
> This change does not seem to be logical part of this patch.

Okay, I've rolled all these things up into my local tree, get ready for
another volley of patches.

This version starts with 2 patches that are only vaguely related, that
are just creature comforts really.  Do with them what you will ;)

-- 
  Peter

[PATCH v3 1/7] aout.h: Fix missing include.

[Peter Jones]

grub_aout_load() has a grub_file_t parameter, and depending on what order
includes land in, it's sometimes not defined.  This patch explicitly adds
file.h to aout.h so that it will always be defined.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 include/grub/aout.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/grub/aout.h b/include/grub/aout.h
index 10d7dde61ec..c8c1d94eca5 100644
--- a/include/grub/aout.h
+++ b/include/grub/aout.h
@@ -52,6 +52,7 @@
 #define GRUB_AOUT_HEADER 1
 
 #include <grub/types.h>
+#include <grub/file.h>
 
 struct grub_aout32_header
 {
-- 
2.15.0

[PATCH v3 2/7] mkimage: make it easier to run syntax checkers on grub-mkimagexx.c

[Peter Jones]

This makes it so you can treat grub-mkimagexx.c as a file you can build
directly, so syntax checkers like vim's "syntastic" plugin, which uses
"gcc -x c -fsyntax-only" to build it, will work.

One still has to do whatever setup is required to make it pick the right
include dirs, which -W options we use, etc., but this makes it so you
can do the checking on the file you're editing, rather than on a
different file.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 util/grub-mkimage32.c | 2 ++
 util/grub-mkimage64.c | 2 ++
 util/grub-mkimagexx.c | 9 +++++++++
 3 files changed, 13 insertions(+)

diff --git a/util/grub-mkimage32.c b/util/grub-mkimage32.c
index 9b31397bc40..1f2ccccd225 100644
--- a/util/grub-mkimage32.c
+++ b/util/grub-mkimage32.c
@@ -19,4 +19,6 @@
 # define ELF_ST_TYPE(val)		ELF32_ST_TYPE(val)
 #define XEN_NOTE_SIZE 132
 
+#ifndef GRUB_MKIMAGEXX
 #include "grub-mkimagexx.c"
+#endif
diff --git a/util/grub-mkimage64.c b/util/grub-mkimage64.c
index d8334592470..4ff72a625e0 100644
--- a/util/grub-mkimage64.c
+++ b/util/grub-mkimage64.c
@@ -19,4 +19,6 @@
 # define ELF_ST_TYPE(val)		ELF64_ST_TYPE(val)
 #define XEN_NOTE_SIZE 120
 
+#ifndef GRUB_MKIMAGEXX
 #include "grub-mkimagexx.c"
+#endif
diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index a2bb05439f0..10d7b41e3bf 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -50,6 +50,15 @@
 
 #pragma GCC diagnostic ignored "-Wcast-align"
 
+#define GRUB_MKIMAGEXX
+#if !defined(MKIMAGE_ELF32) && !defined(MKIMAGE_ELF64)
+#if __SIZEOF_POINTER__ == 8
+#include "grub-mkimage64.c"
+#else
+#include "grub-mkimage32.c"
+#endif
+#endif
+
 /* These structures are defined according to the CHRP binding to IEEE1275,
    "Client Program Format" section.  */
 
-- 
2.15.0

Re: [PATCH v3 2/7] mkimage: make it easier to run syntax checkers on grub-mkimagexx.c

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:24PM -0500, Peter Jones wrote:
> This makes it so you can treat grub-mkimagexx.c as a file you can build
> directly, so syntax checkers like vim's "syntastic" plugin, which uses
> "gcc -x c -fsyntax-only" to build it, will work.
>
> One still has to do whatever setup is required to make it pick the right
> include dirs, which -W options we use, etc., but this makes it so you
> can do the checking on the file you're editing, rather than on a
> different file.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v3 3/7] mkimage: rename a couple of things to be less confusing later.

[Peter Jones]

This renames some things:

- the "strtab" and "strtab_section" in relocate_symbols are changed to "symtab"
  instead, so as to be less confusing when "strtab" is moved to a struct in a
  later patch.

- The places where we pass section_vaddresses to functions are changed to also
  be called section_vaddresses"inside those functions, so I get less confused
  when I put addresses and vaddresses in a struct in a later patch.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 util/grub-mkimagexx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 10d7b41e3bf..e00a831e0ca 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -509,7 +509,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
    Return the address of a start symbol.  */
 static Elf_Addr
 SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
-			   Elf_Shdr *symtab_section, Elf_Addr *section_addresses,
+			   Elf_Shdr *symtab_section, Elf_Addr *section_vaddresses,
 			   Elf_Half section_entsize, Elf_Half num_sections,
 			   void *jumpers, Elf_Addr jumpers_addr,
 			   Elf_Addr bss_start, Elf_Addr end,
@@ -520,15 +520,15 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
   Elf_Addr start_address = (Elf_Addr) -1;
   Elf_Sym *sym;
   Elf_Word i;
-  Elf_Shdr *strtab_section;
-  const char *strtab;
+  Elf_Shdr *symtab_link_section;
+  const char *symtab;
   grub_uint64_t *jptr = jumpers;
 
-  strtab_section
+  symtab_link_section
     = (Elf_Shdr *) ((char *) sections
 		      + (grub_target_to_host32 (symtab_section->sh_link)
 			 * section_entsize));
-  strtab = (char *) e + grub_target_to_host (strtab_section->sh_offset);
+  symtab = (char *) e + grub_target_to_host (symtab_link_section->sh_offset);
 
   symtab_size = grub_target_to_host (symtab_section->sh_size);
   sym_size = grub_target_to_host (symtab_section->sh_entsize);
@@ -542,7 +542,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
       Elf_Section cur_index;
       const char *name;
 
-      name = strtab + grub_target_to_host32 (sym->st_name);
+      name = symtab + grub_target_to_host32 (sym->st_name);
 
       cur_index = grub_target_to_host16 (sym->st_shndx);
       if (cur_index == STN_ABS)
@@ -565,7 +565,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
       else
 	{
 	  sym->st_value = (grub_target_to_host (sym->st_value)
-			   + section_addresses[cur_index]);
+			   + section_vaddresses[cur_index]);
 	}
 
       if (image_target->elf_target == EM_IA_64 && ELF_ST_TYPE (sym->st_info)
@@ -580,7 +580,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
       grub_util_info ("locating %s at 0x%"  GRUB_HOST_PRIxLONG_LONG
 		      " (0x%"  GRUB_HOST_PRIxLONG_LONG ")", name,
 		      (unsigned long long) sym->st_value,
-		      (unsigned long long) section_addresses[cur_index]);
+		      (unsigned long long) section_vaddresses[cur_index]);
 
       if (start_address == (Elf_Addr)-1)
 	if (strcmp (name, "_start") == 0 || strcmp (name, "start") == 0)
@@ -1643,7 +1643,7 @@ create_u64_fixups (struct translate_context *ctx,
 /* Make a .reloc section.  */
 static void
 make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
-		    Elf_Addr *section_addresses, Elf_Shdr *sections,
+		    Elf_Addr *section_vaddresses, Elf_Shdr *sections,
 		    Elf_Half section_entsize, Elf_Half num_sections,
 		    const char *strtab,
 		    const struct grub_install_image_target_desc *image_target)
@@ -1673,7 +1673,7 @@ make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
 	rtab_offset = grub_target_to_host (s->sh_offset);
 	num_rs = rtab_size / r_size;
 
-	section_address = section_addresses[grub_le_to_cpu32 (s->sh_info)];
+	section_address = section_vaddresses[grub_le_to_cpu32 (s->sh_info)];
 
 	for (j = 0, r = (Elf_Rel *) ((char *) e + rtab_offset);
 	     j < num_rs;
-- 
2.15.0

Re: [PATCH v3 3/7] mkimage: rename a couple of things to be less confusing later.

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:25PM -0500, Peter Jones wrote:
> This renames some things:
>
> - the "strtab" and "strtab_section" in relocate_symbols are changed to "symtab"
>   instead, so as to be less confusing when "strtab" is moved to a struct in a
>   later patch.
>
> - The places where we pass section_vaddresses to functions are changed to also
>   be called section_vaddresses"inside those functions, so I get less confused
>   when I put addresses and vaddresses in a struct in a later patch.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v3 4/7] mkimage: make locate_sections() set up vaddresses as well.

[Peter Jones]

This puts both kinds of address initialization at the same place, and also lets
us iterate through the section list one time fewer.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 util/grub-mkimagexx.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index e00a831e0ca..0b7c00e2297 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -1783,18 +1783,19 @@ SUFFIX (put_section) (Elf_Shdr *s, int i,
 	return current_address;
 }
 
-/* Locate section addresses by merging code sections and data sections
-   into .text and .data, respectively. Return the array of section
-   addresses.  */
-static Elf_Addr *
+/*
+ * Locate section addresses by merging code sections and data sections
+ * into .text and .data, respectively.
+ */
+static void
 SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
 			  Elf_Shdr *sections, Elf_Half section_entsize,
-			  Elf_Half num_sections, const char *strtab,
+			  Elf_Half num_sections, Elf_Addr *section_addresses,
+			  Elf_Addr *section_vaddresses, const char *strtab,
 			  struct grub_mkimage_layout *layout,
 			  const struct grub_install_image_target_desc *image_target)
 {
   int i;
-  Elf_Addr *section_addresses;
   Elf_Shdr *s;
 
   layout->align = 1;
@@ -1802,8 +1803,6 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   if (image_target->elf_target == EM_AARCH64)
     layout->align = 4096;
 
-  section_addresses = xmalloc (sizeof (*section_addresses) * num_sections);
-  memset (section_addresses, 0, sizeof (*section_addresses) * num_sections);
 
   layout->kernel_size = 0;
 
@@ -1879,12 +1878,16 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   for (i = 0, s = sections;
        i < num_sections;
        i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
-    if (SUFFIX (is_bss_section) (s, image_target))
-      layout->end = SUFFIX (put_section) (s, i,
-					  layout->end,
-					  section_addresses,
-					  strtab,
-					  image_target);
+    {
+      if (SUFFIX (is_bss_section) (s, image_target))
+	layout->end = SUFFIX (put_section) (s, i, layout->end,
+					    section_addresses, strtab,
+					    image_target);
+      /*
+       * This must to be in the last time this function passes through the loop.
+       */
+      section_vaddresses[i] = section_addresses[i] + image_target->vaddr_offset;
+    }
 
   layout->end = ALIGN_UP (layout->end + image_target->vaddr_offset,
 			      image_target->section_align) - image_target->vaddr_offset;
@@ -1895,8 +1898,6 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   */
   if (image_target->id == IMAGE_EFI || !is_relocatable (image_target))
     layout->kernel_size = layout->end;
-
-  return section_addresses;
 }
 
 char *
@@ -1945,16 +1946,14 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 		      + grub_host_to_target16 (e->e_shstrndx) * section_entsize);
   strtab = (char *) e + grub_host_to_target_addr (s->sh_offset);
 
-  section_addresses = SUFFIX (locate_sections) (e, kernel_path,
-						sections, section_entsize,
-						num_sections, strtab,
-						layout,
-						image_target);
+  section_addresses = xmalloc (sizeof (*section_addresses) * num_sections);
+  memset (section_addresses, 0, sizeof (*section_addresses) * num_sections);
+  section_vaddresses = xmalloc (sizeof (*section_vaddresses) * num_sections);
+  memset (section_vaddresses, 0, sizeof (*section_vaddresses) * num_sections);
 
-  section_vaddresses = xmalloc (sizeof (*section_addresses) * num_sections);
-
-  for (i = 0; i < num_sections; i++)
-    section_vaddresses[i] = section_addresses[i] + image_target->vaddr_offset;
+  SUFFIX (locate_sections) (e, kernel_path, sections, section_entsize, num_sections,
+			    section_addresses, section_vaddresses, strtab, layout,
+			    image_target);
 
   if (!is_relocatable (image_target))
     {
-- 
2.15.0

Re: [PATCH v3 4/7] mkimage: make locate_sections() set up vaddresses as well.

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:26PM -0500, Peter Jones wrote:
> This puts both kinds of address initialization at the same place, and also lets
> us iterate through the section list one time fewer.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v3 5/7] mkimage: refactor a bunch of section data into a struct.

[Peter Jones]

This basically moves a bunch of the section information we pass around a
lot into a struct, and passes a pointer to a single one of those
instead.

Because it's more convenient, it also puts the section_vaddresses
calculations in locate_section(), which no longer returns the allocation
for section_addresses directly either.

This shouldn't change the binary file output or the "grub-mkimage -v"
output in any way.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 util/grub-mkimagexx.c | 273 ++++++++++++++++++++++----------------------------
 1 file changed, 121 insertions(+), 152 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 0b7c00e2297..19aa87f7efe 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -93,6 +93,17 @@ struct fixup_block_list
 
 #define ALIGN_ADDR(x) (ALIGN_UP((x), image_target->voidp_sizeof))
 
+struct section_metadata
+{
+  Elf_Half num_sections;
+  Elf_Shdr *sections;
+  Elf_Addr *addrs;
+  Elf_Addr *vaddrs;
+  Elf_Half section_entsize;
+  Elf_Shdr *symtab;
+  const char *strtab;
+};
+
 static int
 is_relocatable (const struct grub_install_image_target_desc *image_target)
 {
@@ -508,9 +519,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
 /* Relocate symbols; note that this function overwrites the symbol table.
    Return the address of a start symbol.  */
 static Elf_Addr
-SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
-			   Elf_Shdr *symtab_section, Elf_Addr *section_vaddresses,
-			   Elf_Half section_entsize, Elf_Half num_sections,
+SUFFIX (relocate_symbols) (Elf_Ehdr *e, struct section_metadata *smd,
 			   void *jumpers, Elf_Addr jumpers_addr,
 			   Elf_Addr bss_start, Elf_Addr end,
 			   const struct grub_install_image_target_desc *image_target)
@@ -520,19 +529,18 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
   Elf_Addr start_address = (Elf_Addr) -1;
   Elf_Sym *sym;
   Elf_Word i;
-  Elf_Shdr *symtab_link_section;
+  Elf_Shdr *symtab_section;
   const char *symtab;
   grub_uint64_t *jptr = jumpers;
 
-  symtab_link_section
-    = (Elf_Shdr *) ((char *) sections
-		      + (grub_target_to_host32 (symtab_section->sh_link)
-			 * section_entsize));
-  symtab = (char *) e + grub_target_to_host (symtab_link_section->sh_offset);
+  symtab_section = (Elf_Shdr *) ((char *) smd->sections
+				 + grub_target_to_host32 (smd->symtab->sh_link)
+				   * smd->section_entsize);
+  symtab = (char *) e + grub_target_to_host (symtab_section->sh_offset);
 
-  symtab_size = grub_target_to_host (symtab_section->sh_size);
-  sym_size = grub_target_to_host (symtab_section->sh_entsize);
-  symtab_offset = grub_target_to_host (symtab_section->sh_offset);
+  symtab_size = grub_target_to_host (smd->symtab->sh_size);
+  sym_size = grub_target_to_host (smd->symtab->sh_entsize);
+  symtab_offset = grub_target_to_host (smd->symtab->sh_offset);
   num_syms = symtab_size / sym_size;
 
   for (i = 0, sym = (Elf_Sym *) ((char *) e + symtab_offset);
@@ -560,12 +568,12 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
 	  else
 	    continue;
 	}
-      else if (cur_index >= num_sections)
+      else if (cur_index >= smd->num_sections)
 	grub_util_error ("section %d does not exist", cur_index);
       else
 	{
 	  sym->st_value = (grub_target_to_host (sym->st_value)
-			   + section_vaddresses[cur_index]);
+			   + smd->vaddrs[cur_index]);
 	}
 
       if (image_target->elf_target == EM_IA_64 && ELF_ST_TYPE (sym->st_info)
@@ -580,7 +588,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
       grub_util_info ("locating %s at 0x%"  GRUB_HOST_PRIxLONG_LONG
 		      " (0x%"  GRUB_HOST_PRIxLONG_LONG ")", name,
 		      (unsigned long long) sym->st_value,
-		      (unsigned long long) section_vaddresses[cur_index]);
+		      (unsigned long long) smd->vaddrs[cur_index]);
 
       if (start_address == (Elf_Addr)-1)
 	if (strcmp (name, "_start") == 0 || strcmp (name, "start") == 0)
@@ -638,9 +646,9 @@ SUFFIX (count_funcs) (Elf_Ehdr *e, Elf_Shdr *symtab_section,
 #endif
 
 #ifdef MKIMAGE_ELF32
-/* Deal with relocation information. This function relocates addresses
+/* Deal with relocation information. This function relocates addrs
    within the virtual address space starting from 0. So only relative
-   addresses can be fully resolved. Absolute addresses must be relocated
+   addrs can be fully resolved. Absolute addrs must be relocated
    again by a PE32 relocator when loaded.  */
 static grub_size_t
 arm_get_trampoline_size (Elf_Ehdr *e,
@@ -717,17 +725,13 @@ arm_get_trampoline_size (Elf_Ehdr *e,
 }
 #endif
 
-/* Deal with relocation information. This function relocates addresses
+/* Deal with relocation information. This function relocates addrs
    within the virtual address space starting from 0. So only relative
-   addresses can be fully resolved. Absolute addresses must be relocated
+   addrs can be fully resolved. Absolute addrs must be relocated
    again by a PE32 relocator when loaded.  */
 static void
-SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
-			     Elf_Addr *section_addresses,
-			     Elf_Half section_entsize, Elf_Half num_sections,
-			     const char *strtab,
-			     char *pe_target, Elf_Addr tramp_off,
-			     Elf_Addr got_off,
+SUFFIX (relocate_addrs) (Elf_Ehdr *e, struct section_metadata *smd,
+			     char *pe_target, Elf_Addr tramp_off, Elf_Addr got_off,
 			     const struct grub_install_image_target_desc *image_target)
 {
   Elf_Half i;
@@ -741,33 +745,29 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
   grub_uint32_t *tr = (void *) (pe_target + tramp_off);
 #endif
 
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = smd->sections;
+       i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
     if ((s->sh_type == grub_host_to_target32 (SHT_REL)) ||
         (s->sh_type == grub_host_to_target32 (SHT_RELA)))
       {
 	Elf_Rela *r;
 	Elf_Word rtab_size, r_size, num_rs;
 	Elf_Off rtab_offset;
-	Elf_Shdr *symtab_section;
 	Elf_Word target_section_index;
 	Elf_Addr target_section_addr;
 	Elf_Shdr *target_section;
 	Elf_Word j;
 
-	symtab_section = (Elf_Shdr *) ((char *) sections
-					 + (grub_target_to_host32 (s->sh_link)
-					    * section_entsize));
 	target_section_index = grub_target_to_host32 (s->sh_info);
-	target_section_addr = section_addresses[target_section_index];
-	target_section = (Elf_Shdr *) ((char *) sections
+	target_section_addr = smd->addrs[target_section_index];
+	target_section = (Elf_Shdr *) ((char *) smd->sections
 					 + (target_section_index
-					    * section_entsize));
+					    * smd->section_entsize));
 
 	grub_util_info ("dealing with the relocation section %s for %s",
-			strtab + grub_target_to_host32 (s->sh_name),
-			strtab + grub_target_to_host32 (target_section->sh_name));
+			smd->strtab + grub_target_to_host32 (s->sh_name),
+			smd->strtab + grub_target_to_host32 (target_section->sh_name));
 
 	rtab_size = grub_target_to_host (s->sh_size);
 	r_size = grub_target_to_host (s->sh_entsize);
@@ -788,7 +788,7 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
 	    target = SUFFIX (get_target_address) (e, target_section,
 						  offset, image_target);
 	    info = grub_target_to_host (r->r_info);
-	    sym_addr = SUFFIX (get_symbol_address) (e, symtab_section,
+	    sym_addr = SUFFIX (get_symbol_address) (e, smd->symtab,
 						    ELF_R_SYM (info), image_target);
 
             addend = (s->sh_type == grub_target_to_host32 (SHT_RELA)) ?
@@ -918,8 +918,8 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
 		    Elf_Sym *sym;
 
 		    sym = (Elf_Sym *) ((char *) e
-				       + grub_target_to_host (symtab_section->sh_offset)
-				       + ELF_R_SYM (info) * grub_target_to_host (symtab_section->sh_entsize));
+				       + grub_target_to_host (smd->symtab->sh_offset)
+				       + ELF_R_SYM (info) * grub_target_to_host (smd->symtab->sh_entsize));
 		    if (ELF_ST_TYPE (sym->st_info) == STT_FUNC)
 		      sym_addr = grub_target_to_host64 (*(grub_uint64_t *) (pe_target
 									    + sym->st_value
@@ -1104,8 +1104,8 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
 							- (char *) e),
 				       sym_addr);
 		       sym = (Elf_Sym *) ((char *) e
-					  + grub_target_to_host (symtab_section->sh_offset)
-					  + ELF_R_SYM (info) * grub_target_to_host (symtab_section->sh_entsize));
+					  + grub_target_to_host (smd->symtab->sh_offset)
+					  + ELF_R_SYM (info) * grub_target_to_host (smd->symtab->sh_entsize));
 		       if (ELF_ST_TYPE (sym->st_info) != STT_FUNC)
 			 sym_addr |= 1;
 		       if (!(sym_addr & 1))
@@ -1316,7 +1316,7 @@ translate_relocation_pe (struct translate_context *ctx,
 			 Elf_Addr info,
 			 const struct grub_install_image_target_desc *image_target)
 {
-  /* Necessary to relocate only absolute addresses.  */
+  /* Necessary to relocate only absolute addrs.  */
   switch (image_target->elf_target)
     {
     case EM_386:
@@ -1466,7 +1466,7 @@ static enum raw_reloc_type
 classify_raw_reloc (Elf_Addr info,
 		    const struct grub_install_image_target_desc *image_target)
 {
-    /* Necessary to relocate only absolute addresses.  */
+    /* Necessary to relocate only absolute addrs.  */
   switch (image_target->elf_target)
     {
     case EM_ARM:
@@ -1643,9 +1643,7 @@ create_u64_fixups (struct translate_context *ctx,
 /* Make a .reloc section.  */
 static void
 make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
-		    Elf_Addr *section_vaddresses, Elf_Shdr *sections,
-		    Elf_Half section_entsize, Elf_Half num_sections,
-		    const char *strtab,
+		    struct section_metadata *smd,
 		    const struct grub_install_image_target_desc *image_target)
 {
   unsigned i;
@@ -1654,8 +1652,8 @@ make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
 
   translate_reloc_start (&ctx, image_target);
 
-  for (i = 0, s = sections; i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = smd->sections; i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
     if ((grub_target_to_host32 (s->sh_type) == SHT_REL) ||
         (grub_target_to_host32 (s->sh_type) == SHT_RELA))
       {
@@ -1666,14 +1664,14 @@ make_reloc_section (Elf_Ehdr *e, struct grub_mkimage_layout *layout,
 	Elf_Word j;
 
 	grub_util_info ("translating the relocation section %s",
-			strtab + grub_le_to_cpu32 (s->sh_name));
+			smd->strtab + grub_le_to_cpu32 (s->sh_name));
 
 	rtab_size = grub_target_to_host (s->sh_size);
 	r_size = grub_target_to_host (s->sh_entsize);
 	rtab_offset = grub_target_to_host (s->sh_offset);
 	num_rs = rtab_size / r_size;
 
-	section_address = section_vaddresses[grub_le_to_cpu32 (s->sh_info)];
+	section_address = smd->vaddrs[grub_le_to_cpu32 (s->sh_info)];
 
 	for (j = 0, r = (Elf_Rel *) ((char *) e + rtab_offset);
 	     j < num_rs;
@@ -1760,12 +1758,11 @@ SUFFIX (check_elf_header) (Elf_Ehdr *e, size_t size, const struct grub_install_i
 static Elf_Addr
 SUFFIX (put_section) (Elf_Shdr *s, int i,
 		      Elf_Addr current_address,
-		      Elf_Addr *section_addresses,
-		      const char *strtab,
+		      struct section_metadata *smd,
 		      const struct grub_install_image_target_desc *image_target)
 {
 	Elf_Word align = grub_host_to_target_addr (s->sh_addralign);
-	const char *name = strtab + grub_host_to_target32 (s->sh_name);
+	const char *name = smd->strtab + grub_host_to_target32 (s->sh_name);
 
 	if (align)
 	  current_address = ALIGN_UP (current_address + image_target->vaddr_offset,
@@ -1777,8 +1774,8 @@ SUFFIX (put_section) (Elf_Shdr *s, int i,
 			name, (unsigned long long) current_address);
 	if (!is_relocatable (image_target))
 	  current_address = grub_host_to_target_addr (s->sh_addr)
-	    - image_target->link_addr;
-	section_addresses[i] = current_address;
+			    - image_target->link_addr;
+	smd->addrs[i] = current_address;
 	current_address += grub_host_to_target_addr (s->sh_size);
 	return current_address;
 }
@@ -1789,9 +1786,7 @@ SUFFIX (put_section) (Elf_Shdr *s, int i,
  */
 static void
 SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
-			  Elf_Shdr *sections, Elf_Half section_entsize,
-			  Elf_Half num_sections, Elf_Addr *section_addresses,
-			  Elf_Addr *section_vaddresses, const char *strtab,
+			  struct section_metadata *smd,
 			  struct grub_mkimage_layout *layout,
 			  const struct grub_install_image_target_desc *image_target)
 {
@@ -1803,28 +1798,23 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   if (image_target->elf_target == EM_AARCH64)
     layout->align = 4096;
 
-
   layout->kernel_size = 0;
 
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
-    if ((grub_target_to_host (s->sh_flags) & SHF_ALLOC) 
+  for (i = 0, s = smd->sections;
+       i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
+    if ((grub_target_to_host (s->sh_flags) & SHF_ALLOC)
 	&& grub_host_to_target32 (s->sh_addralign) > layout->align)
       layout->align = grub_host_to_target32 (s->sh_addralign);
 
-
   /* .text */
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = smd->sections;
+       i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
     if (SUFFIX (is_text_section) (s, image_target))
       {
-	layout->kernel_size = SUFFIX (put_section) (s, i,
-						layout->kernel_size,
-						section_addresses,
-						strtab,
-						image_target);
+	layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size,
+						smd, image_target);
 	if (!is_relocatable (image_target) &&
 	    grub_host_to_target_addr (s->sh_addr) != image_target->link_addr)
 	  {
@@ -1844,15 +1834,12 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   layout->exec_size = layout->kernel_size;
 
   /* .data */
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = smd->sections;
+       i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
     if (SUFFIX (is_data_section) (s, image_target))
-      layout->kernel_size = SUFFIX (put_section) (s, i,
-					      layout->kernel_size,
-					      section_addresses,
-					      strtab,
-					      image_target);
+      layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, smd,
+						  image_target);
 
 #ifdef MKIMAGE_ELF32
   if (image_target->elf_target == EM_ARM)
@@ -1863,8 +1850,8 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
 
       layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
 
-      tramp = arm_get_trampoline_size (e, sections, section_entsize,
-				       num_sections, image_target);
+      tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,
+				       smd->num_sections, image_target);
 
       layout->tramp_off = layout->kernel_size;
       layout->kernel_size += ALIGN_UP (tramp, 16);
@@ -1875,18 +1862,17 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
   layout->end = layout->kernel_size;
   
   /* .bss */
-  for (i = 0, s = sections;
-       i < num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+  for (i = 0, s = smd->sections;
+       i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
     {
       if (SUFFIX (is_bss_section) (s, image_target))
-	layout->end = SUFFIX (put_section) (s, i, layout->end,
-					    section_addresses, strtab,
-					    image_target);
+	layout->end = SUFFIX (put_section) (s, i, layout->end, smd, image_target);
+
       /*
        * This must to be in the last time this function passes through the loop.
        */
-      section_vaddresses[i] = section_addresses[i] + image_target->vaddr_offset;
+      smd->vaddrs[i] = smd->addrs[i] + image_target->vaddr_offset;
     }
 
   layout->end = ALIGN_UP (layout->end + image_target->vaddr_offset,
@@ -1907,18 +1893,12 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 				  const struct grub_install_image_target_desc *image_target)
 {
   char *kernel_img, *out_img;
-  const char *strtab;
+  struct section_metadata smd = { 0, };
   Elf_Ehdr *e;
-  Elf_Shdr *sections;
-  Elf_Addr *section_addresses;
-  Elf_Addr *section_vaddresses;
   int i;
   Elf_Shdr *s;
-  Elf_Half num_sections;
   Elf_Off section_offset;
-  Elf_Half section_entsize;
   grub_size_t kernel_size;
-  Elf_Shdr *symtab_section = 0;
 
   grub_memset (layout, 0, sizeof (*layout));
 
@@ -1933,46 +1913,45 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
     grub_util_error ("invalid ELF header");
 
   section_offset = grub_target_to_host (e->e_shoff);
-  section_entsize = grub_target_to_host16 (e->e_shentsize);
-  num_sections = grub_target_to_host16 (e->e_shnum);
+  smd.section_entsize = grub_target_to_host16 (e->e_shentsize);
+  smd.num_sections = grub_target_to_host16 (e->e_shnum);
 
-  if (kernel_size < section_offset + (grub_uint32_t) section_entsize * num_sections)
+  if (kernel_size < section_offset
+		    + (grub_uint32_t) smd.section_entsize * smd.num_sections)
     grub_util_error (_("premature end of file %s"), kernel_path);
 
-  sections = (Elf_Shdr *) (kernel_img + section_offset);
+  smd.sections = (Elf_Shdr *) (kernel_img + section_offset);
 
   /* Relocate sections then symbols in the virtual address space.  */
-  s = (Elf_Shdr *) ((char *) sections
-		      + grub_host_to_target16 (e->e_shstrndx) * section_entsize);
-  strtab = (char *) e + grub_host_to_target_addr (s->sh_offset);
+  s = (Elf_Shdr *) ((char *) smd.sections
+		      + grub_host_to_target16 (e->e_shstrndx) * smd.section_entsize);
+  smd.strtab = (char *) e + grub_host_to_target_addr (s->sh_offset);
 
-  section_addresses = xmalloc (sizeof (*section_addresses) * num_sections);
-  memset (section_addresses, 0, sizeof (*section_addresses) * num_sections);
-  section_vaddresses = xmalloc (sizeof (*section_vaddresses) * num_sections);
-  memset (section_vaddresses, 0, sizeof (*section_vaddresses) * num_sections);
+  smd.addrs = xmalloc (sizeof (*smd.addrs) * smd.num_sections);
+  memset (smd.addrs, 0, sizeof (*smd.addrs) * smd.num_sections);
+  smd.vaddrs = xmalloc (sizeof (*smd.vaddrs) * smd.num_sections);
+  memset (smd.vaddrs, 0, sizeof (*smd.vaddrs) * smd.num_sections);
 
-  SUFFIX (locate_sections) (e, kernel_path, sections, section_entsize, num_sections,
-			    section_addresses, section_vaddresses, strtab, layout,
-			    image_target);
+  SUFFIX (locate_sections) (e, kernel_path, &smd, layout, image_target);
 
   if (!is_relocatable (image_target))
     {
       Elf_Addr current_address = layout->kernel_size;
 
-      for (i = 0, s = sections;
-	   i < num_sections;
-	   i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+      for (i = 0, s = smd.sections;
+	   i < smd.num_sections;
+	   i++, s = (Elf_Shdr *) ((char *) s + smd.section_entsize))
 	if (grub_target_to_host32 (s->sh_type) == SHT_NOBITS)
 	  {
 	    Elf_Word sec_align = grub_host_to_target_addr (s->sh_addralign);
-	    const char *name = strtab + grub_host_to_target32 (s->sh_name);
+	    const char *name = smd.strtab + grub_host_to_target32 (s->sh_name);
 
 	    if (sec_align)
 	      current_address = ALIGN_UP (current_address
 					  + image_target->vaddr_offset,
 					  sec_align)
 		- image_target->vaddr_offset;
-	
+
 	    grub_util_info ("locating the section %s at 0x%"
 			    GRUB_HOST_PRIxLONG_LONG,
 			    name, (unsigned long long) current_address);
@@ -1980,7 +1959,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 	      current_address = grub_host_to_target_addr (s->sh_addr)
 		- image_target->link_addr;
 
-	    section_vaddresses[i] = current_address
+	    smd.vaddrs[i] = current_address
 	      + image_target->vaddr_offset;
 	    current_address += grub_host_to_target_addr (s->sh_size);
 	  }
@@ -2001,16 +1980,16 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 
   if (is_relocatable (image_target))
     {
-      symtab_section = NULL;
-      for (i = 0, s = sections;
-	   i < num_sections;
-	   i++, s = (Elf_Shdr *) ((char *) s + section_entsize))
+      smd.symtab = NULL;
+      for (i = 0, s = smd.sections;
+	   i < smd.num_sections;
+	   i++, s = (Elf_Shdr *) ((char *) s + smd.section_entsize))
 	if (s->sh_type == grub_host_to_target32 (SHT_SYMTAB))
 	  {
-	    symtab_section = s;
+	    smd.symtab = s;
 	    break;
 	  }
-      if (! symtab_section)
+      if (! smd.symtab)
 	grub_util_error ("%s", _("no symbol table"));
 #ifdef MKIMAGE_ELF64
       if (image_target->elf_target == EM_IA_64)
@@ -2025,7 +2004,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 	  layout->kernel_size += ALIGN_UP (tramp, 16);
 
 	  layout->ia64jmp_off = layout->kernel_size;
-	  layout->ia64jmpnum = SUFFIX (count_funcs) (e, symtab_section,
+	  layout->ia64jmpnum = SUFFIX (count_funcs) (e, smd.symtab,
 						     image_target);
 	  layout->kernel_size += 16 * layout->ia64jmpnum;
 
@@ -2056,31 +2035,19 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 
   if (is_relocatable (image_target))
     {
-      layout->start_address = SUFFIX (relocate_symbols) (e, sections, symtab_section,
-					  section_vaddresses, section_entsize,
-					  num_sections, 
-					  (char *) out_img + layout->ia64jmp_off, 
-					  layout->ia64jmp_off 
-					  + image_target->vaddr_offset,
-							 layout->bss_start,
-							 layout->end,
-					  image_target);
+      layout->start_address = SUFFIX (relocate_symbols) (e, &smd,
+				  (char *) out_img + layout->ia64jmp_off,
+				  layout->ia64jmp_off + image_target->vaddr_offset,
+				  layout->bss_start, layout->end, image_target);
+
       if (layout->start_address == (Elf_Addr) -1)
 	grub_util_error ("start symbol is not defined");
 
-      /* Resolve addresses in the virtual address space.  */
-      SUFFIX (relocate_addresses) (e, sections, section_addresses, 
-				   section_entsize,
-				   num_sections, strtab,
-				   out_img, layout->tramp_off,
-				   layout->got_off,
-				   image_target);
+      /* Resolve addrs in the virtual address space.  */
+      SUFFIX (relocate_addrs) (e, &smd, out_img, layout->tramp_off,
+				   layout->got_off, image_target);
 
-      make_reloc_section (e, layout,
-			  section_vaddresses, sections,
-			  section_entsize, num_sections,
-			  strtab,
-			  image_target);
+      make_reloc_section (e, layout, &smd, image_target);
       if (image_target->id != IMAGE_EFI)
 	{
 	  out_img = xrealloc (out_img, layout->kernel_size + total_module_size
@@ -2092,9 +2059,9 @@ 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))
+  for (i = 0, s = smd.sections;
+       i < smd.num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd.section_entsize))
     if (SUFFIX (is_data_section) (s, image_target)
 	/* Explicitly initialize BSS
 	   when producing PE32 to avoid a bug in EFI implementations.
@@ -2105,17 +2072,19 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
 	|| SUFFIX (is_text_section) (s, image_target))
       {
 	if (grub_target_to_host32 (s->sh_type) == SHT_NOBITS)
-	  memset (out_img + section_addresses[i], 0,
+	  memset (out_img + smd.addrs[i], 0,
 		  grub_host_to_target_addr (s->sh_size));
 	else
-	  memcpy (out_img + section_addresses[i],
+	  memcpy (out_img + smd.addrs[i],
 		  kernel_img + grub_host_to_target_addr (s->sh_offset),
 		  grub_host_to_target_addr (s->sh_size));
       }
   free (kernel_img);
 
-  free (section_vaddresses);
-  free (section_addresses);
+  free (smd.vaddrs);
+  smd.vaddrs = NULL;
+  free (smd.addrs);
+  smd.addrs = NULL;
 
   return out_img;
 }
-- 
2.15.0

Re: [PATCH v3 5/7] mkimage: refactor a bunch of section data into a struct.

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:27PM -0500, Peter Jones wrote:
> This basically moves a bunch of the section information we pass around a
> lot into a struct, and passes a pointer to a single one of those
> instead.
>
> Because it's more convenient, it also puts the section_vaddresses
> calculations in locate_section(), which no longer returns the allocation
> for section_addresses directly either.
>
> This shouldn't change the binary file output or the "grub-mkimage -v"
> output in any way.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  util/grub-mkimagexx.c | 273 ++++++++++++++++++++++----------------------------
>  1 file changed, 121 insertions(+), 152 deletions(-)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 0b7c00e2297..19aa87f7efe 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -93,6 +93,17 @@ struct fixup_block_list
>
>  #define ALIGN_ADDR(x) (ALIGN_UP((x), image_target->voidp_sizeof))
>
> +struct section_metadata
> +{
> +  Elf_Half num_sections;
> +  Elf_Shdr *sections;
> +  Elf_Addr *addrs;
> +  Elf_Addr *vaddrs;
> +  Elf_Half section_entsize;
> +  Elf_Shdr *symtab;
> +  const char *strtab;
> +};
> +
>  static int
>  is_relocatable (const struct grub_install_image_target_desc *image_target)
>  {
> @@ -508,9 +519,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>  /* Relocate symbols; note that this function overwrites the symbol table.
>     Return the address of a start symbol.  */
>  static Elf_Addr
> -SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
> -			   Elf_Shdr *symtab_section, Elf_Addr *section_vaddresses,
> -			   Elf_Half section_entsize, Elf_Half num_sections,
> +SUFFIX (relocate_symbols) (Elf_Ehdr *e, struct section_metadata *smd,
>  			   void *jumpers, Elf_Addr jumpers_addr,
>  			   Elf_Addr bss_start, Elf_Addr end,
>  			   const struct grub_install_image_target_desc *image_target)
> @@ -520,19 +529,18 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
>    Elf_Addr start_address = (Elf_Addr) -1;
>    Elf_Sym *sym;
>    Elf_Word i;
> -  Elf_Shdr *symtab_link_section;
> +  Elf_Shdr *symtab_section;
>    const char *symtab;
>    grub_uint64_t *jptr = jumpers;
>
> -  symtab_link_section
> -    = (Elf_Shdr *) ((char *) sections
> -		      + (grub_target_to_host32 (symtab_section->sh_link)
> -			 * section_entsize));
> -  symtab = (char *) e + grub_target_to_host (symtab_link_section->sh_offset);
> +  symtab_section = (Elf_Shdr *) ((char *) smd->sections
> +				 + grub_target_to_host32 (smd->symtab->sh_link)
> +				   * smd->section_entsize);
> +  symtab = (char *) e + grub_target_to_host (symtab_section->sh_offset);
>
> -  symtab_size = grub_target_to_host (symtab_section->sh_size);
> -  sym_size = grub_target_to_host (symtab_section->sh_entsize);
> -  symtab_offset = grub_target_to_host (symtab_section->sh_offset);
> +  symtab_size = grub_target_to_host (smd->symtab->sh_size);
> +  sym_size = grub_target_to_host (smd->symtab->sh_entsize);
> +  symtab_offset = grub_target_to_host (smd->symtab->sh_offset);
>    num_syms = symtab_size / sym_size;
>
>    for (i = 0, sym = (Elf_Sym *) ((char *) e + symtab_offset);
> @@ -560,12 +568,12 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
>  	  else
>  	    continue;
>  	}
> -      else if (cur_index >= num_sections)
> +      else if (cur_index >= smd->num_sections)
>  	grub_util_error ("section %d does not exist", cur_index);
>        else
>  	{
>  	  sym->st_value = (grub_target_to_host (sym->st_value)
> -			   + section_vaddresses[cur_index]);
> +			   + smd->vaddrs[cur_index]);
>  	}
>
>        if (image_target->elf_target == EM_IA_64 && ELF_ST_TYPE (sym->st_info)
> @@ -580,7 +588,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
>        grub_util_info ("locating %s at 0x%"  GRUB_HOST_PRIxLONG_LONG
>  		      " (0x%"  GRUB_HOST_PRIxLONG_LONG ")", name,
>  		      (unsigned long long) sym->st_value,
> -		      (unsigned long long) section_vaddresses[cur_index]);
> +		      (unsigned long long) smd->vaddrs[cur_index]);
>
>        if (start_address == (Elf_Addr)-1)
>  	if (strcmp (name, "_start") == 0 || strcmp (name, "start") == 0)
> @@ -638,9 +646,9 @@ SUFFIX (count_funcs) (Elf_Ehdr *e, Elf_Shdr *symtab_section,
>  #endif
>
>  #ifdef MKIMAGE_ELF32
> -/* Deal with relocation information. This function relocates addresses
> +/* Deal with relocation information. This function relocates addrs

I think that this and comments below are changed by mistake.
If it is true I can fix it during commit.

Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v3 5/7] mkimage: refactor a bunch of section data into a struct.

[Peter Jones]

On Fri, Feb 23, 2018 at 03:51:07PM +0100, Daniel Kiper wrote:
> On Wed, Feb 21, 2018 at 03:20:27PM -0500, Peter Jones wrote:
> > This basically moves a bunch of the section information we pass around a
> > lot into a struct, and passes a pointer to a single one of those
> > instead.
> >
> > Because it's more convenient, it also puts the section_vaddresses
> > calculations in locate_section(), which no longer returns the allocation
> > for section_addresses directly either.
> >
> > This shouldn't change the binary file output or the "grub-mkimage -v"
> > output in any way.
...

> >
> >  #ifdef MKIMAGE_ELF32
> > -/* Deal with relocation information. This function relocates addresses
> > +/* Deal with relocation information. This function relocates addrs
> 
> I think that this and comments below are changed by mistake.
> If it is true I can fix it during commit.

You're right, and also I should have removed the middle paragraph of the
commit message, because that part is done in a prior patch now.

Thanks for the careful read through.

-- 
  Peter

[PATCH v3 6/7] mkimage: avoid copying relocations for sections that won't be copied.

[Peter Jones]

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 <pjones@redhat.com>
---
 util/grub-mkimagexx.c | 82 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 9 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 19aa87f7efe..549a63bcf4d 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -725,7 +725,13 @@ arm_get_trampoline_size (Elf_Ehdr *e,
 }
 #endif
 
-/* Deal with relocation information. This function relocates addrs
+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,
+				struct section_metadata *smd);
+
+/* Deal with relocation information. This function relocates addresses
    within the virtual address space starting from 0. So only relative
    addrs can be fully resolved. Absolute addrs must be relocated
    again by a PE32 relocator when loaded.  */
@@ -759,6 +765,14 @@ SUFFIX (relocate_addrs) (Elf_Ehdr *e, struct section_metadata *smd,
 	Elf_Shdr *target_section;
 	Elf_Word j;
 
+	if (!SUFFIX (is_kept_section) (s, image_target) &&
+	    !SUFFIX (is_kept_reloc_section) (s, image_target, smd))
+	  {
+	    grub_util_info ("not translating relocations for omitted section %s",
+			smd->strtab + grub_le_to_cpu32 (s->sh_name));
+	    continue;
+	  }
+
 	target_section_index = grub_target_to_host32 (s->sh_info);
 	target_section_addr = smd->addrs[target_section_index];
 	target_section = (Elf_Shdr *) ((char *) smd->sections
@@ -1663,6 +1677,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, smd))
+	  {
+	    grub_util_info ("not translating the skipped relocation section %s",
+			    smd->strtab + grub_le_to_cpu32 (s->sh_name));
+	    continue;
+	  }
+
 	grub_util_info ("translating the relocation section %s",
 			smd->strtab + grub_le_to_cpu32 (s->sh_name));
 
@@ -1738,6 +1759,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,
+				struct section_metadata *smd)
+{
+  int i;
+  int r = 0;
+  const char *name = smd->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 = smd->sections; i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
+    {
+      const char *sname = smd->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)
@@ -2062,14 +2133,7 @@ SUFFIX (grub_mkimage_load_image) (const char *kernel_path,
   for (i = 0, s = smd.sections;
        i < smd.num_sections;
        i++, s = (Elf_Shdr *) ((char *) s + smd.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 + smd.addrs[i], 0,
-- 
2.15.0

Re: [PATCH v3 6/7] mkimage: avoid copying relocations for sections that won't be copied.

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:28PM -0500, Peter Jones wrote:
> 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 <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v3 7/7] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Peter Jones]

This way debuginfo built from the .module will still include this
information, but the final result won't have the data we don't actually
need in the modules, either on-disk, loaded at runtime, or in prebuilt
images.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 grub-core/genmod.sh.in | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index 3de06ee018f..1250589b3f5 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -58,6 +58,10 @@ if test x@TARGET_APPLE_LINKER@ != x1; then
 		-K grub_mod_init -K grub_mod_fini \
 		-K _grub_mod_init -K _grub_mod_fini \
 		-R .note.gnu.gold-version -R .note.GNU-stack \
+		-R .gnu.build.attributes \
+		-R .rel.gnu.build.attributes \
+		-R .rela.gnu.build.attributes \
+		-R .eh_frame -R .rela.eh_frame -R .rel.eh_frame \
 		-R .note -R .comment -R .ARM.exidx $tmpfile || exit 1
 	fi
 	if ! test -z "${TARGET_OBJ2ELF}"; then
-- 
2.15.0

Re: [PATCH v3 7/7] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:29PM -0500, Peter Jones wrote:
> This way debuginfo built from the .module will still include this
> information, but the final result won't have the data we don't actually
> need in the modules, either on-disk, loaded at runtime, or in prebuilt
> images.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v3 7/7] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Daniel Kiper]

On Fri, Feb 23, 2018 at 03:54:49PM +0100, Daniel Kiper wrote:
> On Wed, Feb 21, 2018 at 03:20:29PM -0500, Peter Jones wrote:
> > This way debuginfo built from the .module will still include this
> > information, but the final result won't have the data we don't actually
> > need in the modules, either on-disk, loaded at runtime, or in prebuilt
> > images.
> >
> > Signed-off-by: Peter Jones <pjones@redhat.com>
>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

If there are no objections I will apply this patch series by the end of next week.

Daniel

Re: [PATCH v3 1/7] aout.h: Fix missing include.

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:23PM -0500, Peter Jones wrote:
> grub_aout_load() has a grub_file_t parameter, and depending on what order
> includes land in, it's sometimes not defined.  This patch explicitly adds
> file.h to aout.h so that it will always be defined.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

[Peter Jones]

On Tue, Feb 20, 2018 at 03:48:44PM +0100, Daniel Kiper wrote:
> On Wed, Jan 31, 2018 at 11:26:59AM -0500, Peter Jones wrote:
> > +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);
> 
> Ugh... Could not you create a struct and pass the pointer to it here?

Fair enough - I did it that way because the whole file seems averse to
that sort of thing, and once you start doing it, it gets a bit messy.
Nevertheless, I'll send you a patchset that includes that in just a few
minutes.

> PS Please CC me on the patches next time.

Will do.

-- 
  Peter