* [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] @ 2021-10-15 20:07 H.J. Lu via Libc-alpha 2021-11-29 12:50 ` PING^1 " H.J. Lu via Libc-alpha 2021-11-29 15:30 ` Carlos O'Donell via Libc-alpha 0 siblings, 2 replies; 4+ messages in thread From: H.J. Lu via Libc-alpha @ 2021-10-15 20:07 UTC (permalink / raw) To: libc-alpha DT_RUNPATH is only used to find the immediate dependencies of the executable or shared object containing the DT_RUNPATH entry. The glibc internal dlopen call should try the DT_RUNPATH of the executable. This partially fixes BZ #28455. --- elf/Makefile | 9 +++++++-- elf/dl-load.c | 30 ++++++++++++++++++++++-------- elf/tst-audit14a.c | 1 + nss/Makefile | 3 +++ nss/tst-nss-test1a.c | 1 + 5 files changed, 34 insertions(+), 10 deletions(-) create mode 100644 elf/tst-audit14a.c create mode 100644 nss/tst-nss-test1a.c diff --git a/elf/Makefile b/elf/Makefile index eeef71b82a..739cd6c8ef 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -239,10 +239,10 @@ ifneq ($(selinux-enabled),1) tests-execstack-yes = tst-execstack tst-execstack-needed tst-execstack-prog endif ifeq ($(have-depaudit),yes) -tests += tst-audit14 tst-audit15 tst-audit16 +tests += tst-audit14 tst-audit15 tst-audit16 tst-audit14a ifeq ($(run-built-tests),yes) tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ - $(objpfx)tst-audit16-cmp.out + $(objpfx)tst-audit16-cmp.out $(objpfx)tst-audit14a-cmp.out endif endif endif @@ -1479,6 +1479,8 @@ tst-auditmany-ENV = \ LDFLAGS-tst-audit14 = -Wl,--audit=tst-auditlogmod-1.so $(objpfx)tst-auditlogmod-1.so: $(libsupport) $(objpfx)tst-audit14.out: $(objpfx)tst-auditlogmod-1.so +LDFLAGS-tst-audit14a = -Wl,--audit=tst-auditlogmod-1.so,--enable-new-dtags +$(objpfx)tst-audit14a.out: $(objpfx)tst-auditlogmod-1.so LDFLAGS-tst-audit15 = \ -Wl,--audit=tst-auditlogmod-1.so,--depaudit=tst-auditlogmod-2.so $(objpfx)tst-auditlogmod-2.so: $(libsupport) @@ -1505,6 +1507,9 @@ tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so $(objpfx)tst-audit14-cmp.out: tst-audit14.exp $(objpfx)tst-audit14.out cmp $^ > $@; \ $(evaluate-test) +$(objpfx)tst-audit14a-cmp.out: tst-audit14.exp $(objpfx)tst-audit14a.out + cmp $^ > $@; \ + $(evaluate-test) $(objpfx)tst-audit15-cmp.out: tst-audit15.exp $(objpfx)tst-audit15.out cmp $^ > $@; \ $(evaluate-test) diff --git a/elf/dl-load.c b/elf/dl-load.c index 18d3e8fe64..95f4d13c12 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -2162,15 +2162,29 @@ _dl_map_object (struct link_map *loader, const char *name, } /* If dynamically linked, try the DT_RPATH of the executable - itself. NB: we do this for lookups in any namespace. */ + itself. NB: we do this for lookups in any namespace. + + Also try DT_RUNPATH for glibc internal dlopen call. */ if (fd == -1 && !did_main_map - && main_map != NULL && main_map->l_type != lt_loaded - && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH, - "RPATH")) - fd = open_path (name, namelen, mode, - &main_map->l_rpath_dirs, - &realname, &fb, loader ?: main_map, LA_SER_RUNPATH, - &found_other_class); + && main_map != NULL && main_map->l_type != lt_loaded) + { + struct r_search_path_struct l_rpath_dirs; + struct r_search_path_struct *l_rpath_dirs_p = NULL; + if (cache_rpath (main_map, &main_map->l_rpath_dirs, + DT_RPATH, "RPATH")) + l_rpath_dirs_p = &main_map->l_rpath_dirs; + else if (__glibc_unlikely (mode & __RTLD_DLOPEN)) + { + l_rpath_dirs.dirs = NULL; + if (cache_rpath (main_map, &l_rpath_dirs, DT_RUNPATH, + "RUNPATH")) + l_rpath_dirs_p = &l_rpath_dirs; + } + if (l_rpath_dirs_p) + fd = open_path (name, namelen, mode, l_rpath_dirs_p, + &realname, &fb, loader ?: main_map, + LA_SER_RUNPATH, &found_other_class); + } } /* Try the LD_LIBRARY_PATH environment variable. */ diff --git a/elf/tst-audit14a.c b/elf/tst-audit14a.c new file mode 100644 index 0000000000..c6232eacf2 --- /dev/null +++ b/elf/tst-audit14a.c @@ -0,0 +1 @@ +#include "tst-audit14.c" diff --git a/nss/Makefile b/nss/Makefile index bccf9f2806..cd99e04732 100644 --- a/nss/Makefile +++ b/nss/Makefile @@ -58,6 +58,7 @@ tests-static = tst-field tests-internal = tst-field tests = test-netdb test-digits-dots tst-nss-getpwent bug17079 \ tst-nss-test1 \ + tst-nss-test1a \ tst-nss-test2 \ tst-nss-test4 \ tst-nss-test5 @@ -189,3 +190,5 @@ endif $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so $(objpfx)tst-nss-files-alias-truncated.out: $(objpfx)/libnss_files.so + +LDFLAGS-tst-nss-test1a = -Wl,--enable-new-dtags diff --git a/nss/tst-nss-test1a.c b/nss/tst-nss-test1a.c new file mode 100644 index 0000000000..f1428259c8 --- /dev/null +++ b/nss/tst-nss-test1a.c @@ -0,0 +1 @@ +#include "tst-nss-test1.c" -- 2.31.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* PING^1 [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] 2021-10-15 20:07 [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] H.J. Lu via Libc-alpha @ 2021-11-29 12:50 ` H.J. Lu via Libc-alpha 2021-11-29 15:30 ` Carlos O'Donell via Libc-alpha 1 sibling, 0 replies; 4+ messages in thread From: H.J. Lu via Libc-alpha @ 2021-11-29 12:50 UTC (permalink / raw) To: GNU C Library On Fri, Oct 15, 2021 at 1:07 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > DT_RUNPATH is only used to find the immediate dependencies of the > executable or shared object containing the DT_RUNPATH entry. The > glibc internal dlopen call should try the DT_RUNPATH of the executable. > This partially fixes BZ #28455. > --- > elf/Makefile | 9 +++++++-- > elf/dl-load.c | 30 ++++++++++++++++++++++-------- > elf/tst-audit14a.c | 1 + > nss/Makefile | 3 +++ > nss/tst-nss-test1a.c | 1 + > 5 files changed, 34 insertions(+), 10 deletions(-) > create mode 100644 elf/tst-audit14a.c > create mode 100644 nss/tst-nss-test1a.c > > diff --git a/elf/Makefile b/elf/Makefile > index eeef71b82a..739cd6c8ef 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -239,10 +239,10 @@ ifneq ($(selinux-enabled),1) > tests-execstack-yes = tst-execstack tst-execstack-needed tst-execstack-prog > endif > ifeq ($(have-depaudit),yes) > -tests += tst-audit14 tst-audit15 tst-audit16 > +tests += tst-audit14 tst-audit15 tst-audit16 tst-audit14a > ifeq ($(run-built-tests),yes) > tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ > - $(objpfx)tst-audit16-cmp.out > + $(objpfx)tst-audit16-cmp.out $(objpfx)tst-audit14a-cmp.out > endif > endif > endif > @@ -1479,6 +1479,8 @@ tst-auditmany-ENV = \ > LDFLAGS-tst-audit14 = -Wl,--audit=tst-auditlogmod-1.so > $(objpfx)tst-auditlogmod-1.so: $(libsupport) > $(objpfx)tst-audit14.out: $(objpfx)tst-auditlogmod-1.so > +LDFLAGS-tst-audit14a = -Wl,--audit=tst-auditlogmod-1.so,--enable-new-dtags > +$(objpfx)tst-audit14a.out: $(objpfx)tst-auditlogmod-1.so > LDFLAGS-tst-audit15 = \ > -Wl,--audit=tst-auditlogmod-1.so,--depaudit=tst-auditlogmod-2.so > $(objpfx)tst-auditlogmod-2.so: $(libsupport) > @@ -1505,6 +1507,9 @@ tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so > $(objpfx)tst-audit14-cmp.out: tst-audit14.exp $(objpfx)tst-audit14.out > cmp $^ > $@; \ > $(evaluate-test) > +$(objpfx)tst-audit14a-cmp.out: tst-audit14.exp $(objpfx)tst-audit14a.out > + cmp $^ > $@; \ > + $(evaluate-test) > $(objpfx)tst-audit15-cmp.out: tst-audit15.exp $(objpfx)tst-audit15.out > cmp $^ > $@; \ > $(evaluate-test) > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 18d3e8fe64..95f4d13c12 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -2162,15 +2162,29 @@ _dl_map_object (struct link_map *loader, const char *name, > } > > /* If dynamically linked, try the DT_RPATH of the executable > - itself. NB: we do this for lookups in any namespace. */ > + itself. NB: we do this for lookups in any namespace. > + > + Also try DT_RUNPATH for glibc internal dlopen call. */ > if (fd == -1 && !did_main_map > - && main_map != NULL && main_map->l_type != lt_loaded > - && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH, > - "RPATH")) > - fd = open_path (name, namelen, mode, > - &main_map->l_rpath_dirs, > - &realname, &fb, loader ?: main_map, LA_SER_RUNPATH, > - &found_other_class); > + && main_map != NULL && main_map->l_type != lt_loaded) > + { > + struct r_search_path_struct l_rpath_dirs; > + struct r_search_path_struct *l_rpath_dirs_p = NULL; > + if (cache_rpath (main_map, &main_map->l_rpath_dirs, > + DT_RPATH, "RPATH")) > + l_rpath_dirs_p = &main_map->l_rpath_dirs; > + else if (__glibc_unlikely (mode & __RTLD_DLOPEN)) > + { > + l_rpath_dirs.dirs = NULL; > + if (cache_rpath (main_map, &l_rpath_dirs, DT_RUNPATH, > + "RUNPATH")) > + l_rpath_dirs_p = &l_rpath_dirs; > + } > + if (l_rpath_dirs_p) > + fd = open_path (name, namelen, mode, l_rpath_dirs_p, > + &realname, &fb, loader ?: main_map, > + LA_SER_RUNPATH, &found_other_class); > + } > } > > /* Try the LD_LIBRARY_PATH environment variable. */ > diff --git a/elf/tst-audit14a.c b/elf/tst-audit14a.c > new file mode 100644 > index 0000000000..c6232eacf2 > --- /dev/null > +++ b/elf/tst-audit14a.c > @@ -0,0 +1 @@ > +#include "tst-audit14.c" > diff --git a/nss/Makefile b/nss/Makefile > index bccf9f2806..cd99e04732 100644 > --- a/nss/Makefile > +++ b/nss/Makefile > @@ -58,6 +58,7 @@ tests-static = tst-field > tests-internal = tst-field > tests = test-netdb test-digits-dots tst-nss-getpwent bug17079 \ > tst-nss-test1 \ > + tst-nss-test1a \ > tst-nss-test2 \ > tst-nss-test4 \ > tst-nss-test5 > @@ -189,3 +190,5 @@ endif > > $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so > $(objpfx)tst-nss-files-alias-truncated.out: $(objpfx)/libnss_files.so > + > +LDFLAGS-tst-nss-test1a = -Wl,--enable-new-dtags > diff --git a/nss/tst-nss-test1a.c b/nss/tst-nss-test1a.c > new file mode 100644 > index 0000000000..f1428259c8 > --- /dev/null > +++ b/nss/tst-nss-test1a.c > @@ -0,0 +1 @@ > +#include "tst-nss-test1.c" > -- > 2.31.1 > PING. -- H.J. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] 2021-10-15 20:07 [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] H.J. Lu via Libc-alpha 2021-11-29 12:50 ` PING^1 " H.J. Lu via Libc-alpha @ 2021-11-29 15:30 ` Carlos O'Donell via Libc-alpha 2021-12-03 3:20 ` H.J. Lu via Libc-alpha 1 sibling, 1 reply; 4+ messages in thread From: Carlos O'Donell via Libc-alpha @ 2021-11-29 15:30 UTC (permalink / raw) To: H.J. Lu, libc-alpha On 10/15/21 16:07, H.J. Lu via Libc-alpha wrote: > DT_RUNPATH is only used to find the immediate dependencies of the > executable or shared object containing the DT_RUNPATH entry. The > glibc internal dlopen call should try the DT_RUNPATH of the executable. > This partially fixes BZ #28455. This needs some changes. High level changes: (1) Split DT_RUNPATH handling into distinct section away from DT_RPATH handling. (2) Should be specific for audit modules in object being loaded. That is to say if this is an audit module then the object that caused the loading should be inspected for DT_RUNPATH. I think the current check is too broad. (3) Why does NSS fail? The modules have the correct DT_RUNPATH set. When the loader itself calls dlopen it does so with __RTLD_DLOPEN to indicate that this is an indirect dlopen of an object based on some other semantic requirement e.g. LD_AUDIT. It is clear to me that an executable with a recorded audit module should use the exectuables DT_RUNPATH to search for the audit module. How we achieve that in the code needs a little bit of restructuring. This patch places the DT_RUNPATH lookup within the DT_RPATH lookup, and that is not logically correct, it should be a distinct lookup *after* LD_LIBRARY_PATH lookup and around line 2187 with the current RUNPATH lookup. This lookup is a *unique* lookup for audit modules IMO since they are the only lookup that is __RTLD_DLOPEN that would need to also lookup via DT_RUNPATH from the executable. I think generally doing this lookup for __RTDL_DLOPEN is too broad and could cause other problems for which we don't have test coverage. > --- > elf/Makefile | 9 +++++++-- > elf/dl-load.c | 30 ++++++++++++++++++++++-------- > elf/tst-audit14a.c | 1 + > nss/Makefile | 3 +++ > nss/tst-nss-test1a.c | 1 + > 5 files changed, 34 insertions(+), 10 deletions(-) > create mode 100644 elf/tst-audit14a.c > create mode 100644 nss/tst-nss-test1a.c > > diff --git a/elf/Makefile b/elf/Makefile > index eeef71b82a..739cd6c8ef 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -239,10 +239,10 @@ ifneq ($(selinux-enabled),1) > tests-execstack-yes = tst-execstack tst-execstack-needed tst-execstack-prog > endif > ifeq ($(have-depaudit),yes) > -tests += tst-audit14 tst-audit15 tst-audit16 > +tests += tst-audit14 tst-audit15 tst-audit16 tst-audit14a OK. Add new tst-audit14a test. > ifeq ($(run-built-tests),yes) > tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ > - $(objpfx)tst-audit16-cmp.out > + $(objpfx)tst-audit16-cmp.out $(objpfx)tst-audit14a-cmp.out OK. Add comparison output for special test. > endif > endif > endif > @@ -1479,6 +1479,8 @@ tst-auditmany-ENV = \ > LDFLAGS-tst-audit14 = -Wl,--audit=tst-auditlogmod-1.so > $(objpfx)tst-auditlogmod-1.so: $(libsupport) > $(objpfx)tst-audit14.out: $(objpfx)tst-auditlogmod-1.so > +LDFLAGS-tst-audit14a = -Wl,--audit=tst-auditlogmod-1.so,--enable-new-dtags > +$(objpfx)tst-audit14a.out: $(objpfx)tst-auditlogmod-1.so OK. Adds new test with --enable-new-dtags to catch differences in DT_RPATH vs. DT_RUNPATH. > LDFLAGS-tst-audit15 = \ > -Wl,--audit=tst-auditlogmod-1.so,--depaudit=tst-auditlogmod-2.so > $(objpfx)tst-auditlogmod-2.so: $(libsupport) > @@ -1505,6 +1507,9 @@ tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so > $(objpfx)tst-audit14-cmp.out: tst-audit14.exp $(objpfx)tst-audit14.out > cmp $^ > $@; \ > $(evaluate-test) > +$(objpfx)tst-audit14a-cmp.out: tst-audit14.exp $(objpfx)tst-audit14a.out > + cmp $^ > $@; \ > + $(evaluate-test) OK. Do comparison. > $(objpfx)tst-audit15-cmp.out: tst-audit15.exp $(objpfx)tst-audit15.out > cmp $^ > $@; \ > $(evaluate-test) > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 18d3e8fe64..95f4d13c12 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -2162,15 +2162,29 @@ _dl_map_object (struct link_map *loader, const char *name, > } > > /* If dynamically linked, try the DT_RPATH of the executable > - itself. NB: we do this for lookups in any namespace. */ > + itself. NB: we do this for lookups in any namespace. > + > + Also try DT_RUNPATH for glibc internal dlopen call. */ OK. Agreed, we should be looking at DT_RPATH and DT_RUNPATH. The entire sequence here in _dl_map_object contains the logic for handling: * DT_RPATH * LD_LIBRARY_PATH * DT_RUNPATH etc. > if (fd == -1 && !did_main_map > - && main_map != NULL && main_map->l_type != lt_loaded > - && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH, > - "RPATH")) > - fd = open_path (name, namelen, mode, > - &main_map->l_rpath_dirs, > - &realname, &fb, loader ?: main_map, LA_SER_RUNPATH, > - &found_other_class); > + && main_map != NULL && main_map->l_type != lt_loaded) OK. Removed the cache_rpath() success call from the conditional since we need to take different actions depending on the success or failure of the call. This specifically is the logic for handling DT_RPATH in the object that caused the current object to be loaded. loader == parent object that needed name to be mapped name == object to be loaded The load path ends at the top-most shared object, but if we are dynamically linked we should look at DT_RPATH for the exectuable. The rules for DT_RPATH imply that all lookups should be using the DT_RPATH of the executable for lookups. > + { > + struct r_search_path_struct l_rpath_dirs; > + struct r_search_path_struct *l_rpath_dirs_p = NULL; > + if (cache_rpath (main_map, &main_map->l_rpath_dirs, > + DT_RPATH, "RPATH")) > + l_rpath_dirs_p = &main_map->l_rpath_dirs; OK. If cach_rpath() via DT_RPATH works then use that. > + else if (__glibc_unlikely (mode & __RTLD_DLOPEN)) > + { > + l_rpath_dirs.dirs = NULL; > + if (cache_rpath (main_map, &l_rpath_dirs, DT_RUNPATH, > + "RUNPATH")) > + l_rpath_dirs_p = &l_rpath_dirs; Else we need to look at DT_RUNPATH. Always using main_map as the link map. I don't think this is correct, I think we should split this out and be more specific about this lookup, restricting the scope to audit modules. If this breaks for NSS modules we need to have a discussion about this and it means NSS modules may need changing how they are built. Today the libnss_test1.so.2 has large DT_RUNPATH that I expect should get used for lookup. Why it fails isn't clearly explained in your commit message and it should be. > + } > + if (l_rpath_dirs_p) > + fd = open_path (name, namelen, mode, l_rpath_dirs_p, > + &realname, &fb, loader ?: main_map, > + LA_SER_RUNPATH, &found_other_class); If we found a path then use that for open_path. > + } > } > > /* Try the LD_LIBRARY_PATH environment variable. */ > diff --git a/elf/tst-audit14a.c b/elf/tst-audit14a.c > new file mode 100644 > index 0000000000..c6232eacf2 > --- /dev/null > +++ b/elf/tst-audit14a.c > @@ -0,0 +1 @@ > +#include "tst-audit14.c" > diff --git a/nss/Makefile b/nss/Makefile > index bccf9f2806..cd99e04732 100644 > --- a/nss/Makefile > +++ b/nss/Makefile > @@ -58,6 +58,7 @@ tests-static = tst-field > tests-internal = tst-field > tests = test-netdb test-digits-dots tst-nss-getpwent bug17079 \ > tst-nss-test1 \ > + tst-nss-test1a \ OK. New test. > tst-nss-test2 \ > tst-nss-test4 \ > tst-nss-test5 > @@ -189,3 +190,5 @@ endif > > $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so > $(objpfx)tst-nss-files-alias-truncated.out: $(objpfx)/libnss_files.so > + > +LDFLAGS-tst-nss-test1a = -Wl,--enable-new-dtags OK. Duplicate tst-nss-test1 as tst-nss-test1a with --enable-new-dtags to increase coverage. > diff --git a/nss/tst-nss-test1a.c b/nss/tst-nss-test1a.c > new file mode 100644 > index 0000000000..f1428259c8 > --- /dev/null > +++ b/nss/tst-nss-test1a.c > @@ -0,0 +1 @@ > +#include "tst-nss-test1.c" OK. > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] 2021-11-29 15:30 ` Carlos O'Donell via Libc-alpha @ 2021-12-03 3:20 ` H.J. Lu via Libc-alpha 0 siblings, 0 replies; 4+ messages in thread From: H.J. Lu via Libc-alpha @ 2021-12-03 3:20 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library On Mon, Nov 29, 2021 at 7:30 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 10/15/21 16:07, H.J. Lu via Libc-alpha wrote: > > DT_RUNPATH is only used to find the immediate dependencies of the > > executable or shared object containing the DT_RUNPATH entry. The > > glibc internal dlopen call should try the DT_RUNPATH of the executable. > > This partially fixes BZ #28455. > > This needs some changes. > > High level changes: > (1) Split DT_RUNPATH handling into distinct section away from DT_RPATH handling. > (2) Should be specific for audit modules in object being loaded. That is to say > if this is an audit module then the object that caused the loading should > be inspected for DT_RUNPATH. I think the current check is too broad. This is how audit works with LD_LIBRARY_PATH and DT_RPATH. My patch simply extends it to DT_RUNPATH as shown in the newly added audit testcase. > (3) Why does NSS fail? The modules have the correct DT_RUNPATH set. DT_RUNPATH only applies to the module, not libc.so. > When the loader itself calls dlopen it does so with __RTLD_DLOPEN to indicate > that this is an indirect dlopen of an object based on some other semantic > requirement e.g. LD_AUDIT. > > It is clear to me that an executable with a recorded audit module should use > the exectuables DT_RUNPATH to search for the audit module. How we achieve that > in the code needs a little bit of restructuring. > > This patch places the DT_RUNPATH lookup within the DT_RPATH lookup, and that > is not logically correct, it should be a distinct lookup *after* LD_LIBRARY_PATH > lookup and around line 2187 with the current RUNPATH lookup. > > This lookup is a *unique* lookup for audit modules IMO since they are the only > lookup that is __RTLD_DLOPEN that would need to also lookup via DT_RUNPATH > from the executable. > > I think generally doing this lookup for __RTDL_DLOPEN is too broad and could > cause other problems for which we don't have test coverage. Do we have 100% test coverage for __RTDL_DLOPEN usages? I can modify the testcase to show the DT_RUNPATH problem with __RTDL_DLOPEN. > > --- > > elf/Makefile | 9 +++++++-- > > elf/dl-load.c | 30 ++++++++++++++++++++++-------- > > elf/tst-audit14a.c | 1 + > > nss/Makefile | 3 +++ > > nss/tst-nss-test1a.c | 1 + > > 5 files changed, 34 insertions(+), 10 deletions(-) > > create mode 100644 elf/tst-audit14a.c > > create mode 100644 nss/tst-nss-test1a.c > > > > diff --git a/elf/Makefile b/elf/Makefile > > index eeef71b82a..739cd6c8ef 100644 > > --- a/elf/Makefile > > +++ b/elf/Makefile > > @@ -239,10 +239,10 @@ ifneq ($(selinux-enabled),1) > > tests-execstack-yes = tst-execstack tst-execstack-needed tst-execstack-prog > > endif > > ifeq ($(have-depaudit),yes) > > -tests += tst-audit14 tst-audit15 tst-audit16 > > +tests += tst-audit14 tst-audit15 tst-audit16 tst-audit14a > > OK. Add new tst-audit14a test. > > > ifeq ($(run-built-tests),yes) > > tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ > > - $(objpfx)tst-audit16-cmp.out > > + $(objpfx)tst-audit16-cmp.out $(objpfx)tst-audit14a-cmp.out > > OK. Add comparison output for special test. > > > endif > > endif > > endif > > @@ -1479,6 +1479,8 @@ tst-auditmany-ENV = \ > > LDFLAGS-tst-audit14 = -Wl,--audit=tst-auditlogmod-1.so > > $(objpfx)tst-auditlogmod-1.so: $(libsupport) > > $(objpfx)tst-audit14.out: $(objpfx)tst-auditlogmod-1.so > > +LDFLAGS-tst-audit14a = -Wl,--audit=tst-auditlogmod-1.so,--enable-new-dtags > > +$(objpfx)tst-audit14a.out: $(objpfx)tst-auditlogmod-1.so > > OK. Adds new test with --enable-new-dtags to catch differences in DT_RPATH vs. DT_RUNPATH. > > > LDFLAGS-tst-audit15 = \ > > -Wl,--audit=tst-auditlogmod-1.so,--depaudit=tst-auditlogmod-2.so > > $(objpfx)tst-auditlogmod-2.so: $(libsupport) > > @@ -1505,6 +1507,9 @@ tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so > > $(objpfx)tst-audit14-cmp.out: tst-audit14.exp $(objpfx)tst-audit14.out > > cmp $^ > $@; \ > > $(evaluate-test) > > +$(objpfx)tst-audit14a-cmp.out: tst-audit14.exp $(objpfx)tst-audit14a.out > > + cmp $^ > $@; \ > > + $(evaluate-test) > > OK. Do comparison. > > > $(objpfx)tst-audit15-cmp.out: tst-audit15.exp $(objpfx)tst-audit15.out > > cmp $^ > $@; \ > > $(evaluate-test) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > > index 18d3e8fe64..95f4d13c12 100644 > > --- a/elf/dl-load.c > > +++ b/elf/dl-load.c > > @@ -2162,15 +2162,29 @@ _dl_map_object (struct link_map *loader, const char *name, > > } > > > > /* If dynamically linked, try the DT_RPATH of the executable > > - itself. NB: we do this for lookups in any namespace. */ > > + itself. NB: we do this for lookups in any namespace. > > + > > + Also try DT_RUNPATH for glibc internal dlopen call. */ > > OK. Agreed, we should be looking at DT_RPATH and DT_RUNPATH. > > The entire sequence here in _dl_map_object contains the logic for handling: > * DT_RPATH > * LD_LIBRARY_PATH > * DT_RUNPATH > etc. > > > if (fd == -1 && !did_main_map > > - && main_map != NULL && main_map->l_type != lt_loaded > > - && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH, > > - "RPATH")) > > - fd = open_path (name, namelen, mode, > > - &main_map->l_rpath_dirs, > > - &realname, &fb, loader ?: main_map, LA_SER_RUNPATH, > > - &found_other_class); > > + && main_map != NULL && main_map->l_type != lt_loaded) > > OK. Removed the cache_rpath() success call from the conditional since we need to take > different actions depending on the success or failure of the call. > > This specifically is the logic for handling DT_RPATH in the object that caused the > current object to be loaded. > > loader == parent object that needed name to be mapped > name == object to be loaded > > The load path ends at the top-most shared object, but if we are dynamically > linked we should look at DT_RPATH for the exectuable. The rules for DT_RPATH > imply that all lookups should be using the DT_RPATH of the executable for lookups. > > > + { > > + struct r_search_path_struct l_rpath_dirs; > > + struct r_search_path_struct *l_rpath_dirs_p = NULL; > > + if (cache_rpath (main_map, &main_map->l_rpath_dirs, > > + DT_RPATH, "RPATH")) > > + l_rpath_dirs_p = &main_map->l_rpath_dirs; > > OK. If cach_rpath() via DT_RPATH works then use that. > > > + else if (__glibc_unlikely (mode & __RTLD_DLOPEN)) > > + { > > + l_rpath_dirs.dirs = NULL; > > + if (cache_rpath (main_map, &l_rpath_dirs, DT_RUNPATH, > > + "RUNPATH")) > > + l_rpath_dirs_p = &l_rpath_dirs; > > Else we need to look at DT_RUNPATH. Always using main_map as the link map. > > I don't think this is correct, I think we should split this out and be more > specific about this lookup, restricting the scope to audit modules. It won't work with NSS. > If this breaks for NSS modules we need to have a discussion about this and > it means NSS modules may need changing how they are built. > > Today the libnss_test1.so.2 has large DT_RUNPATH that I expect should get > used for lookup. Why it fails isn't clearly explained in your commit message > and it should be. NSS in libc.so tries to dlopen libnss_test1.so.2 whose DT_RUNPATH has no impact on dlopen in libc.so. Since we can't add DT_RUNPATH to libc.so, my solution handles audit and NSS properly. I can do diff --git a/elf/dl-load.c b/elf/dl-load.c index bf8957e73c..b51f18dc8b 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -2175,6 +2175,21 @@ _dl_map_object (struct link_map *loader, const char *name, &main_map->l_rpath_dirs, &realname, &fb, loader ?: main_map, LA_SER_RUNPATH, &found_other_class); + + /* Also try DT_RUNPATH in the executable for glibc internal + dlopen call. */ + if (__glibc_unlikely (mode & __RTLD_DLOPEN) + && fd == -1 && !did_main_map + && main_map != NULL && main_map->l_type != lt_loaded) + { + struct r_search_path_struct l_rpath_dirs; + l_rpath_dirs.dirs = NULL; + if (cache_rpath (main_map, &l_rpath_dirs, + DT_RUNPATH, "RUNPATH")) + fd = open_path (name, namelen, mode, &l_rpath_dirs, + &realname, &fb, loader ?: main_map, + LA_SER_RUNPATH, &found_other_class); + } } /* Try the LD_LIBRARY_PATH environment variable. */ > > + } > > + if (l_rpath_dirs_p) > > + fd = open_path (name, namelen, mode, l_rpath_dirs_p, > > + &realname, &fb, loader ?: main_map, > > + LA_SER_RUNPATH, &found_other_class); > > If we found a path then use that for open_path. > > > + } > > } > > > > /* Try the LD_LIBRARY_PATH environment variable. */ > > diff --git a/elf/tst-audit14a.c b/elf/tst-audit14a.c > > new file mode 100644 > > index 0000000000..c6232eacf2 > > --- /dev/null > > +++ b/elf/tst-audit14a.c > > @@ -0,0 +1 @@ > > +#include "tst-audit14.c" > > diff --git a/nss/Makefile b/nss/Makefile > > index bccf9f2806..cd99e04732 100644 > > --- a/nss/Makefile > > +++ b/nss/Makefile > > @@ -58,6 +58,7 @@ tests-static = tst-field > > tests-internal = tst-field > > tests = test-netdb test-digits-dots tst-nss-getpwent bug17079 \ > > tst-nss-test1 \ > > + tst-nss-test1a \ > > OK. New test. > > > tst-nss-test2 \ > > tst-nss-test4 \ > > tst-nss-test5 > > @@ -189,3 +190,5 @@ endif > > > > $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so > > $(objpfx)tst-nss-files-alias-truncated.out: $(objpfx)/libnss_files.so > > + > > +LDFLAGS-tst-nss-test1a = -Wl,--enable-new-dtags > > OK. Duplicate tst-nss-test1 as tst-nss-test1a with --enable-new-dtags to increase coverage. > > > diff --git a/nss/tst-nss-test1a.c b/nss/tst-nss-test1a.c > > new file mode 100644 > > index 0000000000..f1428259c8 > > --- /dev/null > > +++ b/nss/tst-nss-test1a.c > > @@ -0,0 +1 @@ > > +#include "tst-nss-test1.c" > > OK. > > > > > > -- > Cheers, > Carlos. > -- H.J. ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-03 3:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-15 20:07 [PATCH] elf: Also try DT_RUNPATH for internal dlopen [BZ #28455] H.J. Lu via Libc-alpha 2021-11-29 12:50 ` PING^1 " H.J. Lu via Libc-alpha 2021-11-29 15:30 ` Carlos O'Donell via Libc-alpha 2021-12-03 3:20 ` H.J. Lu via Libc-alpha
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).