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 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.
> + }
> }
> }
> }
> diff --git a/elf/tst-audit13.c b/elf/tst-audit13.c
> new file mode 100644
> index 0000000000..6f587baf58
> --- /dev/null
> +++ b/elf/tst-audit13.c
> @@ -0,0 +1,28 @@
> +/* Check for invalid audit version (BZ#24122).
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +
> +static int
> +do_test (void)
> +{
> + puts ("plt call");
> + return 0;
> +}
OK.
> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-audit13mod1.c b/elf/tst-audit13mod1.c
> new file mode 100644
> index 0000000000..598dab5ec4
> --- /dev/null
> +++ b/elf/tst-audit13mod1.c
> @@ -0,0 +1,93 @@
> +/* Check for invalid audit version (BZ#24122).
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <link.h>
> +#include <stdlib.h>
> +
> +unsigned int
> +la_version (unsigned int version)
> +{
> + /* The audit specification says that a version of 0 or a version
> + greater than any version supported by the dynamic loader shall
> + cause the module to be ignored. */
> + return 0;
> +}
OK.
> +
> +void
> +la_activity (uintptr_t *cookie, unsigned int flag)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +char *
> +la_objsearch (const char *name, uintptr_t *cookie, unsigned int flag)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t * cookie)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +void
> +la_preinit (uintptr_t * cookie)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +uintptr_t
> +#if __ELF_NATIVE_CLASS == 32
> +la_symbind32 (Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, unsigned int *flags, const char *symname)
> +#else
> +la_symbind64 (Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, unsigned int *flags, const char *symname)
> +#endif
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +la_objclose (uintptr_t * cookie)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +#include <tst-audit.h>
> +#if (!defined (pltenter) || !defined (pltexit) || !defined (La_regs) \
> + || !defined (La_retval) || !defined (int_retval))
> +# error "architecture specific code needed in sysdeps/CPU/tst-audit.h"
> +#endif
> +
> +ElfW(Addr)
> +pltenter (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, La_regs *regs, unsigned int *flags,
> + const char *symname, long int *framesizep)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +pltexit (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, const La_regs *inregs, La_retval *outregs,
> + const char *symname)
> +{
> + exit (EXIT_FAILURE);
> +}
>
--
Cheers,
Carlos.