Re: [PATCH] ld (bfd) arm32 elf invalid strings offset in .strtab

["tlaronde at polynum dot com" <tlaronde at polynum dot com>]

On Wed, Aug 21, 2019 at 09:09:41AM +0930, Alan Modra wrote:
> On Tue, Aug 20, 2019 at 05:46:52PM +0200, tlaronde@polynum.com wrote:
> > diff --git a/bfd/elf.c b/bfd/elf.c
> > index 42ae162..dd7efd8 100644
> > --- a/bfd/elf.c
> > +++ b/bfd/elf.c
> > @@ -498,6 +498,19 @@ bfd_elf_get_elf_syms (bfd *ibfd,
> >  
> >    /* Convert the symbols to internal form.  */
> >    isymend = intsym_buf + symcount;
> > +	{
> > +		/* arm32 deals with names during the call of (*swap_symbol_in).
> > +		 * It thus needs to associate the correct strings to the
> > +		 * symbols. So we temporarily redefine the private data with
> > +		 * the correct symtab_hdr so that using elf_symtab_hdr (ibfd)
> > +		 * will give the correct information and we restore after in
> > +		 * order to keep it as it for the various callers.
> > +		 */
> > +		Elf_Internal_Shdr bak;	
> > +
> > +		bak = elf_symtab_hdr (ibfd);
> > +		elf_symtab_hdr (ibfd) = *symtab_hdr;
> > +
> >    for (esym = (const bfd_byte *) extsym_buf, isym = intsym_buf,
> >  	   shndx = extshndx_buf;
> >         isym < isymend;
> > @@ -512,8 +525,11 @@ bfd_elf_get_elf_syms (bfd *ibfd,
> >  	if (alloc_intsym != NULL)
> >  	  free (alloc_intsym);
> >  	intsym_buf = NULL;
> > +			elf_symtab_hdr (ibfd) = bak;
> >  	goto out;
> >        }
> > +		elf_symtab_hdr (ibfd) = bak;
> > +	}
> >  
> >   out:
> >    if (alloc_ext != NULL)
> 
> This is just piling on horrible hacks.  elf32_arm_swap_symbol_in
> should not be calling bfd_elf_sym_name.  I would recommend just
> deleting ARM_{GET,SET}_SYM_CMSE_SPCL and instead replacing uses of
> ARM_GET_SYM_CMSE_SPCL with a string compare of the name with
> CMSE_PREFIX.  I don't believe there is any need to use
> st_target_internal & 4 to cache the result of a name comparison,
> and swap_symbol_in is not the best place to do so in terms of speed.
> 
> BTW, if you and the ARM maintainers do want to continue with
> horrible hacks, the following in elf32_arm_swap_symbol_in probably
> will work.

As I mentionned in my report, I investigated the bug because it exists
and because the ones who try to tackle it let it drop; but I had no knowledge
of BFD or low level ARM and don't claim to have a correct view of the
whole. Hence my proposal to
address the problem by limiting the impact since I do not know and do
not have the time to dive in it at the moment; to correct _this_ bug without
risking making part or whole collapse.

The bug is here---I have given means to reproduce. The problematic chunk
of code has been identified. I hope that indeed someone with a far better knowledge
of the code will correct it.

And BTW, since the problem I saw is indeed linked to .strtab being used
instead of .dynstr, I gather your patch below will work.

But one way or another, it has to be addressed since a lot of people are
puzzled by the linker messages wondering if it is indeed safe even to
run the binaries.

> 
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index f1895df278..430c8feaa5 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -19960,7 +19960,10 @@ elf32_arm_swap_symbol_in (bfd * abfd,
>      ARM_SET_SYM_BRANCH_TYPE (dst->st_target_internal, ST_BRANCH_UNKNOWN);
>  
>    /* Mark CMSE special symbols.  */
> -  symtab_hdr = & elf_symtab_hdr (abfd);
> +  if ((abfd->flags & DYNAMIC) == 0 || elf_dynsymtab (abfd) == 0)
> +    symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
> +  else
> +    symtab_hdr = &elf_tdata (abfd)->dynsymtab_hdr;
>    if (symtab_hdr->sh_size)
>      name = bfd_elf_sym_name (abfd, symtab_hdr, dst, NULL);
>    if (name && CONST_STRNEQ (name, CMSE_PREFIX))
> 

Best regards,
-- 
        Thierry Laronde <tlaronde +AT+ polynum +dot+ com>
                     http://www.kergis.com/
                       http://www.sbfa.fr/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89  250D 52B1 AE95 6006 F40C