This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] elf: Ignore LD_AUDIT interfaces if la_version returns 0 [BZ #24122]



On 02/02/2019 13:28, Florian Weimer wrote:
> This is a repost of the early patch for glibc 2.30.
> 
> I updated some comments, replaced one “unsigned” with “unsigned int”,
> and introduced the early_audit_modules_notification function.
> 
> Thanks,
> Florian
> 
> -----
> This change moves the audit module loading and early notification into
> separate functions out of dl_main.
> 
> It restores the bug fix from commit
> 8e889c5da3c5981c5a46a93fec02de40131ac5a6  ("elf: Fix LD_AUDIT for
> modules with invalid version (BZ#24122)") which was reverted in commit
> 83e6b59625f45db1eee93e5684091f740c52a083  ("[elf] Revert 8e889c5da3
> (BZ#24122)").
> 
> The actual bug fix is the separate error message for the case when
> la_version is zero.  The dynamic linker error message (which is NULL
> in this case) is no longer used.  Based on the intended use of version
> zero (ignore this module due to explicit request), the message is only
> printed if debugging is enabled.
> 
> 2019-02-02  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24122]
> 	* elf/rtld.c (unload_audit_module): New function.
> 	(report_audit_module_load_error): Likewise.
> 	(load_audit_module): Likewise.  Extracted from dl_main.  Call
> 	_dl_close if the laversion symbol cannot be found.  Use early
> 	returns for error handling.  Check for a zero return value from
> 	la_version.  Remove spurious comment about static TLS
> 	initialization.  Remove (void) casts of function results.
> 	(load_audit_modules): New function.  Extracted from dl_main.
> 	(early_audit_modules_notification): Likewise.
> 	(dl_main): Call load_audit_modules and
> 	early_audit_modules_notification.

I think we should reinstate the testcase from original commit 
8e889c5da3c5981c5a46a93fec02de40131ac5a6 along with this change. It aligns
with our policy of fixing bug reports with a regression test.

> 
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 5d97f41b7b..3babe0185b 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -863,6 +863,205 @@ handle_ld_preload (const char *preloadlist, struct link_map *main_map)
>    return npreloads;
>  }
>  
> +/* Called if the audit DSO cannot be used: if it does not have the
> +   appropriate interfaces, or it expects a more recent version library
> +   version than what the dynamic linker provides.  */
> +static void
> +unload_audit_module (struct link_map *map, int original_tls_idx)
> +{
> +#ifndef NDEBUG
> +  Lmid_t ns = map->l_ns;
> +#endif
> +  _dl_close (map);
> +
> +  /* Make sure the namespace has been cleared entirely.  */
> +  assert (GL(dl_ns)[ns]._ns_loaded == NULL);
> +  assert (GL(dl_ns)[ns]._ns_nloaded == 0);
> +
> +  GL(dl_tls_max_dtv_idx) = original_tls_idx;
> +}

Ok.

> +
> +/* Called to print an error message if loading of an audit module
> +   failed.  */
> +static void
> +report_audit_module_load_error (const char *name, const char *err_str,
> +				bool malloced)
> +{
> +  _dl_error_printf ("\
> +ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> +		    name, err_str);
> +  if (malloced)
> +    free ((char *) err_str);
> +}

Ok.

> +
> +/* Load one audit module.  */
> +static void
> +load_audit_module (const char *name, struct audit_ifaces **last_audit)
> +{
> +  int original_tls_idx = GL(dl_tls_max_dtv_idx);
> +
> +  struct dlmopen_args dlmargs;
> +  dlmargs.fname = name;
> +  dlmargs.map = NULL;

Maybe a simple initialization as 'struct dlmopen_args dlmargs = { name, NULL }'?

> +
> +  const char *objname;
> +  const char *err_str = NULL;
> +  bool malloced;
> +  _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit, &dlmargs);
> +  if (__glibc_unlikely (err_str != NULL))
> +    {
> +      report_audit_module_load_error (name, err_str, malloced);
> +      return;
> +    }
> +
> +  struct lookup_args largs;
> +  largs.name = "la_version";
> +  largs.map = dlmargs.map;
> +  _dl_catch_error (&objname, &err_str, &malloced, lookup_doit, &largs);
> +  if (__glibc_likely (err_str != NULL))
> +    {
> +      unload_audit_module (dlmargs.map, original_tls_idx);
> +      report_audit_module_load_error (name, err_str, malloced);
> +      return;
> +    }
> +
> +  unsigned int (*laversion) (unsigned int) = largs.result;
> +  assert (laversion != NULL);

I think we need to add a comment why we assert on a NULL 'laversion'
symbol, maybe by adding this means a corrupted audit object.

> +  unsigned int lav = laversion (LAV_CURRENT);
> +  if (lav == 0)

Maybe __glibc_unlikely here?

> +    {
> +      /* Only print an error message if debugging because this can
> +	 happen deliberately.  */
> +      if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
> +	_dl_debug_printf ("\
> +file=%s [%lu]; audit interface function la_version returned zero; ignored.\n",
> +			  dlmargs.map->l_name, dlmargs.map->l_ns);
> +      unload_audit_module (dlmargs.map, original_tls_idx);
> +      return;
> +    }
> +
> +  if (lav > LAV_CURRENT)
> +    {
> +      _dl_debug_printf ("\
> +ERROR: audit interface '%s' requires version %d (maximum supported version %d); ignored.\n",
> +			name, lav, LAV_CURRENT);
> +      unload_audit_module (dlmargs.map, original_tls_idx);
> +      return;
> +    }
> +
> +  /* Allocate structure for the callback function pointers.  This call
> +     can never fail.  */
> +  enum { naudit_ifaces = 8 };
> +  union
> +  {
> +    struct audit_ifaces ifaces;
> +    void (*fptr[naudit_ifaces]) (void);
> +  } *newp = malloc (sizeof (*newp));

If this call should never fail, should we also assert for the malloc ?

> +
> +  /* Names of the auditing interfaces.  All in one
> +     long string.  */
> +  static const char audit_iface_names[] =
> +    "la_activity\0"
> +    "la_objsearch\0"
> +    "la_objopen\0"
> +    "la_preinit\0"
> +#if __ELF_NATIVE_CLASS == 32
> +    "la_symbind32\0"
> +#elif __ELF_NATIVE_CLASS == 64
> +    "la_symbind64\0"
> +#else
> +# error "__ELF_NATIVE_CLASS must be defined"
> +#endif
> +#define STRING(s) __STRING (s)
> +    "la_" STRING (ARCH_LA_PLTENTER) "\0"
> +    "la_" STRING (ARCH_LA_PLTEXIT) "\0"
> +    "la_objclose\0";
> +  unsigned int cnt = 0;
> +  const char *cp = audit_iface_names;
> +  do
> +    {
> +      largs.name = cp;
> +      (void) _dl_catch_error (&objname, &err_str, &malloced,
> +			      lookup_doit, &largs);

Do we need the cast here?

> +
> +      /* Store the pointer.  */
> +      if (err_str == NULL && largs.result != NULL)
> +	{
> +	  newp->fptr[cnt] = largs.result;
> +
> +	  /* The dynamic linker link map is statically allocated,
> +	     initialize the data now.  */
> +	  GL(dl_rtld_map).l_audit[cnt].cookie
> +	    = (intptr_t) &GL(dl_rtld_map);
> +	}
> +      else
> +	newp->fptr[cnt] = NULL;
> +      ++cnt;
> +
> +      cp = (char *) rawmemchr (cp, '\0') + 1;

Same as before.

> +    }
> +  while (*cp != '\0');
> +  assert (cnt == naudit_ifaces);
> +
> +  /* Now append the new auditing interface to the list.  */
> +  newp->ifaces.next = NULL;
> +  if (*last_audit == NULL)
> +    *last_audit = GLRO(dl_audit) = &newp->ifaces;
> +  else
> +    *last_audit = (*last_audit)->next = &newp->ifaces;
> +  ++GLRO(dl_naudit);
> +
> +  /* Mark the DSO as being used for auditing.  */
> +  dlmargs.map->l_auditing = 1;
> +}
> +
> +/* Load all audit modules.  */
> +static void
> +load_audit_modules (void)
> +{
> +  struct audit_ifaces *last_audit = NULL;
> +  struct audit_list_iter al_iter;
> +  audit_list_iter_init (&al_iter);
> +
> +  while (true)
> +    {
> +      const char *name = audit_list_iter_next (&al_iter);
> +      if (name == NULL)
> +	break;
> +      load_audit_module (name, &last_audit);
> +    }
> +}
> +
> +/* If we have any auditing modules, announce that we already have two
> +   objects loaded: the main program and the dynamic linker itself.  */
> +static void
> +early_audit_modules_notification (struct link_map *main_map)
> +{
> +  if (__glibc_likely (GLRO(dl_naudit) == 0))
> +    return;
> +
> +  struct link_map *ls[2] = { main_map, &GL(dl_rtld_map) };
> +
> +  for (unsigned int outer = 0; outer < 2; ++outer)

Maybe size_t here?

> +    {
> +      struct audit_ifaces *afct = GLRO(dl_audit);
> +      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
> +	{
> +	  if (afct->objopen != NULL)
> +	    {
> +	      ls[outer]->l_audit[cnt].bindflags
> +		= afct->objopen (ls[outer], LM_ID_BASE,
> +				 &ls[outer]->l_audit[cnt].cookie);
> +
> +	      ls[outer]->l_audit_any_plt
> +		|= ls[outer]->l_audit[cnt].bindflags != 0;
> +	    }
> +
> +	  afct = afct->next;
> +	}
> +    }
> +}
> +

Ok.

>  static void
>  dl_main (const ElfW(Phdr) *phdr,
>  	 ElfW(Word) phnum,
> @@ -1395,10 +1594,6 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]);
>    if (__glibc_unlikely (audit_list != NULL)
>        || __glibc_unlikely (audit_list_string != NULL))
>      {
> -      struct audit_ifaces *last_audit = NULL;
> -      struct audit_list_iter al_iter;
> -      audit_list_iter_init (&al_iter);
> -
>        /* Since we start using the auditing DSOs right away we need to
>  	 initialize the data structures now.  */
>        tcbp = init_tls ();
> @@ -1410,164 +1605,8 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]);
>        security_init ();
>        need_security_init = false;
>  
> -      while (true)
> -	{
> -	  const char *name = audit_list_iter_next (&al_iter);
> -	  if (name == NULL)
> -	    break;
> -
> -	  int tls_idx = GL(dl_tls_max_dtv_idx);
> -
> -	  /* Now it is time to determine the layout of the static TLS
> -	     block and allocate it for the initial thread.  Note that we
> -	     always allocate the static block, we never defer it even if
> -	     no DF_STATIC_TLS bit is set.  The reason is that we know
> -	     glibc will use the static model.  */
> -	  struct dlmopen_args dlmargs;
> -	  dlmargs.fname = name;
> -	  dlmargs.map = NULL;
> -
> -	  const char *objname;
> -	  const char *err_str = NULL;
> -	  bool malloced;
> -	  (void) _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit,
> -				  &dlmargs);
> -	  if (__glibc_unlikely (err_str != NULL))
> -	    {
> -	    not_loaded:
> -	      _dl_error_printf ("\
> -ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> -				name, err_str);
> -	      if (malloced)
> -		free ((char *) err_str);
> -	    }
> -	  else
> -	    {
> -	      struct lookup_args largs;
> -	      largs.name = "la_version";
> -	      largs.map = dlmargs.map;
> -
> -	      /* Check whether the interface version matches.  */
> -	      (void) _dl_catch_error (&objname, &err_str, &malloced,
> -				      lookup_doit, &largs);
> -
> -	      unsigned int (*laversion) (unsigned int);
> -	      unsigned int lav;
> -	      if  (err_str == NULL
> -		   && (laversion = largs.result) != NULL
> -		   && (lav = laversion (LAV_CURRENT)) > 0
> -		   && lav <= LAV_CURRENT)
> -		{
> -		  /* Allocate structure for the callback function pointers.
> -		     This call can never fail.  */
> -		  union
> -		  {
> -		    struct audit_ifaces ifaces;
> -#define naudit_ifaces 8
> -		    void (*fptr[naudit_ifaces]) (void);
> -		  } *newp = malloc (sizeof (*newp));
> -
> -		  /* Names of the auditing interfaces.  All in one
> -		     long string.  */
> -		  static const char audit_iface_names[] =
> -		    "la_activity\0"
> -		    "la_objsearch\0"
> -		    "la_objopen\0"
> -		    "la_preinit\0"
> -#if __ELF_NATIVE_CLASS == 32
> -		    "la_symbind32\0"
> -#elif __ELF_NATIVE_CLASS == 64
> -		    "la_symbind64\0"
> -#else
> -# error "__ELF_NATIVE_CLASS must be defined"
> -#endif
> -#define STRING(s) __STRING (s)
> -		    "la_" STRING (ARCH_LA_PLTENTER) "\0"
> -		    "la_" STRING (ARCH_LA_PLTEXIT) "\0"
> -		    "la_objclose\0";
> -		  unsigned int cnt = 0;
> -		  const char *cp = audit_iface_names;
> -		  do
> -		    {
> -		      largs.name = cp;
> -		      (void) _dl_catch_error (&objname, &err_str, &malloced,
> -					      lookup_doit, &largs);
> -
> -		      /* Store the pointer.  */
> -		      if (err_str == NULL && largs.result != NULL)
> -			{
> -			  newp->fptr[cnt] = largs.result;
> -
> -			  /* The dynamic linker link map is statically
> -			     allocated, initialize the data now.   */
> -			  GL(dl_rtld_map).l_audit[cnt].cookie
> -			    = (intptr_t) &GL(dl_rtld_map);
> -			}
> -		      else
> -			newp->fptr[cnt] = NULL;
> -		      ++cnt;
> -
> -		      cp = (char *) rawmemchr (cp, '\0') + 1;
> -		    }
> -		  while (*cp != '\0');
> -		  assert (cnt == naudit_ifaces);
> -
> -		  /* Now append the new auditing interface to the list.  */
> -		  newp->ifaces.next = NULL;
> -		  if (last_audit == NULL)
> -		    last_audit = GLRO(dl_audit) = &newp->ifaces;
> -		  else
> -		    last_audit = last_audit->next = &newp->ifaces;
> -		  ++GLRO(dl_naudit);
> -
> -		  /* Mark the DSO as being used for auditing.  */
> -		  dlmargs.map->l_auditing = 1;
> -		}
> -	      else
> -		{
> -		  /* We cannot use the DSO, it does not have the
> -		     appropriate interfaces or it expects something
> -		     more recent.  */
> -#ifndef NDEBUG
> -		  Lmid_t ns = dlmargs.map->l_ns;
> -#endif
> -		  _dl_close (dlmargs.map);
> -
> -		  /* Make sure the namespace has been cleared entirely.  */
> -		  assert (GL(dl_ns)[ns]._ns_loaded == NULL);
> -		  assert (GL(dl_ns)[ns]._ns_nloaded == 0);
> -
> -		  GL(dl_tls_max_dtv_idx) = tls_idx;
> -		  goto not_loaded;
> -		}
> -	    }
> -	}
> -
> -      /* If we have any auditing modules, announce that we already
> -	 have two objects loaded.  */
> -      if (__glibc_unlikely (GLRO(dl_naudit) > 0))
> -	{
> -	  struct link_map *ls[2] = { main_map, &GL(dl_rtld_map) };
> -
> -	  for (unsigned int outer = 0; outer < 2; ++outer)
> -	    {
> -	      struct audit_ifaces *afct = GLRO(dl_audit);
> -	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
> -		{
> -		  if (afct->objopen != NULL)
> -		    {
> -		      ls[outer]->l_audit[cnt].bindflags
> -			= afct->objopen (ls[outer], LM_ID_BASE,
> -					 &ls[outer]->l_audit[cnt].cookie);
> -
> -		      ls[outer]->l_audit_any_plt
> -			|= ls[outer]->l_audit[cnt].bindflags != 0;
> -		    }
> -
> -		  afct = afct->next;
> -		}
> -	    }
> -	}
> +      load_audit_modules ();
> +      early_audit_modules_notification (main_map);
>      }
>  
>    /* Keep track of the currently loaded modules to count how many
> 


Ok.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]