Re: [PATCH] elf: dl-load: Get rid of alloca usage.

[Joe Simmons-Talbott <josimmon@redhat.com>]

Ping.

On Mon, Oct 02, 2023 at 09:24:01AM -0400, Joe Simmons-Talbott wrote:
> Replace alloca usage with scratch_buffers.  Change the sematics of
> is_trusted_path_normalize to return 1, 0, or -1 on error.
> ---
>  elf/dl-load.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..c8e135b6e5 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -21,6 +21,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <libintl.h>
> +#include <scratch_buffer.h>
>  #include <stdbool.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
>  };
>  #define nsystem_dirs_len array_length (system_dirs_len)
>  
> -static bool
> +static int
>  is_trusted_path_normalize (const char *path, size_t len)
>  {
>    if (len == 0)
> -    return false;
> +    return 0;
> +
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
> +
> +  if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> +    return -1;
>  
> -  char *npath = (char *) alloca (len + 2);
> +  char *npath = sbuf.data;
>    char *wnp = npath;
> +
>    while (*path != '\0')
>      {
>        if (path[0] == '/')
> @@ -172,12 +180,12 @@ is_trusted_path_normalize (const char *path, size_t len)
>        if (wnp - npath >= system_dirs_len[idx]
>  	  && memcmp (trun, npath, system_dirs_len[idx]) == 0)
>  	/* Found it.  */
> -	return true;
> +	return 1;
>  
>        trun += system_dirs_len[idx] + 1;
>      }
>  
> -  return false;
> +  return 0;
>  }
>  
>  /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -363,7 +371,7 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>       this way because it may be manipulated in some ways with hard
>       links.  */
>    if (__glibc_unlikely (check_for_trusted)
> -      && !is_trusted_path_normalize (result, wp - result))
> +      && is_trusted_path_normalize (result, wp - result) != 1)
>      {
>        *result = '\0';
>        return result;
> @@ -951,6 +959,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>    /* Initialize to keep the compiler happy.  */
>    const char *errstring = NULL;
>    int errval = 0;
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>  
>    /* Get file information.  To match the kernel behavior, do not fill
>       in this information for the executable in case of an explicit
> @@ -982,6 +992,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	    free ((void *) l->l_phdr);
>  	  free (l);
>  	  free (realname);
> +	  scratch_buffer_free (&sbuf);
>  	  _dl_signal_error (errval, name, NULL, errstring);
>  	}
>  
> @@ -998,6 +1009,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	    free (realname);
>  	    add_name_to_object (l, name);
>  
> +	    scratch_buffer_free (&sbuf);
>  	    return l;
>  	  }
>      }
> @@ -1029,6 +1041,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>        /* Add the map for the mirrored object to the object list.  */
>        _dl_add_to_namespace_list (l, nsid);
>  
> +      scratch_buffer_free (&sbuf);
>        return l;
>      }
>  #endif
> @@ -1039,6 +1052,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	 loaded.  So return now.  */
>        free (realname);
>        __close_nocancel (fd);
> +      scratch_buffer_free (&sbuf);
>        return NULL;
>      }
>  
> @@ -1071,7 +1085,12 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      phdr = (void *) (fbp->buf + header->e_phoff);
>    else
>      {
> -      phdr = alloca (maplength);
> +      if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
> +	{
> +	  errstring = N_("cannot allocate memory");
> +	  goto lose_errno;
> +	}
> +      phdr = sbuf.data;
>        if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
>  				       header->e_phoff) != maplength)
>  	{
> @@ -1485,7 +1504,10 @@ cannot enable executable stack as shared object requires");
>  
>    /* Skip auditing and debugger notification when called from 'sprof'.  */
>    if (mode & __RTLD_SPROF)
> -    return l;
> +    {
> +      scratch_buffer_free (&sbuf);
> +      return l;
> +    }
>  
>    /* Signal that we are going to add new objects.  */
>    struct r_debug *r = _dl_debug_update (nsid);
> @@ -1515,6 +1537,7 @@ cannot enable executable stack as shared object requires");
>      _dl_audit_objopen (l, nsid);
>  #endif
>  
> +  scratch_buffer_free (&sbuf);
>    return l;
>  }
>  \f
> @@ -1598,6 +1621,8 @@ open_verify (const char *name, int fd,
>    /* Initialize it to make the compiler happy.  */
>    const char *errstring = NULL;
>    int errval = 0;
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>  
>  #ifdef SHARED
>    /* Give the auditing libraries a chance.  */
> @@ -1660,6 +1685,7 @@ open_verify (const char *name, int fd,
>  	      name = strdupa (realname);
>  	      free (realname);
>  	    }
> +	  scratch_buffer_free (&sbuf);
>  	  __close_nocancel (fd);
>  	  _dl_signal_error (errval, name, NULL, errstring);
>  	}
> @@ -1696,6 +1722,7 @@ open_verify (const char *name, int fd,
>  		 32-bit and 64-bit binaries can be run this might
>  		 happen.  */
>  	      *found_other_class = true;
> +	      scratch_buffer_free (&sbuf);
>  	      __close_nocancel (fd);
>  	      __set_errno (ENOENT);
>  	      return -1;
> @@ -1734,6 +1761,7 @@ open_verify (const char *name, int fd,
>  	}
>        if (! __glibc_likely (elf_machine_matches_host (ehdr)))
>  	{
> +	  scratch_buffer_free (&sbuf);
>  	  __close_nocancel (fd);
>  	  __set_errno (ENOENT);
>  	  return -1;
> @@ -1755,7 +1783,14 @@ open_verify (const char *name, int fd,
>  	phdr = (void *) (fbp->buf + ehdr->e_phoff);
>        else
>  	{
> -	  phdr = alloca (maplength);
> +	  if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
> +	    {
> +	      errval = errno;
> +	      errstring = N_("cannot allocate memory");
> +	      goto lose;
> +	    }
> +	  phdr = sbuf.data;
> +
>  	  if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
>  					   ehdr->e_phoff) != maplength)
>  	    {
> @@ -1769,6 +1804,7 @@ open_verify (const char *name, int fd,
>  			    (phdr, ehdr->e_phnum, fbp->buf, fbp->len,
>  			     loader, fd)))
>  	{
> +	  scratch_buffer_free (&sbuf);
>  	  __close_nocancel (fd);
>  	  __set_errno (ENOENT);
>  	  return -1;
> @@ -1776,6 +1812,7 @@ open_verify (const char *name, int fd,
>  
>      }
>  
> +  scratch_buffer_free (&sbuf);
>    return fd;
>  }
>  \f
> @@ -1796,13 +1833,18 @@ open_path (const char *name, size_t namelen, int mode,
>    int fd = -1;
>    const char *current_what = NULL;
>    int any = 0;
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>  
>    if (__glibc_unlikely (dirs == NULL))
>      /* We're called before _dl_init_paths when loading the main executable
>         given on the command line when rtld is run directly.  */
>      return -1;
>  
> -  buf = alloca (max_dirnamelen + max_capstrlen + namelen);
> +  if (!scratch_buffer_set_array_size (&sbuf, 1,
> +	max_dirnamelen + max_capstrlen + namelen))
> +    return -1;
> +  buf = sbuf.data;
>    do
>      {
>        struct r_search_path_elem *this_dir = *dirs;
> @@ -1901,6 +1943,7 @@ open_path (const char *name, size_t namelen, int mode,
>  	  if (*realname != NULL)
>  	    {
>  	      memcpy (*realname, buf, buflen);
> +	      scratch_buffer_free (&sbuf);
>  	      return fd;
>  	    }
>  	  else
> @@ -1908,12 +1951,16 @@ open_path (const char *name, size_t namelen, int mode,
>  	      /* No memory for the name, we certainly won't be able
>  		 to load and link it.  */
>  	      __close_nocancel (fd);
> +	      scratch_buffer_free (&sbuf);
>  	      return -1;
>  	    }
>  	}
>        if (here_any && (err = errno) != ENOENT && err != EACCES)
> -	/* The file exists and is readable, but something went wrong.  */
> -	return -1;
> +	{
> +	  /* The file exists and is readable, but something went wrong.  */
> +          scratch_buffer_free (&sbuf);
> +	  return -1;
> +	}
>  
>        /* Remember whether we found anything.  */
>        any |= here_any;
> @@ -1934,6 +1981,7 @@ open_path (const char *name, size_t namelen, int mode,
>  	sps->dirs = (void *) -1;
>      }
>  
> +  scratch_buffer_free (&sbuf);
>    return -1;
>  }
>  
> -- 
> 2.39.2
>