[v2] elf: Don't match corrupt section header in linker input
Commit Message
On Sun, Sep 21, 2025 at 3:47 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Sep 19, 2025 at 7:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > Don't swap in nor match corrupt section header to avoid linker crash
> > later.
> >
> > PR ld/33457
> > * elfcode.h (elf_swap_shdr_in): Changed to return bool. Return
> > false for corrupt section header.
> > (elf_object_p): Don't match corrupt section header.
> >
> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> > ---
> > bfd/elfcode.h | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/bfd/elfcode.h b/bfd/elfcode.h
> > index 9c65852e103..04a51918a0e 100644
> > --- a/bfd/elfcode.h
> > +++ b/bfd/elfcode.h
> > @@ -311,7 +311,7 @@ elf_swap_ehdr_out (bfd *abfd,
> > /* Translate an ELF section header table entry in external format into an
> > ELF section header table entry in internal format. */
> >
> > -static void
> > +static bool
> > elf_swap_shdr_in (bfd *abfd,
> > const Elf_External_Shdr *src,
> > Elf_Internal_Shdr *dst)
> > @@ -341,7 +341,8 @@ elf_swap_shdr_in (bfd *abfd,
> > {
> > _bfd_error_handler (_("warning: %pB has a section "
> > "extending past end of file"), abfd);
> > - abfd->read_only = 1;
> > + /* Don't match corrupt section header: PR ld/33457. */
> > + return false;
> > }
> > }
> > dst->sh_link = H_GET_32 (abfd, src->sh_link);
> > @@ -350,6 +351,7 @@ elf_swap_shdr_in (bfd *abfd,
> > dst->sh_entsize = H_GET_WORD (abfd, src->sh_entsize);
> > dst->bfd_section = NULL;
> > dst->contents = NULL;
> > + return true;
> > }
> >
> > /* Translate an ELF section header table entry in internal format into an
> > @@ -642,9 +644,9 @@ elf_object_p (bfd *abfd)
> >
> > /* Read the first section header at index 0, and convert to internal
> > form. */
> > - if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr))
> > + if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr)
> > + || !elf_swap_shdr_in (abfd, &x_shdr, &i_shdr))
> > goto got_no_match;
> > - elf_swap_shdr_in (abfd, &x_shdr, &i_shdr);
> >
> > /* If the section count is zero, the actual count is in the first
> > section header. */
> > @@ -730,9 +732,9 @@ elf_object_p (bfd *abfd)
> > to internal form. */
> > for (shindex = 1; shindex < i_ehdrp->e_shnum; shindex++)
> > {
> > - if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr))
> > + if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr)
> > + || !elf_swap_shdr_in (abfd, &x_shdr, i_shdrp + shindex))
> > goto got_no_match;
> > - elf_swap_shdr_in (abfd, &x_shdr, i_shdrp + shindex);
> >
> > /* Sanity check sh_link and sh_info. */
> > if (i_shdrp[shindex].sh_link >= num_sec)
> > --
> > 2.51.0
> >
>
> Any comments or objections?
>
Here is the v2 patch to reject link input with corrupt section header.
OK for master?
Thanks.
Comments
On 22.09.2025 01:34, H.J. Lu wrote:
> Here is the v2 patch to reject link input with corrupt section header.
What's the (user visible) effect of this failing to match? I.e. what
potentially entirely off diagnostic are they going to see? Or is there
perhaps a risk of seeing the same diagnostic multiple times, because
of further matching attempts? And is the warning issued here guaranteed
to be followed by another, stronger diagnostic?
Jan
On Tue, Sep 23, 2025 at 7:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.09.2025 01:34, H.J. Lu wrote:
> > Here is the v2 patch to reject link input with corrupt section header.
>
> What's the (user visible) effect of this failing to match? I.e. what
> potentially entirely off diagnostic are they going to see? Or is there
> perhaps a risk of seeing the same diagnostic multiple times, because
> of further matching attempts? And is the warning issued here guaranteed
> to be followed by another, stronger diagnostic?
>
> Jan
The corrupt linker input will be rejected by linker:
./ld --version-exports-section symbol --shared -o pr33457.so pr33457.o
./ld: warning: pr33457.o has a section extending past end of file
pr33457.o: file not recognized: file format not recognized
instead of linker crash.
On Tue, Sep 23, 2025 at 08:14:12AM +0800, H.J. Lu wrote:
> On Tue, Sep 23, 2025 at 7:21 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 22.09.2025 01:34, H.J. Lu wrote:
> > > Here is the v2 patch to reject link input with corrupt section header.
> >
> > What's the (user visible) effect of this failing to match? I.e. what
> > potentially entirely off diagnostic are they going to see? Or is there
> > perhaps a risk of seeing the same diagnostic multiple times, because
> > of further matching attempts? And is the warning issued here guaranteed
> > to be followed by another, stronger diagnostic?
> >
> > Jan
>
> The corrupt linker input will be rejected by linker:
>
> ./ld --version-exports-section symbol --shared -o pr33457.so pr33457.o
> ./ld: warning: pr33457.o has a section extending past end of file
> pr33457.o: file not recognized: file format not recognized
>
> instead of linker crash.
I think that's a good enough error. Patch is OK. I know from
experience with fuzzer induced misbehaviour in the various binutils
that without this sort of heavy-handed fix that you're forever fixing
lots of places that assume sane input.
From 5606dd570385c0b251d448bd07eb641d18309264 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 18 Sep 2025 16:59:25 -0700
Subject: [PATCH v2] elf: Don't match corrupt section header in linker input
Don't swap in nor match corrupt section header in linker input to avoid
linker crash later.
PR ld/33457
* elfcode.h (elf_swap_shdr_in): Changed to return bool. Return
false for corrupt section header in linker input.
(elf_object_p): Reject if elf_swap_shdr_in returns false.
Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
bfd/elfcode.h | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
@@ -311,7 +311,7 @@ elf_swap_ehdr_out (bfd *abfd,
/* Translate an ELF section header table entry in external format into an
ELF section header table entry in internal format. */
-static void
+static bool
elf_swap_shdr_in (bfd *abfd,
const Elf_External_Shdr *src,
Elf_Internal_Shdr *dst)
@@ -341,6 +341,9 @@ elf_swap_shdr_in (bfd *abfd,
{
_bfd_error_handler (_("warning: %pB has a section "
"extending past end of file"), abfd);
+ /* PR ld/33457: Don't match corrupt section header. */
+ if (abfd->is_linker_input)
+ return false;
abfd->read_only = 1;
}
}
@@ -350,6 +353,7 @@ elf_swap_shdr_in (bfd *abfd,
dst->sh_entsize = H_GET_WORD (abfd, src->sh_entsize);
dst->bfd_section = NULL;
dst->contents = NULL;
+ return true;
}
/* Translate an ELF section header table entry in internal format into an
@@ -642,9 +646,9 @@ elf_object_p (bfd *abfd)
/* Read the first section header at index 0, and convert to internal
form. */
- if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr))
+ if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr)
+ || !elf_swap_shdr_in (abfd, &x_shdr, &i_shdr))
goto got_no_match;
- elf_swap_shdr_in (abfd, &x_shdr, &i_shdr);
/* If the section count is zero, the actual count is in the first
section header. */
@@ -730,9 +734,9 @@ elf_object_p (bfd *abfd)
to internal form. */
for (shindex = 1; shindex < i_ehdrp->e_shnum; shindex++)
{
- if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr))
+ if (bfd_read (&x_shdr, sizeof x_shdr, abfd) != sizeof (x_shdr)
+ || !elf_swap_shdr_in (abfd, &x_shdr, i_shdrp + shindex))
goto got_no_match;
- elf_swap_shdr_in (abfd, &x_shdr, i_shdrp + shindex);
/* Sanity check sh_link and sh_info. */
if (i_shdrp[shindex].sh_link >= num_sec)
--
2.51.0