[v2] elf: Avoid linker crash on corrupt .eh_frame section

Message ID CAMe9rOr6hqJ967wcU+hW=sA9PWo_ZmUbfVNOR_rn+ugQFvC-0w@mail.gmail.com
State New
Headers
Series [v2] elf: Avoid linker crash on corrupt .eh_frame section |

Commit Message

H.J. Lu Sept. 23, 2025, 9:01 p.m. UTC
  On Wed, Sep 24, 2025 at 4:46 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Sep 23, 2025 at 10:50 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 23.09.2025 04:23, H.J. Lu wrote:
> > > Return false on corrupt .eh_frame section.  Cap the .eh_frame section
> > > alignment to the larger of the section size and the pointer size.
> >
> > Like for patch 2, I'm unconvinced of it being a good idea to build in
> > heuristics like this. What if someone feels a need to align .eh_frame
> > to, say, a page boundary?
>
> .eh_frame section is special:
>
>      eh_alignment = ((1 << o->alignment_power)
>                       * bfd_octets_per_byte (output_bfd, o));
>       /* Skip over zero terminator, and prevent empty sections from
>          adding alignment padding at the end.  */
>       for (i = o->map_tail.s; i != NULL; i = i->map_tail.s)
>         if (i->size == 0)
>           i->flags |= SEC_EXCLUDE;
>         else if (i->size > 4)
>           break;
>       /* The last non-empty eh_frame section doesn't need padding.  */
>       if (i != NULL)
>         i = i->map_tail.s;
>       /* Any prior sections must pad the last FDE out to the output
>          section alignment.  Otherwise we might have zero padding
>          between sections, which would be seen as a terminator.  */
>       for (; i != NULL; i = i->map_tail.s)
>         if (i->size == 4)
>           /* All but the last zero terminator should have been removed.  */
>           BFD_FAIL ();
>         else
>           {
>             bfd_size_type size
>               = (i->size + eh_alignment - 1) & -eh_alignment;
>
> If .eh_frame section alignment is larger than the section size,
> we will get buffer overflow.
>
>             if (i->size != size)
>               {
>                 i->size = size;
>                 changed = 1;
>                 eh_changed = 1;
>               }
>           }
>
> > Also, please format unsigned long-s using %lu or (here) %lx. Especially
> > when values get large, imo printing them as decimal is unhelpful.
>
> Will do.
>
> > Finally - why would .eh_frame be special in this regard? What about,
> > say, .sframe?
> >
>
> .sframe section doesn't do the code above.
>
> --
> H.J.

Normally, .eh_frame section is aligned to 8 or 4 bytes, depending on ELF
class.  But linker may add alignment padding at the end of .eh_frame
section.  If .eh_frame section alignment is larger than the section size,
alignment padding will go beyond the .eh_frame section size.  Cap the
.eh_frame section alignment to the larger of the section size and the
pointer size.  Section size is checked since the .eh_frame section
alignment may be larger than ELF class size.  Otherwise, Linux/x86-64
will get these test failures:

./ld-new: tmpdir/eh3.o: warning: ignore .eh_frame section alignment (16), limit
it to 8
FAIL: ld-elf/eh3

../binutils/objdump: tmpdir/dump: warning: ignore .eh_frame section alignment (8
), limit it to 4
FAIL: ld-x86-64/pr18160

./ld-new: tmpdir/simple-x32.o: warning: ignore .eh_frame section alignment (8),
limit it to 4
FAIL: X32 DSO from x86-64 object

Also return false on corrupt .eh_frame section.

PR ld/33453
* elf-bfd.h (ABI_64_P): New.
* elf-eh-frame.c (_bfd_elf_write_section_eh_frame): Return false
on corrupt .eh_frame section.
* elf.c (_bfd_elf_make_section_from_shdr): Limit .eh_frame section
alignment.
* elfxx-mips.c (ABI_64_P): Removed.
* elfxx-sparc.c (ABI_64_P): Likewise.
* elfxx-tilegx.c (ABI_64_P): Likewise.
* elfxx-x86.h (ABI_64_P): Likewise.

OK for master?
  

Patch

From aabdad718634d43e36b3da7b77753267af9d4851 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 23 Sep 2025 08:25:49 +0800
Subject: [PATCH v2] elf: Avoid linker crash on corrupt .eh_frame section

Normally, .eh_frame section is aligned to 8 or 4 bytes, depending on ELF
class.  But linker may add alignment padding at the end of .eh_frame
section.  If .eh_frame section alignment is larger than the section size,
alignment padding will go beyond the .eh_frame section size.  Cap the
.eh_frame section alignment to the larger of the section size and the
pointer size.  Section size is checked since the .eh_frame section
alignment may be larger than ELF class size.  Otherwise, Linux/x86-64
will get these test failures:

./ld-new: tmpdir/eh3.o: warning: ignore .eh_frame section alignment (16), limit it to 8
FAIL: ld-elf/eh3

../binutils/objdump: tmpdir/dump: warning: ignore .eh_frame section alignment (8), limit it to 4
FAIL: ld-x86-64/pr18160

./ld-new: tmpdir/simple-x32.o: warning: ignore .eh_frame section alignment (8), limit it to 4
FAIL: X32 DSO from x86-64 object

Also return false on corrupt .eh_frame section.

	PR ld/33453
	* elf-bfd.h (ABI_64_P): New.
	* elf-eh-frame.c (_bfd_elf_write_section_eh_frame): Return false
	on corrupt .eh_frame section.
	* elf.c (_bfd_elf_make_section_from_shdr): Limit .eh_frame section
	alignment.
	* elfxx-mips.c (ABI_64_P): Removed.
	* elfxx-sparc.c (ABI_64_P): Likewise.
	* elfxx-tilegx.c (ABI_64_P): Likewise.
	* elfxx-x86.h (ABI_64_P): Likewise.

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 bfd/elf-bfd.h      |  3 +++
 bfd/elf-eh-frame.c |  5 +++++
 bfd/elf.c          | 21 +++++++++++++++++++--
 bfd/elfxx-mips.c   |  4 ----
 bfd/elfxx-sparc.c  |  3 ---
 bfd/elfxx-tilegx.c |  3 ---
 bfd/elfxx-x86.h    |  3 ---
 7 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 51e6ae7bb78..5a131d735ff 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1919,6 +1919,9 @@  struct bfd_elf_section_data
 #define get_elf_backend_data(abfd) \
    xvec_get_elf_backend_data ((abfd)->xvec)
 
+#define ABI_64_P(abfd) \
+  (get_elf_backend_data (abfd)->s->elfclass == ELFCLASS64)
+
 /* The least object attributes (within an attributes subsection) known
    for any target.  Some code assumes that the value 0 is not used and
    the field for that attribute can instead be used as a marker to
diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
index 30bb313489c..93d412b38f0 100644
--- a/bfd/elf-eh-frame.c
+++ b/bfd/elf-eh-frame.c
@@ -2126,6 +2126,11 @@  _bfd_elf_write_section_eh_frame (bfd *abfd,
 
 	  /* Skip length.  */
 	  cie = ent->u.fde.cie_inf;
+	  if (cie == NULL)
+	    {
+	      _bfd_error_handler (_("corrupt .eh_frame section"));
+	      return false;
+	    }
 	  buf += 4;
 	  value = ((ent->new_offset + sec->output_offset + 4)
 		   - (cie->new_offset + cie->u.cie.u.sec->output_offset));
diff --git a/bfd/elf.c b/bfd/elf.c
index 6ef60301091..04d9e532b02 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -981,10 +981,27 @@  _bfd_elf_make_section_from_shdr (bfd *abfd,
 	}
     }
 
+  bfd_size_type alignment = hdr->sh_addralign;
+  if (streq (name, ".eh_frame"))
+    {
+      bfd_size_type eh_alignment = ABI_64_P (abfd) ? 8 : 4;
+      if (hdr->sh_size > eh_alignment)
+	eh_alignment = 1 << bfd_log2 (hdr->sh_size);
+      if (alignment > eh_alignment)
+	{
+	  _bfd_error_handler
+	    /* xgettext:c-format */
+	    (_("%pB: warning: ignore .eh_frame section alignment (0x%lx), limit it to 0x%lx"),
+	     abfd, (unsigned long) alignment,
+	     (unsigned long) eh_alignment);
+	  alignment = eh_alignment;
+	}
+    }
+
   if (!bfd_set_section_vma (newsect, hdr->sh_addr / opb)
       || !bfd_set_section_size (newsect, hdr->sh_size)
-      || !bfd_set_section_alignment (newsect, bfd_log2 (hdr->sh_addralign
-							& -hdr->sh_addralign)))
+      || !bfd_set_section_alignment (newsect, bfd_log2 (alignment
+							& -alignment)))
     return false;
 
   /* As a GNU extension, if the name begins with .gnu.linkonce, we
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index bf3fd7df805..181f9f5e9fc 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -816,10 +816,6 @@  static bfd *reldyn_sorting_bfd;
 #define ABI_N32_P(abfd) \
   ((elf_elfheader (abfd)->e_flags & EF_MIPS_ABI2) != 0)
 
-/* Nonzero if ABFD is using the N64 ABI.  */
-#define ABI_64_P(abfd) \
-  (get_elf_backend_data (abfd)->s->elfclass == ELFCLASS64)
-
 /* Nonzero if ABFD is using NewABI conventions.  */
 #define NEWABI_P(abfd) (ABI_N32_P (abfd) || ABI_64_P (abfd))
 
diff --git a/bfd/elfxx-sparc.c b/bfd/elfxx-sparc.c
index bbaa7829132..1f3b9d831d8 100644
--- a/bfd/elfxx-sparc.c
+++ b/bfd/elfxx-sparc.c
@@ -37,9 +37,6 @@ 
 /* In case we're on a 32-bit machine, construct a 64-bit "-1" value.  */
 #define MINUS_ONE (~ (bfd_vma) 0)
 
-#define ABI_64_P(abfd) \
-  (get_elf_backend_data (abfd)->s->elfclass == ELFCLASS64)
-
 /* The relocation "howto" table.  */
 
 /* Utility for performing the standard initial work of an instruction
diff --git a/bfd/elfxx-tilegx.c b/bfd/elfxx-tilegx.c
index 79358e6b204..577d259a3b8 100644
--- a/bfd/elfxx-tilegx.c
+++ b/bfd/elfxx-tilegx.c
@@ -27,9 +27,6 @@ 
 #include "libiberty.h"
 #include "elfxx-tilegx.h"
 
-#define ABI_64_P(abfd) \
-  (get_elf_backend_data (abfd)->s->elfclass == ELFCLASS64)
-
 #define TILEGX_ELF_WORD_BYTES(htab) \
   ((htab)->bytes_per_word)
 
diff --git a/bfd/elfxx-x86.h b/bfd/elfxx-x86.h
index 1ebc9d2f2e5..902592a3cf0 100644
--- a/bfd/elfxx-x86.h
+++ b/bfd/elfxx-x86.h
@@ -102,9 +102,6 @@ 
    header.  */
 #define PLT_SFRAME_FDE_START_OFFSET	sizeof (sframe_header)
 
-#define ABI_64_P(abfd) \
-  (get_elf_backend_data (abfd)->s->elfclass == ELFCLASS64)
-
 /* If ELIMINATE_COPY_RELOCS is non-zero, the linker will try to avoid
    copying dynamic variables from a shared lib into an app's dynbss
    section, and instead use a dynamic relocation to point into the
-- 
2.51.0