Re: [PATCH v4 3/7] bfd: Improve nm and objdump without section header

[Fangrui Song <i@maskray.me>]

On Wed, Jul 12, 2023 at 10:02 PM Alan Modra via Binutils
<binutils@sourceware.org> wrote:
>
> On Sun, Jul 09, 2023 at 11:30:01PM -0400, Simon Marchi wrote:
> >
> > > It works for me:
> > >
> > > $ make check TESTS="gdb.base/eu-strip-infcall.exp"
> > > ....
> > > === gdb Summary ===
> > >
> > > # of expected passes 1
> > >
> > > My change only impacts files without section header. eu-strip-infcall.exp does
> > > "eu-strip -f ${binfile}.debug $binfile", which doesn't remove section header.
> > >
> >
> > I can reliably reproduce the problem on two separate machine, one Ubuntu
> > 22.04 and one failrly up to date Arch Linux.  elfutils version 0.186 and
> > 0.189, respectively.
> >
> > It goes wrong when GDB does a bfd_check_format call on
> > testsuite/outputs/gdb.base/eu-strip-infcall/eu-strip-infcall.debug.
> > Before you commit it works, and after your commit it returns false.  It
> > happens in this new statement added to elf_object_p, added by the commit:
> >
> >             if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize)
> >               goto got_no_match;
> >
> > (top-gdb) p i_phdr->p_offset
> > $1 = 8192
> > (top-gdb) p i_phdr->p_filesz
> > $2 = 196
> > (top-gdb) p filesize
> > $3 = 5104
> > (top-gdb) p i
> > $4 = 4
> >
> > It would be this program header causing the condition to fail:
> >
> >   Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
> >   ...
> >   LOAD           0x002000 0x0000000000002000 0x0000000000002000 0x0000c4 0x0000c4 R   0x1000
> >
> > So, the program header of the .debug file describes the segments of the
> > main binary, not sure if that's expected.
>
> No, that's not expected.  Program headers in a .debug file ought to
> describe the contents of the debug file.  You'll typically see many
> with p_filesz zero.  eu-strip appears to be broken in this respect.

Do you have a bug number for eu-strip ? It seems good to keep a
reference of the bug
so that we can track it.

> There is another problem with the code added to elf_object_p:
> _bfd_elf_get_dynamic_symbols is told that it can access up to e_phnum
> program headers, but they very likely haven't all been swapped in.
>
> I'm going to apply the following patch.
> ----
>
> elf_object_p load of dynamic symbols
>
> This fixes an uninitialised memory access on a fuzzed file:
> 0 0xf22e9b in offset_from_vma /src/binutils-gdb/bfd/elf.c:1899:2
> 1 0xf1e90f in _bfd_elf_get_dynamic_symbols /src/binutils-gdb/bfd/elf.c:2099:13
> 2 0x10e6a54 in bfd_elf32_object_p /src/binutils-gdb/bfd/elfcode.h:851:9
>
> Hopefully it will also stop any attempt to load dynamic symbols from
> eu-strip debug files.
>
>         * elfcode.h (elf_object_p): Do not attempt to load dynamic
>         symbols for a file with no section headers until all the
>         program headers are swapped in.  Do not fail on eu-strip debug
>         files.
>
> diff --git a/bfd/elfcode.h b/bfd/elfcode.h
> index aae66bcebf8..b2277921680 100644
> --- a/bfd/elfcode.h
> +++ b/bfd/elfcode.h
> @@ -819,6 +819,7 @@ elf_object_p (bfd *abfd)
>         goto got_no_match;
>        if (bfd_seek (abfd, (file_ptr) i_ehdrp->e_phoff, SEEK_SET) != 0)
>         goto got_no_match;
> +      bool eu_strip_broken_phdrs = false;
>        i_phdr = elf_tdata (abfd)->phdr;
>        for (i = 0; i < i_ehdrp->e_phnum; i++, i_phdr++)
>         {
> @@ -839,21 +840,31 @@ elf_object_p (bfd *abfd)
>                   abfd->read_only = 1;
>                 }
>             }
> -         if (i_phdr->p_filesz != 0)
> -           {
> -             if ((i_phdr->p_offset + i_phdr->p_filesz) > filesize)
> -               goto got_no_match;
> -             /* Try to reconstruct dynamic symbol table from PT_DYNAMIC
> -                segment if there is no section header.  */
> -             if (i_phdr->p_type == PT_DYNAMIC
> -                 && i_ehdrp->e_shstrndx == 0
> -                 && i_ehdrp->e_shoff == 0
> -                 && !_bfd_elf_get_dynamic_symbols (abfd, i_phdr,
> -                                                   elf_tdata (abfd)->phdr,
> -                                                   i_ehdrp->e_phnum,
> -                                                   filesize))
> -               goto got_no_match;
> -           }
> +         /* Detect eu-strip -f debug files, which have program
> +            headers that describe the original file.  */
> +         if (i_phdr->p_filesz != 0
> +             && (i_phdr->p_filesz > filesize
> +                 || i_phdr->p_offset > filesize - i_phdr->p_filesz))
> +           eu_strip_broken_phdrs = true;
> +       }
> +      if (!eu_strip_broken_phdrs
> +         && i_ehdrp->e_shoff == 0
> +         && i_ehdrp->e_shstrndx == 0)
> +       {
> +         /* Try to reconstruct dynamic symbol table from PT_DYNAMIC
> +            segment if there is no section header.  */
> +         i_phdr = elf_tdata (abfd)->phdr;
> +         for (i = 0; i < i_ehdrp->e_phnum; i++, i_phdr++)
> +           if (i_phdr->p_type == PT_DYNAMIC)
> +             {
> +               if (i_phdr->p_filesz != 0
> +                   && !_bfd_elf_get_dynamic_symbols (abfd, i_phdr,
> +                                                     elf_tdata (abfd)->phdr,
> +                                                     i_ehdrp->e_phnum,
> +                                                     filesize))
> +                 goto got_no_match;
> +               break;
> +             }
>         }
>      }
>
>
> --
> Alan Modra
> Australia Development Lab, IBM