Re: [PATCH v4 08/10] csu: Move static pie self relocation later [BZ #27072]

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 01/19/2021 11:07, Adhemerval Zanella wrote:
> On 18/01/2021 13:25, 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.
> > 
> > The early startup code before relocation processing includes
> > 
> >   _dl_aux_init (auxvec);
> >   __libc_init_secure ();
> >   __tunables_init (__environ);
> >   ARCH_INIT_CPU_FEATURES ();
> > 
> > These are simple enough that RELATIVE relocs can be avoided.
> > 
> > __ehdr_start may require RELATIVE relocation so it was moved
> > later, fortunately ehdr and phdr are not used in the early code.
> > 
> > Fixes bug 27072.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


sigh, this is an old version of this patch, i made a
mistake putting the series together.

the problem is that _dl_phdr is used in ARCH_SETUP_TLS
(to get the tls program headers) so the __ehdr_start
magic should be before that (this only matters if auxv
lacks AT_PHDR for some reason, which should not happen
normally on linux, so testing won't show the problem)

i'm trying to figure out where i lost the new version..

thanks for the reviews.

> 
> > ---
> >  csu/libc-start.c | 44 +++++++++++++++++++++++++-------------------
> >  1 file changed, 25 insertions(+), 19 deletions(-)
> > 
> > diff --git a/csu/libc-start.c b/csu/libc-start.c
> > index 1e90dcb0a7..c2b59431a3 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;
> Ok.
> 
> 
> > @@ -169,24 +167,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> >    }
> >  #  endif
> >    _dl_aux_init (auxvec);
> > -  if (GL(dl_phdr) == NULL)
> >  # endif
> > -    {
> > -      /* Starting from binutils-2.23, the linker will define the
> > -         magic symbol __ehdr_start to point to our own ELF header
> > -         if it is visible in a segment that also includes the phdrs.
> > -         So we can set up _dl_phdr and _dl_phnum even without any
> > -         information from auxv.  */
> > -
> > -      extern const ElfW(Ehdr) __ehdr_start
> > -	__attribute__ ((weak, visibility ("hidden")));
> > -      if (&__ehdr_start != NULL)
> > -        {
> > -          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
> > -          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
> > -          GL(dl_phnum) = __ehdr_start.e_phnum;
> > -        }
> > -    }
> >  
> >    /* Initialize very early so that tunables can use it.  */
> >    __libc_init_secure ();
> 
> Ok.
> 
> > @@ -195,6 +176,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.
> 
> > @@ -206,6 +192,26 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> >       hwcap and platform fields available in the TCB.  */
> >    ARCH_APPLY_IREL ();
> >  
> > +# ifdef HAVE_AUX_VECTOR
> > +  if (GL(dl_phdr) == NULL)
> > +# endif
> > +    {
> > +      /* Starting from binutils-2.23, the linker will define the
> > +         magic symbol __ehdr_start to point to our own ELF header
> > +         if it is visible in a segment that also includes the phdrs.
> > +         So we can set up _dl_phdr and _dl_phnum even without any
> > +         information from auxv.  */
> > +
> > +      extern const ElfW(Ehdr) __ehdr_start
> > +	__attribute__ ((weak, visibility ("hidden")));
> > +      if (&__ehdr_start != NULL)
> > +        {
> > +          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
> > +          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
> > +          GL(dl_phnum) = __ehdr_start.e_phnum;
> > +        }
> > +    }
> > +
> >    /* Set up the stack checker's canary.  */
> >    uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
> >  # ifdef THREAD_SET_STACK_GUARD
> > 
> 
> Ok.