diff mbox series

[v5,6/7] csu: Move static pie self relocation later [BZ #27072]

Message ID b5dbaa167f4317c312e22fb761036d3570e6614a.1611155254.git.szabolcs.nagy@arm.com
State Committed
Commit 86d439b06fb29af8d063ee8855ff63a863f46ef3
Headers show
Series fix ifunc with static pie [BZ #27072] | expand

Commit Message

Szabolcs Nagy Jan. 20, 2021, 3:31 p.m. UTC
IFUNC resolvers may depend on tunables and cpu feature setup so
move static pie self relocation after those.

It is hard to guarantee that the ealy startup code does not rely
on relocations so this is a bit fragile. It would be more robust
to handle RELATIVE relocs early and only IRELATIVE relocs later,
but the current relocation processing code cannot do that.

The early startup code up to relocation processing includes

  _dl_aux_init (auxvec);
  __libc_init_secure ();
  __tunables_init (__environ);
  ARCH_INIT_CPU_FEATURES ();
  _dl_relocate_static_pie ();

These are simple enough that RELATIVE relocs can be avoided.

The following steps include

  ARCH_SETUP_IREL ();
  ARCH_SETUP_TLS ();
  ARCH_APPLY_IREL ();

On some targets IRELATIVE processing relies on TLS setup on
others TLS setup relies on IRELATIVE relocs, so the right
position for _dl_relocate_static_pie is target dependent.
For now move self relocation as early as possible on targets
that support static PIE.

Fixes bug 27072.
---
 csu/libc-start.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Adhemerval Zanella Jan. 21, 2021, 2:07 p.m. UTC | #1
On 20/01/2021 12:31, Szabolcs Nagy via Libc-alpha wrote:
> IFUNC resolvers may depend on tunables and cpu feature setup so
> move static pie self relocation after those.
> 
> It is hard to guarantee that the ealy startup code does not rely
> on relocations so this is a bit fragile. It would be more robust
> to handle RELATIVE relocs early and only IRELATIVE relocs later,
> but the current relocation processing code cannot do that.

As a side note, how hard would that be? I really think we should aim
to make the bootstrap less fragile and it would also make porting
static-pie to other architecture easier.

> 
> The early startup code up to relocation processing includes
> 
>   _dl_aux_init (auxvec);
>   __libc_init_secure ();
>   __tunables_init (__environ);
>   ARCH_INIT_CPU_FEATURES ();
>   _dl_relocate_static_pie ();
> 
> These are simple enough that RELATIVE relocs can be avoided.
> 
> The following steps include
> 
>   ARCH_SETUP_IREL ();
>   ARCH_SETUP_TLS ();
>   ARCH_APPLY_IREL ();
> 
> On some targets IRELATIVE processing relies on TLS setup on
> others TLS setup relies on IRELATIVE relocs, so the right
> position for _dl_relocate_static_pie is target dependent.
> For now move self relocation as early as possible on targets
> that support static PIE.
> 
> Fixes bug 27072.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  csu/libc-start.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index a2f6e12728..feb0d7ce11 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -146,8 +146,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    int result;
>  
>  #ifndef SHARED
> -  _dl_relocate_static_pie ();
> -
>    char **ev = &argv[argc + 1];
>  
>    __environ = ev;
> @@ -199,6 +197,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  
>    ARCH_INIT_CPU_FEATURES ();
>  
> +  /* Do static pie self relocation after tunables and cpu features
> +     are setup for ifunc resolvers. Before this point relocations
> +     must be avoided.  */
> +  _dl_relocate_static_pie ();
> +
>    /* Perform IREL{,A} relocations.  */
>    ARCH_SETUP_IREL ();
>  
> 

Ok.
Szabolcs Nagy Jan. 21, 2021, 3:38 p.m. UTC | #2
The 01/21/2021 11:07, Adhemerval Zanella wrote:
> 
> 
> On 20/01/2021 12:31, Szabolcs Nagy via Libc-alpha wrote:
> > IFUNC resolvers may depend on tunables and cpu feature setup so
> > move static pie self relocation after those.
> > 
> > It is hard to guarantee that the ealy startup code does not rely
> > on relocations so this is a bit fragile. It would be more robust
> > to handle RELATIVE relocs early and only IRELATIVE relocs later,
> > but the current relocation processing code cannot do that.
> 
> As a side note, how hard would that be? I really think we should aim
> to make the bootstrap less fragile and it would also make porting
> static-pie to other architecture easier.

the IRELATIVE relocs are in the DT_JMPREL area, while RELATIVE
relocs are outside of it. (this split is to allow simpler lazy
binding, i don't know how strictly this is enforced in elf, but
since ifunc is a gnu extension we can just say that this is abi.)

so i think you only have to change/copy elf/dynamic-link.h and
have a ELF_DYNAMIC_RELOCATE variant that processes the two sets
of relocations separately.

if you don't want to rely on DT_JMPREL then i think you have to
change elf/do-rel.h as well to have a elf_dynamic_do_Rel variant
that does IRELATIVE separately.

either case changing these looked more scary to me than the
current patches: likely we have to copy complex logic with slight
modification and there are a lot of things going on there. (but
that was before i learnt about the i386 hidden issue and hidden
weak refs and tunables_strdup.)

> > 
> > The early startup code up to relocation processing includes
> > 
> >   _dl_aux_init (auxvec);
> >   __libc_init_secure ();
> >   __tunables_init (__environ);
> >   ARCH_INIT_CPU_FEATURES ();
> >   _dl_relocate_static_pie ();
> > 
> > These are simple enough that RELATIVE relocs can be avoided.
> > 
> > The following steps include
> > 
> >   ARCH_SETUP_IREL ();
> >   ARCH_SETUP_TLS ();
> >   ARCH_APPLY_IREL ();
> > 
> > On some targets IRELATIVE processing relies on TLS setup on
> > others TLS setup relies on IRELATIVE relocs, so the right
> > position for _dl_relocate_static_pie is target dependent.
> > For now move self relocation as early as possible on targets
> > that support static PIE.
> > 
> > Fixes bug 27072.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

thanks.
diff mbox series

Patch

diff --git a/csu/libc-start.c b/csu/libc-start.c
index a2f6e12728..feb0d7ce11 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -146,8 +146,6 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   int result;
 
 #ifndef SHARED
-  _dl_relocate_static_pie ();
-
   char **ev = &argv[argc + 1];
 
   __environ = ev;
@@ -199,6 +197,11 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
 
   ARCH_INIT_CPU_FEATURES ();
 
+  /* Do static pie self relocation after tunables and cpu features
+     are setup for ifunc resolvers. Before this point relocations
+     must be avoided.  */
+  _dl_relocate_static_pie ();
+
   /* Perform IREL{,A} relocations.  */
   ARCH_SETUP_IREL ();