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

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

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

Absolutely.  I already posted that as a separate patch.  I want to
retain your authorship, and since it is a completely separate change, a
separate commit seems reasonable to me.

>> +/* 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 }'?

It's what was there before. 8-/

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

I can add:

/* A null symbol indicates that something is very wrong with the loaded
   object because defined symbols are supposed to have a valid, non-null
   address.  */

>> +  unsigned int lav = laversion (LAV_CURRENT);
>> +  if (lav == 0)
>
> Maybe __glibc_unlikely here?

We don't really know what's unlikely or not.  Since audit modules are
used only rarely, I don't want to worry about it.


>> +  /* 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 ?

I think we use _dl_fatal_printf elsewhere.  Again, this is a
pre-existing issue. 8-/

>
>> +
>> +  /* 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?

I think we should switch to the new error handling framework
(_dl_catch_exception) and not worry about it until then.

>> +
>> +      /* 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.

Yeah, an unrelated, pre-existing issue.

>> +/* 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?

Despite outer < 2?

I can unroll the loop in the caller, like this:

  if (__glibc_likely (GLRO(dl_naudit) > 0)
    {
      early_audit_modules_notification (main_map);
      early_audit_modules_notification (&GL(dl_rtld_map));
    }

That actually looks quite reasonable.  What do you think?

Thanks,
Florian