This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] elf: Fix LD_AUDIT for modules with invalid version (BZ#24122)
On 24/01/2019 12:16, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 24/01/2019 11:20, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 24/01/2019 10:33, Florian Weimer wrote:
>>>>> * Siddhesh Poyarekar:
>>>>>
>>>>>> On 24/01/19 2:14 AM, Carlos O'Donell wrote:
>>>>>>>> Siddhesh,
>>>>>>>>
>>>>>>>> It is acceptable for 2.29?
>>>>>>>
>>>>>>> It's OK with me. Siddhesh gets to make the call.
>>>>>>> This is only a bug fix.
>>>>>>>
>>>>>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>>>>>>
>>>>>> OK
>>>>>
>>>>> This broke the build with at least GCC 8 and the GCC 9 pre-release on
>>>>> x86-64 and ppc64le:
>>>>>
>>>>> rtld.c: In function ‘dl_main’:
>>>>> rtld.c:1556:8: error: ‘lav’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>> _dl_debug_printf (
>>>>> ^~~~~~~~~~~~~~~~~~
>>>>> " auditor disabled since expected version %d is greater than "
>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> "supported version %d.\n",
>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> lav, LAV_CURRENT);
>>>>> ~~~~~~~~~~~~~~~~~
>>>>>
>>>>> Looks like the control flow is too complicated. I think it's a false
>>>>> positive.
>>>>>
>>>>> How should we fix this? Inhibit the warning (making the sources even
>>>>> more convoluted than they are)? Revert the patch? Factor out the audit
>>>>> module loading into a separate function and simplify the control flow?
>>>>
>>>> It does seem a false positive, since lav will always be set if laversion
>>>> is not NULL and the code explicitly checks for it. Let me try if
>>>> refactoring this can help gcc handle it correctly.
>>>
>>> I'm testing this patch. It changes the error messages slightly.
>>>
>>> It also calls _dl_close if we cannot find the laversion symbol,
>>> something the previous code did not do.
>>
>> The current code does calls _dl_close in this case, it does not if
>> _dl_catch_error returns an err_str (in this case the object is not
>> loaded).
>
> I meant this part:
>
> /* 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)
> goto not_loaded;
>
> The not_loaded label is in the true branch of the surrounding if, and
> the unloading on error only happens at the end of the else branch.
Right, the object is already loaded, the lookup that failed.
>
>>> +/* Called the audit DSO cannot be used, if it does not have the
>>> + appropriate interfaces or it expects something more recent. */
>>> +static void
>>> +unload_audit_modile (struct link_map *map, int original_tls_idx)
>>
>> s/modile/module
>
> Fixed.
>
>>> + (void) _dl_catch_error (&objname, &err_str, &malloced,
>>> + lookup_doit, &largs);
>>
>> Do we need this (void) cast?
>
> No, I can remove it, it came from the original code. I removed it.
>
>>
>>> + if (__glibc_likely (err_str != NULL))
>>> + {
>>> + unload_audit_modile (dlmargs.map, original_tls_idx);
>>> + report_audit_module_load_error (name, err_str, malloced);
>>> + return;
>>> + }
>>> +
>>> + unsigned int (*laversion) (unsigned int) = largs.result;
>>> + if (laversion == NULL)
>>> + {
>>> + _dl_error_printf ("\
>>> +ERROR: ld.so: audit interface '%s' has NULL laversion symbol; ignored.\n",
>>> + name);
>>
>> Maybe "ERROR: ld.so: audit interface does not contain a laversion symbol; ignored.\n" ?
>
> No, the missing symbol case is covered earlier, where lookup_doit will
> fail. This is really about a NULL symbol, a different error case.
> Maybe this is so bizarre that we should drop this check? (You need an
> SHN_ABS symbol or an IFUNC resolver that returns NULL to get this.)
I tend to agree that we should just assert as Carlos suggested.
>
>>> + unload_audit_modile (dlmargs.map, original_tls_idx);
>>> + return;
>>> + }
>>> +
>>> + unsigned lav = laversion (LAV_CURRENT);
>>> + if (lav == 0)
>>> + {
>>> + /* Only print an error message if debugging because this can
>>> + happen deliberately. */
>>> + if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
>>> + _dl_debug_printf ("\
>>> +audit interface '%s' requested to be ignored (returned version of 0).\n",
>>> + name);
>>
>> I used the '\nfile=%s...' to follow the messages already printed for DL_DEBUG_FILES.
>> Maybe 'file=%s requested to be ignored (returned version of 0).\n".
>
> This doesn't say that it's ignored as an audit module.
Maybe as original patch "\nfile=%s cannot be loaded as audit interface; ignored.\n", name);?
>
>>> + unload_audit_modile (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);
>>
>> My understanding is Carlos asked to this scenario not be reported as
>> an error.
>
> I think Carlos meant the lav == 0 case, where I agree that warning
> should be suppressed by default. The lav > LAV_CURRENT suggests a
> broken audit module because it did not properly check the required
> version against LAV_CURRENT. (If it does not want ld.so to log an
> error, the audit module can always return 0 instead.)
I don't have a strong opinion about, this rationale seems fine.
>
> New patch below.
>
> Thanks,
> Florian
>
> elf: Refactor audit module loading out of dl_main
>
> This avoids a compiler warning about an uninitialized lav variable
> (seen with GCC 8 and 9 on x86-64).
>
> 2019-01-24 Florian Weimer <fweimer@redhat.com>
>
> * 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. Remove spurious comment about static
> TLS initialization. Remove (void) casts of function results.
> (load_audit_modules): New function. Extracted from dl_main.
> (dl_main): Call load_audit_modules.
>
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 9e0f752482..f88dba8910 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -863,6 +863,182 @@ handle_ld_preload (const char *preloadlist, struct link_map *main_map)
> return npreloads;
> }
>
> +/* Called the audit DSO cannot be used, if it does not have the
> + appropriate interfaces or it expects something more recent. */
> +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;
> +}
> +
> +/* 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);
> +}
> +
> +/* 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;
> +
> + 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;
> + if (laversion == NULL)
> + {
> + _dl_error_printf ("\
> +ERROR: ld.so: audit interface '%s' has NULL laversion symbol; ignored.\n",
> + name);
> + unload_audit_module (dlmargs.map, original_tls_idx);
> + return;
> + }
> +
> + unsigned lav = laversion (LAV_CURRENT);
> + if (lav == 0)
> + {
> + /* Only print an error message if debugging because this can
> + happen deliberately. */
> + if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
> + _dl_debug_printf ("\
> +audit interface '%s' requested to be ignored (returned version of 0).\n",
> + name);
> + 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));
> +
> + /* 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;
> +}
> +
> +/* 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);
> + }
> +}
> +
> static void
> dl_main (const ElfW(Phdr) *phdr,
> ElfW(Word) phnum,
> @@ -1395,10 +1571,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,158 +1582,7 @@ 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)
> - goto not_loaded;
> -
> - if ((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;
> - if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
> - {
> - _dl_debug_printf ("\
> -\nfile=%s cannot be loaded as audit interface; ignored.\n", name);
> - if (laversion == NULL)
> - _dl_debug_printf (
> -" la_version function not found.\n");
> - else
> - {
> - if (lav == 0)
> - _dl_debug_printf (
> -" auditor requested to be ignored (returned version of 0).\n");
> - else
> - _dl_debug_printf (
> -" auditor disabled since expected version %d is greater than "
> -"supported version %d.\n",
> - lav, LAV_CURRENT);
> - }
> - }
> - }
> - }
> - }
> + load_audit_modules ();
>
> /* If we have any auditing modules, announce that we already
> have two objects loaded. */
>