| Message ID | 20211216020605.792222-1-hjl.tools@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Adhemerval Zanella Netto |
| Headers | show |
| Series | [v4] elf: Also try DT_RUNPATH for LD_AUDIT dlopen [BZ #28455] | expand |
| Context | Check | Description |
|---|---|---|
| dj/TryBot-apply_patch | success | Patch applied to master at the time it was sent |
| dj/TryBot-32bit | success | Build for i686 |
On 15/12/2021 23:06, H.J. Lu via Libc-alpha wrote: > Changes in v4: > > 1. Move the RUNPATH search for LD_AUDIT dlopen after the other RUNPATH > search > 2. Split one test per line and sort. > > DT_RUNPATH is only used to find the immediate dependencies of the > executable or shared object containing the DT_RUNPATH entry. Update > LD_AUDIT dlopen call to try the DT_RUNPATH entry of the executable. > This partially fixes BZ #28455. LGTM, I have only a question below. > --- > elf/Makefile | 20 +++++++++++++++++--- > elf/dl-load.c | 37 ++++++++++++++++++++++++++++++++----- > elf/tst-audit14a.c | 1 + > 3 files changed, 50 insertions(+), 8 deletions(-) > create mode 100644 elf/tst-audit14a.c > > diff --git a/elf/Makefile b/elf/Makefile > index fe42caeb0e..625b1a023f 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -249,10 +249,19 @@ 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-audit14a \ > + tst-audit15 \ > + tst-audit16 \ > + > ifeq ($(run-built-tests),yes) > -tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ > - $(objpfx)tst-audit16-cmp.out > +tests-special += \ > + $(objpfx)tst-audit14-cmp.out \ > + $(objpfx)tst-audit14a-cmp.out \ > + $(objpfx)tst-audit15-cmp.out \ > + $(objpfx)tst-audit16-cmp.out \ > + > endif > endif > endif Ok. > @@ -1529,6 +1538,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) > @@ -1555,6 +1566,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) Ok. > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 721593135e..1c90801903 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -2143,14 +2143,16 @@ _dl_map_object (struct link_map *loader, const char *name, > > fd = -1; > > + struct link_map *main_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded; > + bool did_main_map; > + > /* When the object has the RUNPATH information we don't use any > RPATHs. */ > if (loader == NULL || loader->l_info[DT_RUNPATH] == NULL) > { > /* This is the executable's map (if there is one). Make sure that > we do not look at it twice. */ > - struct link_map *main_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded; > - bool did_main_map = false; > + did_main_map = false; > > /* First try the DT_RPATH of the dependent object that caused NAME > to be loaded. Then that object's dependent, and on up. */ > @@ -2186,13 +2188,38 @@ _dl_map_object (struct link_map *loader, const char *name, > loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded, > LA_SER_LIBPATH, &found_other_class); > > + /* Make sure that we do not look at RUNPATH in the executable > + twice. */ > + did_main_map = false; > + > /* Look at the RUNPATH information for this binary. */ > if (fd == -1 && loader != NULL > && cache_rpath (loader, &loader->l_runpath_dirs, > DT_RUNPATH, "RUNPATH")) > - fd = open_path (name, namelen, mode, > - &loader->l_runpath_dirs, &realname, &fb, loader, > - LA_SER_RUNPATH, &found_other_class); > + { > + did_main_map = loader == main_map; > + fd = open_path (name, namelen, mode, > + &loader->l_runpath_dirs, &realname, &fb, loader, > + LA_SER_RUNPATH, &found_other_class); > + } > + > + /* When processing the lookup we may need to additionally try > + DT_RUNPATH in the executable for a glibc internal dlopen call > + when looking for audit modules. */ > + if (__glibc_unlikely (mode & __RTLD_AUDIT) > + && fd == -1 > + && !did_main_map > + && main_map != NULL When main_map would be NULL? For namespaces different than LM_ID_BASE it does make sense, but I think it is assumed in a lot of places that for LM_ID_BASE that _ns_loaded is always non-NULL (even for static linking dl-support guarantees it). > + && 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); > + } > > if (fd == -1) > { > 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" Add a comment that it checks the -Wl,--enable-new-dtags. Should we also enforce --disable-new-dtags for tst-audit14 ?
On Mon, Jan 3, 2022 at 12:41 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 15/12/2021 23:06, H.J. Lu via Libc-alpha wrote: > > Changes in v4: > > > > 1. Move the RUNPATH search for LD_AUDIT dlopen after the other RUNPATH > > search > > 2. Split one test per line and sort. > > > > DT_RUNPATH is only used to find the immediate dependencies of the > > executable or shared object containing the DT_RUNPATH entry. Update > > LD_AUDIT dlopen call to try the DT_RUNPATH entry of the executable. > > This partially fixes BZ #28455. > > LGTM, I have only a question below. > > > --- > > elf/Makefile | 20 +++++++++++++++++--- > > elf/dl-load.c | 37 ++++++++++++++++++++++++++++++++----- > > elf/tst-audit14a.c | 1 + > > 3 files changed, 50 insertions(+), 8 deletions(-) > > create mode 100644 elf/tst-audit14a.c > > > > diff --git a/elf/Makefile b/elf/Makefile > > index fe42caeb0e..625b1a023f 100644 > > --- a/elf/Makefile > > +++ b/elf/Makefile > > @@ -249,10 +249,19 @@ 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-audit14a \ > > + tst-audit15 \ > > + tst-audit16 \ > > + > > ifeq ($(run-built-tests),yes) > > -tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ > > - $(objpfx)tst-audit16-cmp.out > > +tests-special += \ > > + $(objpfx)tst-audit14-cmp.out \ > > + $(objpfx)tst-audit14a-cmp.out \ > > + $(objpfx)tst-audit15-cmp.out \ > > + $(objpfx)tst-audit16-cmp.out \ > > + > > endif > > endif > > endif > > Ok. > > > @@ -1529,6 +1538,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) > > @@ -1555,6 +1566,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) > > Ok. > > > diff --git a/elf/dl-load.c b/elf/dl-load.c > > index 721593135e..1c90801903 100644 > > --- a/elf/dl-load.c > > +++ b/elf/dl-load.c > > @@ -2143,14 +2143,16 @@ _dl_map_object (struct link_map *loader, const char *name, > > > > fd = -1; > > > > + struct link_map *main_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded; > > + bool did_main_map; > > + > > /* When the object has the RUNPATH information we don't use any > > RPATHs. */ > > if (loader == NULL || loader->l_info[DT_RUNPATH] == NULL) > > { > > /* This is the executable's map (if there is one). Make sure that > > we do not look at it twice. */ > > - struct link_map *main_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded; > > - bool did_main_map = false; > > + did_main_map = false; > > > > /* First try the DT_RPATH of the dependent object that caused NAME > > to be loaded. Then that object's dependent, and on up. */ > > @@ -2186,13 +2188,38 @@ _dl_map_object (struct link_map *loader, const char *name, > > loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded, > > LA_SER_LIBPATH, &found_other_class); > > > > + /* Make sure that we do not look at RUNPATH in the executable > > + twice. */ > > + did_main_map = false; > > + > > /* Look at the RUNPATH information for this binary. */ > > if (fd == -1 && loader != NULL > > && cache_rpath (loader, &loader->l_runpath_dirs, > > DT_RUNPATH, "RUNPATH")) > > - fd = open_path (name, namelen, mode, > > - &loader->l_runpath_dirs, &realname, &fb, loader, > > - LA_SER_RUNPATH, &found_other_class); > > + { > > + did_main_map = loader == main_map; > > + fd = open_path (name, namelen, mode, > > + &loader->l_runpath_dirs, &realname, &fb, loader, > > + LA_SER_RUNPATH, &found_other_class); > > + } > > + > > + /* When processing the lookup we may need to additionally try > > + DT_RUNPATH in the executable for a glibc internal dlopen call > > + when looking for audit modules. */ > > + if (__glibc_unlikely (mode & __RTLD_AUDIT) > > + && fd == -1 > > + && !did_main_map > > + && main_map != NULL > > When main_map would be NULL? For namespaces different than LM_ID_BASE it > does make sense, but I think it is assumed in a lot of places that > for LM_ID_BASE that _ns_loaded is always non-NULL (even for static > linking dl-support guarantees it). I simplified it in the v5 patch. But I kept the main_map check since it is also checked for DT_RPATH in the executable. > > + && 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); > > + } > > > > if (fd == -1) > > { > > 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" > > Add a comment that it checks the -Wl,--enable-new-dtags. Should we also > enforce --disable-new-dtags for tst-audit14 ? Fixed in the v5: https://patchwork.sourceware.org/project/glibc/patch/20220103235753.2062518-2-hjl.tools@gmail.com/ Thanks.
diff --git a/elf/Makefile b/elf/Makefile index fe42caeb0e..625b1a023f 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -249,10 +249,19 @@ 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-audit14a \ + tst-audit15 \ + tst-audit16 \ + ifeq ($(run-built-tests),yes) -tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ - $(objpfx)tst-audit16-cmp.out +tests-special += \ + $(objpfx)tst-audit14-cmp.out \ + $(objpfx)tst-audit14a-cmp.out \ + $(objpfx)tst-audit15-cmp.out \ + $(objpfx)tst-audit16-cmp.out \ + endif endif endif @@ -1529,6 +1538,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) @@ -1555,6 +1566,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 721593135e..1c90801903 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -2143,14 +2143,16 @@ _dl_map_object (struct link_map *loader, const char *name, fd = -1; + struct link_map *main_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded; + bool did_main_map; + /* When the object has the RUNPATH information we don't use any RPATHs. */ if (loader == NULL || loader->l_info[DT_RUNPATH] == NULL) { /* This is the executable's map (if there is one). Make sure that we do not look at it twice. */ - struct link_map *main_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded; - bool did_main_map = false; + did_main_map = false; /* First try the DT_RPATH of the dependent object that caused NAME to be loaded. Then that object's dependent, and on up. */ @@ -2186,13 +2188,38 @@ _dl_map_object (struct link_map *loader, const char *name, loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded, LA_SER_LIBPATH, &found_other_class); + /* Make sure that we do not look at RUNPATH in the executable + twice. */ + did_main_map = false; + /* Look at the RUNPATH information for this binary. */ if (fd == -1 && loader != NULL && cache_rpath (loader, &loader->l_runpath_dirs, DT_RUNPATH, "RUNPATH")) - fd = open_path (name, namelen, mode, - &loader->l_runpath_dirs, &realname, &fb, loader, - LA_SER_RUNPATH, &found_other_class); + { + did_main_map = loader == main_map; + fd = open_path (name, namelen, mode, + &loader->l_runpath_dirs, &realname, &fb, loader, + LA_SER_RUNPATH, &found_other_class); + } + + /* When processing the lookup we may need to additionally try + DT_RUNPATH in the executable for a glibc internal dlopen call + when looking for audit modules. */ + if (__glibc_unlikely (mode & __RTLD_AUDIT) + && 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); + } if (fd == -1) { 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"