Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Carlos O'Donell <carlos at redhat dot com>]

On 10/8/19 8:01 PM, Carlos O'Donell wrote:
> I was doing a Fedora triage with Florian and we caught that this
> issue had slipped through the cracks. Here is the patch with a
> very simple test case.

Ping.

> 8< --- 8< --- 8<
> After d1d5471579eb0426671bf94f2d71e61dfb204c30 we always setup the
> link map l to make the static and shared cases the same.  The bug
> is that in elf/dl-load.c (_dl_init_paths) we conditionally set l
> only in the #ifdef SHARED case, but unconditionally use it later.
> The simple solution is to remove the #ifdef SHARED conditional,
> because it's no longer needed, and unconditionally setup l for
> both the static and shared cases. A regression test is added to
> run a static binary with LD_LIBRARY_PATH='$ORIGIN' which crashes
> before the fix and runs after the fix.
> 
> Regression tested on x86_64.
> ---
>  ChangeLog            |  9 ++++++
>  elf/Makefile         |  5 +++-
>  elf/dl-load.c        | 67 +++++++++++++++++++++-----------------------
>  elf/tst-dst-static.c | 30 ++++++++++++++++++++
>  4 files changed, 75 insertions(+), 36 deletions(-)
>  create mode 100644 elf/tst-dst-static.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 2ce48223f4..cf198257ba 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,12 @@
> +2019-10-08  Carlos O'Donell  <carlos@redhat.com>
> +
> +	[BZ #23462]
> +	* elf/Makefile (tests-static-normal): tst-dst-static.
> +	(tst-dst-static-ENV): Define.
> +	* elf/dl-load.c (_dl_init_paths): Remove #ifdef SHARED, and refactor
> +	to always assume l is non-NULL.
> +	* elf/tst-dst-static.c: New file.
> +
>  2019-10-07  Carlos O'Donell  <carlos@redhat.com>
>  
>  	* nptl/cancellation.c: Document that all functions here must be
> diff --git a/elf/Makefile b/elf/Makefile
> index dea51ca182..bb55baaa1f 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -148,7 +148,8 @@ endif
>  tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \
>  	       tst-dl-iter-static \
>  	       tst-tlsalign-static tst-tlsalign-extern-static \
> -	       tst-linkall-static tst-env-setuid tst-env-setuid-tunables
> +	       tst-linkall-static tst-env-setuid tst-env-setuid-tunables \
> +	       tst-dst-static
>  tests-static-internal := tst-tls1-static tst-tls2-static \
>  	       tst-ptrguard1-static tst-stackguard1-static \
>  	       tst-tls1-static-non-pie tst-libc_dlvsym-static
> @@ -1557,3 +1558,5 @@ $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
>  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>  
>  CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
> +
> +tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN'
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 438793e53d..903f8af13a 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -748,50 +748,47 @@ _dl_init_paths (const char *llp)
>    max_dirnamelen = SYSTEM_DIRS_MAX_LEN;
>    *aelem = NULL;
>  
> -#ifdef SHARED
> -  /* This points to the map of the main object.  */
> +  /* This points to the map of the main object.  It is always non-NULL
> +     since we have purposely made the dynamic and static cases look the
> +     same.  */
>    l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> -  if (l != NULL)
> +  assert (l->l_type != lt_loaded);
> +
> +  if (l->l_info[DT_RUNPATH])
> +    {
> +      /* Allocate room for the search path and fill in information
> +	 from RUNPATH.  */
> +      decompose_rpath (&l->l_runpath_dirs,
> +		       (const void *) (D_PTR (l, l_info[DT_STRTAB])
> +				       + l->l_info[DT_RUNPATH]->d_un.d_val),
> +		       l, "RUNPATH");
> +      /* During rtld init the memory is allocated by the stub malloc,
> +	 prevent any attempt to free it by the normal malloc.  */
> +      l->l_runpath_dirs.malloced = 0;
> +
> +      /* The RPATH is ignored.  */
> +      l->l_rpath_dirs.dirs = (void *) -1;
> +    }
> +  else
>      {
> -      assert (l->l_type != lt_loaded);
> +      l->l_runpath_dirs.dirs = (void *) -1;
>  
> -      if (l->l_info[DT_RUNPATH])
> +      if (l->l_info[DT_RPATH])
>  	{
>  	  /* Allocate room for the search path and fill in information
> -	     from RUNPATH.  */
> -	  decompose_rpath (&l->l_runpath_dirs,
> +	     from RPATH.  */
> +	  decompose_rpath (&l->l_rpath_dirs,
>  			   (const void *) (D_PTR (l, l_info[DT_STRTAB])
> -					   + l->l_info[DT_RUNPATH]->d_un.d_val),
> -			   l, "RUNPATH");
> -	  /* During rtld init the memory is allocated by the stub malloc,
> -	     prevent any attempt to free it by the normal malloc.  */
> -	  l->l_runpath_dirs.malloced = 0;
> -
> -	  /* The RPATH is ignored.  */
> -	  l->l_rpath_dirs.dirs = (void *) -1;
> +					   + l->l_info[DT_RPATH]->d_un.d_val),
> +			   l, "RPATH");
> +	  /* During rtld init the memory is allocated by the stub
> +	     malloc, prevent any attempt to free it by the normal
> +	     malloc.  */
> +	  l->l_rpath_dirs.malloced = 0;
>  	}
>        else
> -	{
> -	  l->l_runpath_dirs.dirs = (void *) -1;
> -
> -	  if (l->l_info[DT_RPATH])
> -	    {
> -	      /* Allocate room for the search path and fill in information
> -		 from RPATH.  */
> -	      decompose_rpath (&l->l_rpath_dirs,
> -			       (const void *) (D_PTR (l, l_info[DT_STRTAB])
> -					       + l->l_info[DT_RPATH]->d_un.d_val),
> -			       l, "RPATH");
> -	      /* During rtld init the memory is allocated by the stub
> -		 malloc, prevent any attempt to free it by the normal
> -		 malloc.  */
> -	      l->l_rpath_dirs.malloced = 0;
> -	    }
> -	  else
> -	    l->l_rpath_dirs.dirs = (void *) -1;
> -	}
> +	l->l_rpath_dirs.dirs = (void *) -1;
>      }
> -#endif	/* SHARED */
>  
>    if (llp != NULL && *llp != '\0')
>      {
> diff --git a/elf/tst-dst-static.c b/elf/tst-dst-static.c
> new file mode 100644
> index 0000000000..7092f24a8e
> --- /dev/null
> +++ b/elf/tst-dst-static.c
> @@ -0,0 +1,30 @@
> +/* Test DST expansion for static binaries doesn't carsh. Bug 23462.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* The purpose of this test is to exercise the code in elf/dl-loac.c
> +   (_dl_init_paths) or thereabout and ensure that static binaries
> +   don't crash when expanding DSTs. */
> +static int
> +do_test (void)
> +{
> +  /* If the dynamic loader code linked into the static binary cannot
> +     handle expanding the DSTs e.g. null-deref on an incomplete link
> +     map, then it will crash before reaching main.  */
> +  return 0;
> +}
> +#include <support/test-driver.c>
> 


-- 
Cheers,
Carlos.