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 23/01/2019 16:02, Carlos O'Donell wrote:
> On 1/23/19 12:44 PM, Adhemerval Zanella wrote:
>>
>>
>> On 23/01/2019 12:58, Carlos O'Donell wrote:
>>> On 1/23/19 9:07 AM, Adhemerval Zanella wrote:
>>>
>>> Thanks for fixing this!
>>>
>>> I'd like to see v2 please
>>>
>>> - Only output message when LD_DEBUG=all, since la_version returning 0
>>> or > LAV_CURRENT is not an error, but simply an API mandated situation
>>> where we ignore the module. I don't see that as an error.
>>>
>>> The Solaris docs say:
>>> "If the audit library return is zero, or a version that is greater
>>> than the rtld-audit interface the runtime linker supports, the audit
>>> library is discarded."
>>>
>>> Just discarded. It is up to the auditor to decide how it exposes to
>>> the user that it is running and operating as expected. We can provide
>>> feedback via LD_DEBUG=all.
>>
>> Right, this rationale make sense. However I think this information is
>> relates to the 'files' option, instead of print when 'all' option are
>> selected.
>>
>>>
>>> - Optional: use -Wl,z,lazy for the test, or remove the comment about
>>> the PLT call.
>>
>> The idea is indeed to force PLT call, so the PLT audit stub is called
>> and tests explicit fails.
>>
>>>
>>> - Suggested comment in tst-audit13mod1.c
>>
>> Ack. Patch updated below.
>>
>
> OK for master if you fix the error messages as noted below.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
>> ---
>>
>> [BZ #24122]
>> * elf/Makefile (tests): Add tst-audit13.
>> (modules-names): Add tst-audit13mod1.
>> (tst-audit13.out, LDFLAGS-tst-audit13mod1.so, tst-audit13-ENV): New
>> rule.
>> * elf/rtld.c (dl_main): Handle invalid audit module version.
>> * elf/tst-audit13.c: New file.
>> * elf/tst-audit13mod1.c: Likewise.
>>
>> --
>>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 9cf5cd8dfd..c24d765730 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -187,7 +187,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>> tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>> tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>> tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
>> - tst-unwind-ctor tst-unwind-main
>> + tst-unwind-ctor tst-unwind-main tst-audit13
>
> OK.
>
>> # reldep9
>> tests-internal += loadtest unload unload2 circleload1 \
>> neededtest neededtest2 neededtest3 neededtest4 \
>> @@ -275,7 +275,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>> tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
>> tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
>> tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
>> - tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib
>> + tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
>> + tst-audit13mod1
>
> OK.
>
>> # Most modules build with _ISOMAC defined, but those filtered out
>> # depend on internal headers.
>> modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
>> @@ -1382,6 +1383,10 @@ tst-audit12-ENV = LD_AUDIT=$(objpfx)tst-auditmod12.so
>> $(objpfx)tst-audit12mod1.so: $(objpfx)tst-audit12mod2.so
>> LDFLAGS-tst-audit12mod2.so = -Wl,--version-script=tst-audit12mod2.map
>>
>> +$(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
>> +LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
>> +tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
>
> OK.
>
>> +
>> # Override -z defs, so that we can reference an undefined symbol.
>> # Force lazy binding for the same reason.
>> LDFLAGS-tst-latepthreadmod.so = \
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 5d97f41b7b..5abb345b1e 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -1453,10 +1453,12 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>>
>> unsigned int (*laversion) (unsigned int);
>> unsigned int lav;
>> - if (err_str == NULL
>> - && (laversion = largs.result) != NULL
>> - && (lav = laversion (LAV_CURRENT)) > 0
>> - && lav <= LAV_CURRENT)
>> + if (err_str != NULL)
>> + goto not_loaded;
>
> OK, goto not_loaded if we had a real dlopen failure.
>
>> +
>> + if ((laversion = largs.result) != NULL
>> + && (lav = laversion (LAV_CURRENT)) > 0
>> + && lav <= LAV_CURRENT)
>
> OK.
>
>> {
>> /* Allocate structure for the callback function pointers.
>> This call can never fail. */
>> @@ -1538,7 +1540,18 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>> assert (GL(dl_ns)[ns]._ns_nloaded == 0);
>>
>> GL(dl_tls_max_dtv_idx) = tls_idx;
>> - goto not_loaded;
>> + if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
>
> OK, I think DL_DEBUG_FILES is an acceptable place to put the information.
> Thanks for fixing this part.
>
>> + {
>> + _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
>> + _dl_debug_printf (
>> +" invalid version '%u' (expected minimum of '%u').\n",
>> + lav, LAV_CURRENT);
>
> It is not "invalid" and I think printing that will lead to confusion.
>
> Likewise "expected minimum" ignores that 0 is low-enough but should also be ignored.
>
> I think this needs a cleanup, and I should have been clearer:
>
> For lav == 0 we should print "Auditor requested to be ignored (returned version of 0)."
>
> For lav > LAV_CURRENT "Auditor disabled since expected version %d is greater than supported version %d."
>
> All the information a developer needs is now in those messages.
>
> We should be clear about why it's disabled.
Right, I changed to:
---
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);
}
}