This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] ld (bfd) arm32 elf invalid strings offset in .strtab
Hello,
On Thu, Aug 22, 2019 at 02:32:14PM +0930, Alan Modra wrote:
> On Wed, Aug 21, 2019 at 12:35:03PM +0000, Tamar Christina wrote:
> > Hi Alan,
> >
> > Thanks for the reply,
> >
> > >
> > > 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.
> > >
> >
> > I may be missing something, but why not? Where would be a better place?
> > It seems to me that replacing a simple mask check with strcmp everywhere is a worse option,
> > but perhaps I am misunderstanding something here.
>
> Yes, see below. You might be getting more strncmp calls than you
> realise.
>
> > > 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.
> > >
> >
> > I am not an ARM maintainer so I can't speak for them,
> > but out of interest what makes the change you proposed to be a hack, and what other hack is it layering on top?
>
> Doing the strncmp in swap_symbol_in is the hack (*). As evidenced by
> this bug, swap_symbol_in wasn't designed for doing that. Name strings
> are set up later (for global symbols anyway). If you're concerned
> about speed you don't want to repeat the lookup of st_name in .strtab/
> .dynstr. Also for globals you don't want to strncmp on every
> __acle_se_foo when loading from the object file since more than one
> object file might weakly define __acle_se_foo. Do the strncmp *after*
> merging all the __acle_se_foo seen. Or in elf32_arm_link_hash_newfunc
> if you really want to be fast, but tricky!
>
> But I don't think speed was the reason why st_target_internal was used
> to cache a strncmp result. It was probably more a case of copying
> existing practice for other special symbols.
>
> *) Calling it "horrible" was an over-reaction.. I guess I was
> reacting to the second of Thierry's patches here
> https://sourceware.org/ml/binutils/2019-08/msg00191.html
> which was on not-so-good advice from Tamar to hack
> bfd_elf_get_elf_syms. The first of Thierry's patches looked better to
> me, and had an excellent description of the bug. Adding a symtab_hdr
> parameter to all ELF swap_symbol_in functions is reasonable and bomb
> proof, but does draw the attention of those pesky global maintainers..
>
> I'm going to apply the following.
> ----
> ARM CMSE symbols
>
> This patch removes use of st_target_internal to cache the result of
> comparing symbol names against CMSE_PREFIX. The problem with setting
> a bit in st_target_internal in swap_symbol_in is that calling
> bfd_elf_sym_name from swap_symbol_in requires symtab_hdr, and you
> don't know for sure whether swap_symbol_in is operating on dynsyms
> (and thus elf_tdata (abfd)->dynsymtab_hdr should be used) or on the
> normal symtab (thus elf_tdata (abfd)->symtab_hdr). You can make an
> educated guess based on abfd->flags & DYNAMIC but that relies on
> knowing a lot about calls to bfd_elf_get_elf_syms, and is fragile in
> the face of possible future changes.
>
> include/
> * elf/arm.h (ARM_GET_SYM_CMSE_SPCL, ARM_SET_SYM_CMSE_SPCL): Delete.
> bfd/
> * elf32-arm.c (cmse_scan): Don't use ARM_GET_SYM_CMSE_SPCL,
> instead recognize CMSE_PREFIX in symbol name.
> (elf32_arm_gc_mark_extra_sections): Likewise.
> (elf32_arm_filter_cmse_symbols): Don't test ARM_GET_SYM_CMSE_SPCL.
> (elf32_arm_swap_symbol_in): Don't invoke ARM_SET_SYM_CMSE_SPCL.
>
> diff --git a/bfd/ChangeLog b/bfd/ChangeLog
> index 97d3726516..a725504c02 100644
> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog
> @@ -1,3 +1,11 @@
> +2019-08-22 Alan Modra <amodra@gmail.com>
> +
> + * elf32-arm.c (cmse_scan): Don't use ARM_GET_SYM_CMSE_SPCL,
> + instead recognize CMSE_PREFIX in symbol name.
> + (elf32_arm_gc_mark_extra_sections): Likewise.
> + (elf32_arm_filter_cmse_symbols): Don't test ARM_GET_SYM_CMSE_SPCL.
> + (elf32_arm_swap_symbol_in): Don't invoke ARM_SET_SYM_CMSE_SPCL.
> +
> 2019-08-20 Dennis Zhang <dennis.zhang@arm.com>
>
> * cpu-aarch64.c: New entries for Cortex-A34, Cortex-A65,
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index d1548d6db3..b675fc60c1 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -6002,12 +6002,12 @@ cmse_scan (bfd *input_bfd, struct elf32_arm_link_hash_table *htab,
> if (i < ext_start)
> {
> cmse_sym = &local_syms[i];
> - /* Not a special symbol. */
> - if (!ARM_GET_SYM_CMSE_SPCL (cmse_sym->st_target_internal))
> - continue;
> sym_name = bfd_elf_string_from_elf_section (input_bfd,
> symtab_hdr->sh_link,
> cmse_sym->st_name);
> + if (!sym_name || !CONST_STRNEQ (sym_name, CMSE_PREFIX))
> + continue;
> +
> /* Special symbol with local binding. */
> cmse_invalid = TRUE;
> }
> @@ -6015,9 +6015,7 @@ cmse_scan (bfd *input_bfd, struct elf32_arm_link_hash_table *htab,
> {
> cmse_hash = elf32_arm_hash_entry (sym_hashes[i - ext_start]);
> sym_name = (char *) cmse_hash->root.root.root.string;
> -
> - /* Not a special symbol. */
> - if (!ARM_GET_SYM_CMSE_SPCL (cmse_hash->root.target_internal))
> + if (!CONST_STRNEQ (sym_name, CMSE_PREFIX))
> continue;
>
> /* Special symbol has incorrect binding or type. */
> @@ -15990,7 +15988,8 @@ elf32_arm_gc_mark_extra_sections (struct bfd_link_info *info,
>
> /* Assume it is a special symbol. If not, cmse_scan will
> warn about it and user can do something about it. */
> - if (ARM_GET_SYM_CMSE_SPCL (cmse_hash->root.target_internal))
> + if (CONST_STRNEQ (cmse_hash->root.root.root.string,
> + CMSE_PREFIX))
> {
> cmse_sec = cmse_hash->root.root.u.def.section;
> if (!cmse_sec->gc_mark
> @@ -18610,9 +18609,6 @@ elf32_arm_filter_cmse_symbols (bfd *abfd ATTRIBUTE_UNUSED,
> || cmse_hash->root.type != STT_FUNC)
> continue;
>
> - if (!ARM_GET_SYM_CMSE_SPCL (cmse_hash->root.target_internal))
> - continue;
> -
> syms[dst_count++] = sym;
> }
> free (cmse_name);
> @@ -19935,9 +19931,6 @@ elf32_arm_swap_symbol_in (bfd * abfd,
> const void *pshn,
> Elf_Internal_Sym *dst)
> {
> - Elf_Internal_Shdr *symtab_hdr;
> - const char *name = NULL;
> -
> if (!bfd_elf32_swap_symbol_in (abfd, psrc, pshn, dst))
> return FALSE;
> dst->st_target_internal = 0;
> @@ -19966,13 +19959,6 @@ elf32_arm_swap_symbol_in (bfd * abfd,
> else
> ARM_SET_SYM_BRANCH_TYPE (dst->st_target_internal, ST_BRANCH_UNKNOWN);
>
> - /* Mark CMSE special symbols. */
> - symtab_hdr = & elf_symtab_hdr (abfd);
> - if (symtab_hdr->sh_size)
> - name = bfd_elf_sym_name (abfd, symtab_hdr, dst, NULL);
> - if (name && CONST_STRNEQ (name, CMSE_PREFIX))
> - ARM_SET_SYM_CMSE_SPCL (dst->st_target_internal);
> -
> return TRUE;
> }
>
> diff --git a/include/ChangeLog b/include/ChangeLog
> index 1813cb38d8..e779c17702 100644
> --- a/include/ChangeLog
> +++ b/include/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-08-22 Alan Modra <amodra@gmail.com>
> +
> + * elf/arm.h (ARM_GET_SYM_CMSE_SPCL, ARM_SET_SYM_CMSE_SPCL): Delete.
> +
> 2019-08-09 Mihailo Stojanovic <mihailo.stojanovic@rt-rk.com>
>
> * elf/mips.h (SHT_GNU_XHASH): New define.
> diff --git a/include/elf/arm.h b/include/elf/arm.h
> index 5cb9970644..75fb5e26ca 100644
> --- a/include/elf/arm.h
> +++ b/include/elf/arm.h
> @@ -399,11 +399,4 @@ enum arm_st_branch_type {
> | ((TYPE) & ENUM_ARM_ST_BRANCH_TYPE_BITMASK))
> #endif
>
> -/* Get or set whether a symbol is a special symbol of an entry function of CMSE
> - secure code. */
> -#define ARM_GET_SYM_CMSE_SPCL(SYM_TARGET_INTERNAL) \
> - (((SYM_TARGET_INTERNAL) >> 2) & 1)
> -#define ARM_SET_SYM_CMSE_SPCL(SYM_TARGET_INTERNAL) \
> - (SYM_TARGET_INTERNAL) |= 4
> -
> #endif /* _ELF_ARM_H */
>
Thank you for the patch and the explanations. I'm always willing to
learn. As Tamar has indicated, will it be back-ported to 2.32? As
NetBSD, for example, has also branched for a next release using 2.32,
it would be simpler for it to just update to a bug fixing version
instead of upgrading or trying to apply diffs at random.
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