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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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>

> ---
>  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.