diff mbox series

[3/4] elf: Avoid linker crash on corrupt .eh_frame section

Message ID CAMe9rOqP44oi5pxb7LNsEPK0+owjjdWdQW_f6Qp5Zb8o-WTE+Q@mail.gmail.com
State New
Headers
Series [1/4] elf: Disallow the empty symbol name |

Commit Message

H.J. Lu Sept. 23, 2025, 2:23 a.m. UTC 
  Return false on corrupt .eh_frame section.  Cap the .eh_frame section
alignment to the larger of the section size and the pointer size.

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.

Comments

Alan Modra Sept. 23, 2025, 11:58 a.m. UTC | #1 
On Tue, Sep 23, 2025 at 10:23:45AM +0800, 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.
> 
> 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.
> 
> -- 
> H.J.

> From dc96baa76c046ed34b05926a6401c6ebd0d1eb03 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 3/4] elf: Avoid linker crash on corrupt .eh_frame section
> 
> Return false on corrupt .eh_frame section.  Cap the .eh_frame section
> alignment to the larger of the section size and the pointer size.
> 
> 	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..07428cb91f9 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);

sh_size??

> +      if (alignment > eh_alignment)
> +	{
> +	  _bfd_error_handler
> +	    /* xgettext:c-format */
> +	    (_("%pB: warning: ignore .eh_frame section alignment (%ld), limit it to %ld"),
> +	     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
>
Jan Beulich Sept. 23, 2025, 2:50 p.m. UTC | #2 
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?

Also, please format unsigned long-s using %lu or (here) %lx. Especially
when values get large, imo printing them as decimal is unhelpful.

Finally - why would .eh_frame be special in this regard? What about,
say, .sframe?

Jan
H.J. Lu Sept. 23, 2025, 8:42 p.m. UTC | #3 
On Tue, Sep 23, 2025 at 7:58 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Tue, Sep 23, 2025 at 10:23:45AM +0800, 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.
> >
> > 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.
> >
> > --
> > H.J.
>
> > From dc96baa76c046ed34b05926a6401c6ebd0d1eb03 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 3/4] elf: Avoid linker crash on corrupt .eh_frame section
> >
> > Return false on corrupt .eh_frame section.  Cap the .eh_frame section
> > alignment to the larger of the section size and the pointer size.
> >
> >       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..07428cb91f9 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);
>
> sh_size??

Normally, .eh_frame section is aligned to 8 or 4 bytes, depending on ELF
class.  But a larger alignment is possible.  Without

eh_alignment = 1 << bfd_log2 (hdr->sh_size);

I got:

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

/export/build/gnu/tools-build/binutils-gitlab-test/build-x86_64-linux/ld/../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

The section size is a reasonable upper bound.

> > +      if (alignment > eh_alignment)
> > +     {
> > +       _bfd_error_handler
> > +         /* xgettext:c-format */
> > +         (_("%pB: warning: ignore .eh_frame section alignment (%ld), limit it to %ld"),
> > +          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
> >
>
>
> --
> Alan Modra
H.J. Lu Sept. 23, 2025, 8:46 p.m. UTC | #4 
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.
Jan Beulich Sept. 25, 2025, 6:06 a.m. UTC | #5 
On 23.09.2025 22:46, H.J. Lu 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.

I understand that, but I hoped you would also understand that I'm merely
trying to give examples. In all of your reply you didn't address my question
"What if someone feels a need to align .eh_frame to, say, a page boundary?"
If someone doing so resulted in a buffer overflow, that would indeed want
fixing. But not by preventing them to use that kind of alignment.

Jan
H.J. Lu Sept. 25, 2025, 6:32 a.m. UTC | #6 
On Thu, Sep 25, 2025 at 2:06 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.09.2025 22:46, H.J. Lu 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.
>
> I understand that, but I hoped you would also understand that I'm merely
> trying to give examples. In all of your reply you didn't address my question
> "What if someone feels a need to align .eh_frame to, say, a page boundary?"
> If someone doing so resulted in a buffer overflow, that would indeed want
> fixing. But not by preventing them to use that kind of alignment.
>
> Jan

So far, we don't have any request for large .eh_frame section alignment.
My patch simply changes the linker crash to a linker error.  If people
do need large .eh_frame section alignments in the future, we can work
with them to resolve their issues.
Jan Beulich Sept. 25, 2025, 6:53 a.m. UTC | #7 
On 25.09.2025 08:32, H.J. Lu wrote:
> On Thu, Sep 25, 2025 at 2:06 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 23.09.2025 22:46, H.J. Lu 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.
>>
>> I understand that, but I hoped you would also understand that I'm merely
>> trying to give examples. In all of your reply you didn't address my question
>> "What if someone feels a need to align .eh_frame to, say, a page boundary?"
>> If someone doing so resulted in a buffer overflow, that would indeed want
>> fixing. But not by preventing them to use that kind of alignment.
> 
> So far, we don't have any request for large .eh_frame section alignment.
> My patch simply changes the linker crash to a linker error.  If people
> do need large .eh_frame section alignments in the future, we can work
> with them to resolve their issues.

That's not how I look at things, sorry. If Alan or Nick want to approve this
patch, I'm not going to stand in the way, but I won't (and I object to you
putting it in without approval, ftaod).

Jan
Nick Clifton Oct. 3, 2025, 1:13 p.m. UTC | #8 
Hi Guys,

>>>>>> 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?

>> So far, we don't have any request for large .eh_frame section alignment.
>> My patch simply changes the linker crash to a linker error.  If people
>> do need large .eh_frame section alignments in the future, we can work
>> with them to resolve their issues.
> 
> That's not how I look at things, sorry. If Alan or Nick want to approve this
> patch, I'm not going to stand in the way, but I won't (and I object to you
> putting it in without approval, ftaod).

I am with Jan here.  If we are going to implement a cap on the .eh_frame
section size then we need to a) document it and b) inform the user if the
cap is ever exceeded, oh and c) not break if it happens.

Setting an arbitrary limit of max(pointer size, section size) makes sense
from a practical point of view, but we must be ready to revisit this decision
if it turns out that there are real world situations where a larger alignment
(eg page boundary) is desired.

So either the code must support any (power of two) alignment, or it must
be friendly to the user and let them know if a limit is reached.

Cheers
   Nick
diff mbox series

Patch

From dc96baa76c046ed34b05926a6401c6ebd0d1eb03 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 3/4] elf: Avoid linker crash on corrupt .eh_frame section

Return false on corrupt .eh_frame section.  Cap the .eh_frame section
alignment to the larger of the section size and the pointer size.

	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..07428cb91f9 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 (%ld), limit it to %ld"),
+	     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