diff mbox

Fix BZ #16634 -- assert in ld.so when dlopen("a.out"...) is called repeatedly.

Message ID ye6qpplo78g2.fsf@elbrus2.mtv.corp.google.com
State New
Headers show

Commit Message

Paul Pluzhnikov March 14, 2014, 9:54 p.m. UTC 
Greetings,

Attached patch fixes BZ #16634 by moving sanity check for dlopen()ing
a.out before we call _dl_next_tls_modid() for it.

Tested on Linux/x86_64; no new failures.

--

2014-03-14  Paul Pluzhnikov  <ppluzhnikov@google.com>

	BZ #16634
        * elf/dl-load.c (open_verify): Add mode parameter.
        Error early when ET_EXEC and mode does not have __RTLD_OPENEXEC.
        (open_path): Change from boolean 'secure' to complete flag 'mode'
        (_dl_map_object): Adjust.

Comments

Siddhesh Poyarekar March 16, 2014, 10:51 a.m. UTC | #1 
On Fri, Mar 14, 2014 at 02:54:05PM -0700, Paul Pluzhnikov wrote:
> Greetings,
> 
> Attached patch fixes BZ #16634 by moving sanity check for dlopen()ing
> a.out before we call _dl_next_tls_modid() for it.
> 
> Tested on Linux/x86_64; no new failures.

A more detailed description please, assuming that it's going to be
used for the commit log.

> 
> --
> 
> 2014-03-14  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	BZ #16634

In square brackets.

>         * elf/dl-load.c (open_verify): Add mode parameter.
>         Error early when ET_EXEC and mode does not have __RTLD_OPENEXEC.
>         (open_path): Change from boolean 'secure' to complete flag 'mode'
>         (_dl_map_object): Adjust.
> 
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 8ebc128..3f90b92 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1667,7 +1667,7 @@ print_search_path (struct r_search_path_elem **list,
>     user might want to know about this.  */
>  static int
>  open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
> -	     int whatcode, bool *found_other_class, bool free_name)
> +	     int whatcode, int mode, bool *found_other_class, bool free_name)
>  {
>    /* This is the expected ELF header.  */
>  #define ELF32_CLASS ELFCLASS32
> @@ -1843,6 +1843,13 @@ open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
>  	  errstring = N_("only ET_DYN and ET_EXEC can be loaded");
>  	  goto call_lose;
>  	}
> +      else if (__glibc_unlikely (ehdr->e_type == ET_EXEC
> +				 && (mode & __RTLD_OPENEXEC) == 0))
> +	{
> +	  /* For BZ #16634, return early.  */

Likewise, please explain the condition instead of just quoting the bz
number.

> +	  errstring = N_("cannot dynamically load executable");
> +	  goto call_lose;
> +	}
>        else if (__builtin_expect (ehdr->e_phentsize, sizeof (ElfW(Phdr)))
>  	       != sizeof (ElfW(Phdr)))
>  	{
> @@ -1928,7 +1935,7 @@ open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
>     if MAY_FREE_DIRS is true.  */
>  
>  static int
> -open_path (const char *name, size_t namelen, int secure,
> +open_path (const char *name, size_t namelen, int mode,
>  	   struct r_search_path_struct *sps, char **realname,
>  	   struct filebuf *fbp, struct link_map *loader, int whatcode,
>  	   bool *found_other_class)
> @@ -1980,8 +1987,8 @@ open_path (const char *name, size_t namelen, int secure,
>  	  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
>  	    _dl_debug_printf ("  trying file=%s\n", buf);
>  
> -	  fd = open_verify (buf, fbp, loader, whatcode, found_other_class,
> -			    false);
> +	  fd = open_verify (buf, fbp, loader, whatcode, mode,
> +			    found_other_class, false);
>  	  if (this_dir->status[cnt] == unknown)
>  	    {
>  	      if (fd != -1)
> @@ -2010,7 +2017,7 @@ open_path (const char *name, size_t namelen, int secure,
>  	  /* Remember whether we found any existing directory.  */
>  	  here_any |= this_dir->status[cnt] != nonexisting;
>  
> -	  if (fd != -1 && __builtin_expect (secure, 0)
> +	  if (fd != -1 && __builtin_expect (mode & __RTLD_SECURE, 0)

Use __glibc_unlikely.


Siddhesh
diff mbox

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 8ebc128..3f90b92 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1667,7 +1667,7 @@  print_search_path (struct r_search_path_elem **list,
    user might want to know about this.  */
 static int
 open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
-	     int whatcode, bool *found_other_class, bool free_name)
+	     int whatcode, int mode, bool *found_other_class, bool free_name)
 {
   /* This is the expected ELF header.  */
 #define ELF32_CLASS ELFCLASS32
@@ -1843,6 +1843,13 @@  open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
 	  errstring = N_("only ET_DYN and ET_EXEC can be loaded");
 	  goto call_lose;
 	}
+      else if (__glibc_unlikely (ehdr->e_type == ET_EXEC
+				 && (mode & __RTLD_OPENEXEC) == 0))
+	{
+	  /* For BZ #16634, return early.  */
+	  errstring = N_("cannot dynamically load executable");
+	  goto call_lose;
+	}
       else if (__builtin_expect (ehdr->e_phentsize, sizeof (ElfW(Phdr)))
 	       != sizeof (ElfW(Phdr)))
 	{
@@ -1928,7 +1935,7 @@  open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
    if MAY_FREE_DIRS is true.  */
 
 static int
-open_path (const char *name, size_t namelen, int secure,
+open_path (const char *name, size_t namelen, int mode,
 	   struct r_search_path_struct *sps, char **realname,
 	   struct filebuf *fbp, struct link_map *loader, int whatcode,
 	   bool *found_other_class)
@@ -1980,8 +1987,8 @@  open_path (const char *name, size_t namelen, int secure,
 	  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
 	    _dl_debug_printf ("  trying file=%s\n", buf);
 
-	  fd = open_verify (buf, fbp, loader, whatcode, found_other_class,
-			    false);
+	  fd = open_verify (buf, fbp, loader, whatcode, mode,
+			    found_other_class, false);
 	  if (this_dir->status[cnt] == unknown)
 	    {
 	      if (fd != -1)
@@ -2010,7 +2017,7 @@  open_path (const char *name, size_t namelen, int secure,
 	  /* Remember whether we found any existing directory.  */
 	  here_any |= this_dir->status[cnt] != nonexisting;
 
-	  if (fd != -1 && __builtin_expect (secure, 0)
+	  if (fd != -1 && __builtin_expect (mode & __RTLD_SECURE, 0)
 	      && INTUSE(__libc_enable_secure))
 	    {
 	      /* This is an extra security effort to make sure nobody can
@@ -2184,7 +2191,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 	  for (l = loader; l; l = l->l_loader)
 	    if (cache_rpath (l, &l->l_rpath_dirs, DT_RPATH, "RPATH"))
 	      {
-		fd = open_path (name, namelen, mode & __RTLD_SECURE,
+		fd = open_path (name, namelen, mode,
 				&l->l_rpath_dirs,
 				&realname, &fb, loader, LA_SER_RUNPATH,
 				&found_other_class);
@@ -2200,7 +2207,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 	      && main_map != NULL && main_map->l_type != lt_loaded
 	      && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH,
 			      "RPATH"))
-	    fd = open_path (name, namelen, mode & __RTLD_SECURE,
+	    fd = open_path (name, namelen, mode,
 			    &main_map->l_rpath_dirs,
 			    &realname, &fb, loader ?: main_map, LA_SER_RUNPATH,
 			    &found_other_class);
@@ -2208,7 +2215,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 
       /* Try the LD_LIBRARY_PATH environment variable.  */
       if (fd == -1 && env_path_list.dirs != (void *) -1)
-	fd = open_path (name, namelen, mode & __RTLD_SECURE, &env_path_list,
+	fd = open_path (name, namelen, mode, &env_path_list,
 			&realname, &fb,
 			loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded,
 			LA_SER_LIBPATH, &found_other_class);
@@ -2217,7 +2224,7 @@  _dl_map_object (struct link_map *loader, const char *name,
       if (fd == -1 && loader != NULL
 	  && cache_rpath (loader, &loader->l_runpath_dirs,
 			  DT_RUNPATH, "RUNPATH"))
-	fd = open_path (name, namelen, mode & __RTLD_SECURE,
+	fd = open_path (name, namelen, mode,
 			&loader->l_runpath_dirs, &realname, &fb, loader,
 			LA_SER_RUNPATH, &found_other_class);
 
@@ -2267,7 +2274,8 @@  _dl_map_object (struct link_map *loader, const char *name,
 		{
 		  fd = open_verify (cached,
 				    &fb, loader ?: GL(dl_ns)[nsid]._ns_loaded,
-				    LA_SER_CONFIG, &found_other_class, false);
+				    LA_SER_CONFIG, mode, &found_other_class,
+				    false);
 		  if (__glibc_likely (fd != -1))
 		    {
 		      realname = local_strdup (cached);
@@ -2287,7 +2295,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 	  && ((l = loader ?: GL(dl_ns)[nsid]._ns_loaded) == NULL
 	      || __builtin_expect (!(l->l_flags_1 & DF_1_NODEFLIB), 1))
 	  && rtld_search_dirs.dirs != (void *) -1)
-	fd = open_path (name, namelen, mode & __RTLD_SECURE, &rtld_search_dirs,
+	fd = open_path (name, namelen, mode, &rtld_search_dirs,
 			&realname, &fb, l, LA_SER_DEFAULT, &found_other_class);
 
       /* Add another newline when we are tracing the library loading.  */
@@ -2305,7 +2313,7 @@  _dl_map_object (struct link_map *loader, const char *name,
       else
 	{
 	  fd = open_verify (realname, &fb,
-			    loader ?: GL(dl_ns)[nsid]._ns_loaded, 0,
+			    loader ?: GL(dl_ns)[nsid]._ns_loaded, 0, mode,
 			    &found_other_class, true);
 	  if (__builtin_expect (fd, 0) == -1)
 	    free (realname);