| Message ID | 20190123140740.27433-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8e889c5da3c5981c5a46a93fec02de40131ac5a6 |
| Headers | show |
| Series | elf: Fix LD_AUDIT for modules with invalid version (BZ#24122) | expand |
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. - Optional: use -Wl,z,lazy for the test, or remove the comment about the PLT call. - Suggested comment in tst-audit13mod1.c > The error handling patch for invalid audit modules version access > invalid memory: > > elf/rtld.c: > > 1454 unsigned int (*laversion) (unsigned int); > 1455 unsigned int lav; > 1456 if (err_str == NULL > 1457 && (laversion = largs.result) != NULL > 1458 && (lav = laversion (LAV_CURRENT)) > 0 > 1459 && lav <= LAV_CURRENT) > 1460 { > [...] > 1526 else > 1527 { > 1528 /* We cannot use the DSO, it does not have the > 1529 appropriate interfaces or it expects something > 1530 more recent. */ > 1531 #ifndef NDEBUG > 1532 Lmid_t ns = dlmargs.map->l_ns; > 1533 #endif > 1534 _dl_close (dlmargs.map); > 1535 > 1536 /* Make sure the namespace has been cleared entirely. */ > 1537 assert (GL(dl_ns)[ns]._ns_loaded == NULL); > 1538 assert (GL(dl_ns)[ns]._ns_nloaded == 0); > 1539 > 1540 GL(dl_tls_max_dtv_idx) = tls_idx; > 1541 goto not_loaded; > 1542 } > > 1431 const char *err_str = NULL; > 1432 bool malloced; > 1433 (void) _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit, > 1434 &dlmargs); > 1435 if (__glibc_unlikely (err_str != NULL)) > 1436 { > 1437 not_loaded: > 1438 _dl_error_printf ("\ > 1439 ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", > 1440 name, err_str); > 1441 if (malloced) > 1442 free ((char *) err_str); > 1443 } > > On failure the err_str will be NULL and _dl_debug_vdprintf does not handle > it properly: > > elf/dl-misc.c: > 200 case 's': > 201 /* Get the string argument. */ > 202 iov[niov].iov_base = va_arg (arg, char *); > 203 iov[niov].iov_len = strlen (iov[niov].iov_base); > 204 if (prec != -1) > 205 iov[niov].iov_len = MIN ((size_t) prec, iov[niov].iov_len); > 206 ++niov; > 207 break; > > This patch fixes the issues and improves the error message. > > Checked on x86_64-linux-gnu and i686-linux-gnu > > [BZ #24122] > * elf/Makefile (tests): Add tst-audit13. > (modules-names): Add tst-audit13mod1. > (tst-audit13.out, 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. > --- > ChangeLog | 10 +++++ > elf/Makefile | 8 +++- > elf/rtld.c | 19 ++++++--- > elf/tst-audit13.c | 28 +++++++++++++ > elf/tst-audit13mod1.c | 91 +++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 149 insertions(+), 7 deletions(-) > create mode 100644 elf/tst-audit13.c > create mode 100644 elf/tst-audit13mod1.c > > diff --git a/elf/Makefile b/elf/Makefile > index 9cf5cd8dfd..f71ed7cfff 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 > # 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 > # 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,9 @@ 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 > +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..ad62c58e17 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. This means we had a dlopen failure of the module, which is something we *should* raise an error about. > + > + if ((laversion = largs.result) != NULL > + && (lav = laversion (LAV_CURRENT)) > 0 > + && lav <= LAV_CURRENT) OK, this means the DSO is loaded, and we run la_version symbol and check for the validity of the results. If lav == 0 or lav > LAV_CURRENT then we ignore the module. > { > /* Allocate structure for the callback function pointers. > This call can never fail. */ > @@ -1538,7 +1540,14 @@ 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; OK, this is the "ignore the module case." > + _dl_error_printf ("\ > +ERROR: ld.so: object '%s' cannot be loaded as audit interface: ", name); > + if (laversion == NULL) > + _dl_error_printf ("la_version function not found.\n"); > + else > + _dl_error_printf ( > +"invalid version '%u' (expected minimum of '%u'); ignored.\n", > + lav, LAV_CURRENT); If this message is output unconditionally, then I think this is wrong. We must only output a message in the event of LD_DEBUG=all. It is not "wrong" to return 0 or larger than the audit version, it is simply part of the API, and is not an error that should generate a message the user would see. The module is silently dropped. I think this is important for modules which dynamically disable themselves. We don't want those module to start printing errors at load time because of them. Consider a module that only audits when the sysadmin sets a system parameter, and otherwise disables itself by returning 0. This should not be an error if LD_AUDIT remains set (as it should be to keep the module checking for the admin parameter). During LD_DEBUG=all you definitely want to see these audit issues as a developer of the application, but not as a user running the application. > } > } > } > 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. Note that there might not be a PLT call here if the distro was defaulting to -Wl,-z,now. I don't think it matters in this case though. If you really want that PLT call you must use cflags -Wl,-z,lazy when you build the test. > +} > + > +#include <support/test-driver.c> > diff --git a/elf/tst-audit13mod1.c b/elf/tst-audit13mod1.c > new file mode 100644 > index 0000000000..96f1adef7a > --- /dev/null > +++ b/elf/tst-audit13mod1.c > @@ -0,0 +1,91 @@ > +/* 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) > +{ > + /* Invalid version, object should be discarded by the loader. */ Suggest: /* 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. */ Basically "return 0;" is the way to dynamically disable the auditor. > + return 0; > +} > + > +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); > +} OK. Perfect. I like the 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); > +} > OK. -- Cheers, Carlos.
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. --- [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 # 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 # 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 + # 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; + + 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. */ @@ -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) + { + _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); + } } } } 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; +} + +#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; +} + +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); +} -- 2.17.1
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.
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); } }
On 23/01/2019 17:00, Adhemerval Zanella wrote: > On 23/01/2019 16:02, Carlos O'Donell wrote: >> 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); > } > } > Siddhesh, It is acceptable for 2.29?
On 1/23/19 2:59 PM, Adhemerval Zanella wrote: > > > On 23/01/2019 17:00, Adhemerval Zanella wrote: >> On 23/01/2019 16:02, Carlos O'Donell wrote: >>> 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: Awesome, these are much clearer debug messages. >> --- >> 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); >> } >> } >> > > 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> -- Cheers, Carlos.
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 Siddhesh
* 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? Thanks, Florian
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.
* 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. If this is the direction we want to go in, I will write a ChangeLog entry. Thanks, Florian diff --git a/elf/rtld.c b/elf/rtld.c index 9e0f752482..20cea19f6f 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -863,6 +863,184 @@ 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_modile (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; + (void) _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; + (void) _dl_catch_error (&objname, &err_str, &malloced, + lookup_doit, &largs); + 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); + 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); + 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); + unload_audit_modile (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 +1573,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 +1584,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. */
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). > > If this is the direction we want to go in, I will write a ChangeLog > entry. This is similar to what I am working it, we can go with this version. > > Thanks, > Florian > > diff --git a/elf/rtld.c b/elf/rtld.c > index 9e0f752482..20cea19f6f 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -863,6 +863,184 @@ 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_modile (struct link_map *map, int original_tls_idx) s/modile/module > +{ > +#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; > + (void) _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; > + (void) _dl_catch_error (&objname, &err_str, &malloced, > + lookup_doit, &largs); Do we need this (void) cast? > + 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" ? > + 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". > + 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. > + unload_audit_modile (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 +1573,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 +1584,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. */ >
* 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. >> +/* 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.) >> + 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. >> + 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.) 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. */
On 1/24/19 9:16 AM, 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. > >>> +/* 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.) > >>> + 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. > >>> + 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.) > > 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. This work looks great. However, this has gotten too big. Please revert the fix, and put this back in when 2.30 opens. I have been convinced of the following: * No error or warning when lav == 0. * A warning when lav > LAV_CURRENT, because such modules can see LAV_CURRENT as input and should disable themselves by returning 0, rather than returning a really high lav. This allows users to catch this mistake (Florian convinced me of this on IRC). Lastly, we should not look for a la_version == NULL, which really means you had a (null) symbol (hard to create), but should instead just assert(la_version != NULL) in this case I think. -- Cheers, Carlos.
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. */ >
On 24/01/2019 12:32, Carlos O'Donell wrote: > On 1/24/19 9:16 AM, 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. >> >>>> +/* 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.) >> >>>> + 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. >> >>>> + 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.) >> >> 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. > > This work looks great. > > However, this has gotten too big. > > Please revert the fix, and put this back in when 2.30 opens. I would like to avoid the revert, I think based on new requirements regarding laversion handling that we can use simpler patch and work towards the refactor on 2.30: --- diff --git a/elf/rtld.c b/elf/rtld.c index 9e0f752..305b443 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -1434,7 +1434,6 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]); &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); @@ -1446,18 +1445,20 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", struct lookup_args largs; largs.name = "la_version"; largs.map = dlmargs.map; + Lmid_t ns; /* Check whether the interface version matches. */ (void) _dl_catch_error (&objname, &err_str, &malloced, lookup_doit, &largs); + if (err_str != NULL) + goto fail_lookup; unsigned int (*laversion) (unsigned int); - unsigned int lav; - if (err_str != NULL) - goto not_loaded; + laversion = largs.result; + assert (laversion != NULL); - if ((laversion = largs.result) != NULL - && (lav = laversion (LAV_CURRENT)) > 0 + unsigned int lav = laversion (LAV_CURRENT); + if ((lav = laversion (LAV_CURRENT)) > 0 && lav <= LAV_CURRENT) { /* Allocate structure for the callback function pointers. @@ -1530,8 +1531,17 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", /* We cannot use the DSO, it does not have the appropriate interfaces or it expects something more recent. */ + if (lav > LAV_CURRENT) + _dl_error_printf ("\ +ERROR: audit interface '%s' requires version %d (maximum supported version %d); ignored.\n", + name, lav, LAV_CURRENT); + else if (GLRO(dl_debug_mask) & DL_DEBUG_FILES) + _dl_debug_printf ("\ +\nfile=%s cannot be loaded as audit interface; ignored.\n", name); + + fail_lookup: #ifndef NDEBUG - Lmid_t ns = dlmargs.map->l_ns; + ns = dlmargs.map->l_ns; #endif _dl_close (dlmargs.map); @@ -1540,25 +1550,6 @@ 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; - 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); - } - } } } } --- what do you think? > > I have been convinced of the following: > > * No error or warning when lav == 0. > > * A warning when lav > LAV_CURRENT, because such modules can see > LAV_CURRENT as input and should disable themselves by returning > 0, rather than returning a really high lav. This allows users > to catch this mistake (Florian convinced me of this on IRC). > > Lastly, we should not look for a la_version == NULL, which really > means you had a (null) symbol (hard to create), but should instead > just assert(la_version != NULL) in this case I think.
On 1/24/19 10:16 AM, Adhemerval Zanella wrote:
> what do you think?
We're not a rush for this.
I suggest:
* Revert the existing changes which break the build with gcc9.
* Post a clean v3 for Florian and I to review.
Siddhesh needs to make the call for freezing everything.
The Red Hat team is basically waiting for the full freeze to
kick off final testing and results.
--
Cheers,
Carlos.
On 25/01/19 6:37 AM, Carlos O'Donell wrote: > On 1/24/19 10:16 AM, Adhemerval Zanella wrote: >> what do you think? > > We're not a rush for this. > > I suggest: > > * Revert the existing changes which break the build with gcc9. > > * Post a clean v3 for Florian and I to review. > > Siddhesh needs to make the call for freezing everything. > > The Red Hat team is basically waiting for the full freeze to > kick off final testing and results. Agreed, I would like to do the freeze at least by Sunday night so that I have the last four days to run tests and cut the release branch. You can always backport the fix to the 2.29 branch when it's ready. Siddhesh
On 24/01/2019 23:38, Siddhesh Poyarekar wrote: > On 25/01/19 6:37 AM, Carlos O'Donell wrote: >> On 1/24/19 10:16 AM, Adhemerval Zanella wrote: >>> what do you think? >> >> We're not a rush for this. >> >> I suggest: >> >> * Revert the existing changes which break the build with gcc9. >> >> * Post a clean v3 for Florian and I to review. >> >> Siddhesh needs to make the call for freezing everything. >> >> The Red Hat team is basically waiting for the full freeze to >> kick off final testing and results. > > Agreed, I would like to do the freeze at least by Sunday night so that I > have the last four days to run tests and cut the release branch. You > can always backport the fix to the 2.29 branch when it's ready. > > Siddhesh > Patch reverted.
* Adhemerval Zanella: >> 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. Okay, I put in an assert, but maybe I shoul even drop that. >> 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);? The new patch uses this: 6066: file=elf/tst-audit13mod1.so [1]; audit interface function la_version returned zero; ignored. It uses the l_name and l_ns, like must LD_DEBUG=files output. Thanks, Florian elf: Ignore LD_AUDIT interfaces if la_version returns 0 [BZ #24122] This commit 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-01-24 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. (dl_main): Call load_audit_modules. diff --git a/elf/rtld.c b/elf/rtld.c index 5d97f41b7b..574d1497b0 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -863,6 +863,174 @@ 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; + assert (laversion != NULL); + 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 ("\ +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)); + + /* 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 +1563,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,138 +1574,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 - && (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; - } - } - } + load_audit_modules (); /* If we have any auditing modules, announce that we already have two objects loaded. */
* Florian Weimer: > * Adhemerval Zanella: > >>> 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. > > Okay, I put in an assert, but maybe I shoul even drop that. > >>> 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);? > > The new patch uses this: > > 6066: file=elf/tst-audit13mod1.so [1]; audit interface function la_version returned zero; ignored. > > It uses the l_name and l_ns, like must LD_DEBUG=files output. And I suggest to reinstantiate the test like this. Thanks, Florian From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Subject: elf: Test for LD_AUDIT module returning zero from la_version [BZ #24122] This includes the original test case from commit 8e889c5da3c5981c5a46a93fec02de40131ac5a6 ("elf: Fix LD_AUDIT for modules with invalid version (BZ#24122)). 2019-01-24 Adhemerval Zanella <adhemerval.zanella@linaro.org> [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/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 # 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 # 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 + # 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/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; +} + +#include <support/test-driver.c> diff --git a/elf/tst-audit13mod1.c b/elf/tst-audit13mod1.c new file mode 100644 index 0000000000..cf017e235c --- /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; +} + +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); +}
diff --git a/elf/Makefile b/elf/Makefile index 9cf5cd8dfd..f71ed7cfff 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 # 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 # 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,9 @@ 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 +tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so + # 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..ad62c58e17 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; + + 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. */ @@ -1538,7 +1540,14 @@ 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; + _dl_error_printf ("\ +ERROR: ld.so: object '%s' cannot be loaded as audit interface: ", name); + if (laversion == NULL) + _dl_error_printf ("la_version function not found.\n"); + else + _dl_error_printf ( +"invalid version '%u' (expected minimum of '%u'); ignored.\n", + lav, LAV_CURRENT); } } } 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; +} + +#include <support/test-driver.c> diff --git a/elf/tst-audit13mod1.c b/elf/tst-audit13mod1.c new file mode 100644 index 0000000000..96f1adef7a --- /dev/null +++ b/elf/tst-audit13mod1.c @@ -0,0 +1,91 @@ +/* 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) +{ + /* Invalid version, object should be discarded by the loader. */ + return 0; +} + +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); +}
The error handling patch for invalid audit modules version access invalid memory: elf/rtld.c: 1454 unsigned int (*laversion) (unsigned int); 1455 unsigned int lav; 1456 if (err_str == NULL 1457 && (laversion = largs.result) != NULL 1458 && (lav = laversion (LAV_CURRENT)) > 0 1459 && lav <= LAV_CURRENT) 1460 { [...] 1526 else 1527 { 1528 /* We cannot use the DSO, it does not have the 1529 appropriate interfaces or it expects something 1530 more recent. */ 1531 #ifndef NDEBUG 1532 Lmid_t ns = dlmargs.map->l_ns; 1533 #endif 1534 _dl_close (dlmargs.map); 1535 1536 /* Make sure the namespace has been cleared entirely. */ 1537 assert (GL(dl_ns)[ns]._ns_loaded == NULL); 1538 assert (GL(dl_ns)[ns]._ns_nloaded == 0); 1539 1540 GL(dl_tls_max_dtv_idx) = tls_idx; 1541 goto not_loaded; 1542 } 1431 const char *err_str = NULL; 1432 bool malloced; 1433 (void) _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit, 1434 &dlmargs); 1435 if (__glibc_unlikely (err_str != NULL)) 1436 { 1437 not_loaded: 1438 _dl_error_printf ("\ 1439 ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", 1440 name, err_str); 1441 if (malloced) 1442 free ((char *) err_str); 1443 } On failure the err_str will be NULL and _dl_debug_vdprintf does not handle it properly: elf/dl-misc.c: 200 case 's': 201 /* Get the string argument. */ 202 iov[niov].iov_base = va_arg (arg, char *); 203 iov[niov].iov_len = strlen (iov[niov].iov_base); 204 if (prec != -1) 205 iov[niov].iov_len = MIN ((size_t) prec, iov[niov].iov_len); 206 ++niov; 207 break; This patch fixes the issues and improves the error message. Checked on x86_64-linux-gnu and i686-linux-gnu [BZ #24122] * elf/Makefile (tests): Add tst-audit13. (modules-names): Add tst-audit13mod1. (tst-audit13.out, 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. --- ChangeLog | 10 +++++ elf/Makefile | 8 +++- elf/rtld.c | 19 ++++++--- elf/tst-audit13.c | 28 +++++++++++++ elf/tst-audit13mod1.c | 91 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 7 deletions(-) create mode 100644 elf/tst-audit13.c create mode 100644 elf/tst-audit13mod1.c -- 2.17.1