[v5,04/22] elf: Suppress audit calls when a (new) namespace is empty (BZ #28062)
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
From: Vivek Das Mohapatra <vivek@collabora.com>
For a new Lmid_t the namespace link_map list are empty, so it requires
to check if before using it. This can happen for when audit module
is used along with dlmopen.
Checked on x86_64-linux-gnu.
Co-authored-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
elf/Makefile | 6 ++
elf/dl-load.c | 7 ++-
elf/tst-audit20.c | 129 +++++++++++++++++++++++++++++++++++++++++++
elf/tst-audit20mod.c | 26 +++++++++
elf/tst-auditmod20.c | 73 ++++++++++++++++++++++++
5 files changed, 239 insertions(+), 2 deletions(-)
create mode 100644 elf/tst-audit20.c
create mode 100644 elf/tst-audit20mod.c
create mode 100644 elf/tst-auditmod20.c
Comments
* Adhemerval Zanella:
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 9f4fa9617d..72298776f6 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1067,8 +1067,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> && __glibc_unlikely (GLRO(dl_naudit) > 0))
> {
> struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
> - /* Do not call the functions for any auditing object. */
> - if (head->l_auditing == 0)
> + /* Do not call the functions for any auditing object and also do not
> + try to call auditing functions if the namespace is currently
> + empty. This happens when opening the first DSO in a new
> + namespace. */
> + if (head != NULL && head->l_auditing == 0)
> {
> struct audit_ifaces *afct = GLRO(dl_audit);
> for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
As far as I can tell, using GL(dl_ns)[nsid]._ns_loaded for la_activity
is a completely arbitrary choice. I think we should use
&GL(dl_ns)[nsid] for secondary namespace instead, and keep
GL(dl_ns)[LM_ID_BASE]._ns_loaded for backwards compatibility.
This will allow us to generate an LA_ACT_ADD event for an empty
namespace.
Thanks,
Florian
On 10/11/2021 11:15, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 9f4fa9617d..72298776f6 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1067,8 +1067,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>> && __glibc_unlikely (GLRO(dl_naudit) > 0))
>> {
>> struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
>> - /* Do not call the functions for any auditing object. */
>> - if (head->l_auditing == 0)
>> + /* Do not call the functions for any auditing object and also do not
>> + try to call auditing functions if the namespace is currently
>> + empty. This happens when opening the first DSO in a new
>> + namespace. */
>> + if (head != NULL && head->l_auditing == 0)
>> {
>> struct audit_ifaces *afct = GLRO(dl_audit);
>> for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>
> As far as I can tell, using GL(dl_ns)[nsid]._ns_loaded for la_activity
> is a completely arbitrary choice. I think we should use
> &GL(dl_ns)[nsid] for secondary namespace instead, and keep
> GL(dl_ns)[LM_ID_BASE]._ns_loaded for backwards compatibility.
>
> This will allow us to generate an LA_ACT_ADD event for an empty
> namespace.
I am not really following you here, '&GL(dl_ns)[nsid]' is just the container
here, we need to iterate over the 'link_maps' within it.
* Adhemerval Zanella:
> On 10/11/2021 11:15, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>>> index 9f4fa9617d..72298776f6 100644
>>> --- a/elf/dl-load.c
>>> +++ b/elf/dl-load.c
>>> @@ -1067,8 +1067,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>> && __glibc_unlikely (GLRO(dl_naudit) > 0))
>>> {
>>> struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
>>> - /* Do not call the functions for any auditing object. */
>>> - if (head->l_auditing == 0)
>>> + /* Do not call the functions for any auditing object and also do not
>>> + try to call auditing functions if the namespace is currently
>>> + empty. This happens when opening the first DSO in a new
>>> + namespace. */
>>> + if (head != NULL && head->l_auditing == 0)
>>> {
>>> struct audit_ifaces *afct = GLRO(dl_audit);
>>> for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>>
>> As far as I can tell, using GL(dl_ns)[nsid]._ns_loaded for la_activity
>> is a completely arbitrary choice. I think we should use
>> &GL(dl_ns)[nsid] for secondary namespace instead, and keep
>> GL(dl_ns)[LM_ID_BASE]._ns_loaded for backwards compatibility.
>>
>> This will allow us to generate an LA_ACT_ADD event for an empty
>> namespace.
>
> I am not really following you here, '&GL(dl_ns)[nsid]' is just the container
> here, we need to iterate over the 'link_maps' within it.
Hmm. I had a peeked at the Solaris documentation, and it says that
LA_ACT_ADD uses the head link map of the namespace as a cookie.
I really dislike that we produce a LA_ACT_DELETE without the
corresponding LA_ACT_ADD due to this issue.
Can we use the link map allocated used _dl_new_object as the cookie if
the namespace is empty? This seems like the right thing to do here.
The allocation happens just a few lines further down.
Thanks,
Florian
On 11/11/2021 09:02, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 10/11/2021 11:15, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>>>> index 9f4fa9617d..72298776f6 100644
>>>> --- a/elf/dl-load.c
>>>> +++ b/elf/dl-load.c
>>>> @@ -1067,8 +1067,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>>> && __glibc_unlikely (GLRO(dl_naudit) > 0))
>>>> {
>>>> struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
>>>> - /* Do not call the functions for any auditing object. */
>>>> - if (head->l_auditing == 0)
>>>> + /* Do not call the functions for any auditing object and also do not
>>>> + try to call auditing functions if the namespace is currently
>>>> + empty. This happens when opening the first DSO in a new
>>>> + namespace. */
>>>> + if (head != NULL && head->l_auditing == 0)
>>>> {
>>>> struct audit_ifaces *afct = GLRO(dl_audit);
>>>> for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>>>
>>> As far as I can tell, using GL(dl_ns)[nsid]._ns_loaded for la_activity
>>> is a completely arbitrary choice. I think we should use
>>> &GL(dl_ns)[nsid] for secondary namespace instead, and keep
>>> GL(dl_ns)[LM_ID_BASE]._ns_loaded for backwards compatibility.
>>>
>>> This will allow us to generate an LA_ACT_ADD event for an empty
>>> namespace.
>>
>> I am not really following you here, '&GL(dl_ns)[nsid]' is just the container
>> here, we need to iterate over the 'link_maps' within it.
>
> Hmm. I had a peeked at the Solaris documentation, and it says that
> LA_ACT_ADD uses the head link map of the namespace as a cookie.
>
> I really dislike that we produce a LA_ACT_DELETE without the
> corresponding LA_ACT_ADD due to this issue.
>
> Can we use the link map allocated used _dl_new_object as the cookie if
> the namespace is empty? This seems like the right thing to do here.
> The allocation happens just a few lines further down.
But afaiu the LA_ACT_ADD activity is to inform already loaded objects
that a new object is being processed. Both man-pages and Solaris
documentation states 'objects are *being added*...', so I think passing
the cookie of the new allocated is not what the interface is suppose
to do.
* Adhemerval Zanella:
>> Hmm. I had a peeked at the Solaris documentation, and it says that
>> LA_ACT_ADD uses the head link map of the namespace as a cookie.
>>
>> I really dislike that we produce a LA_ACT_DELETE without the
>> corresponding LA_ACT_ADD due to this issue.
>>
>> Can we use the link map allocated used _dl_new_object as the cookie if
>> the namespace is empty? This seems like the right thing to do here.
>> The allocation happens just a few lines further down.
>
> But afaiu the LA_ACT_ADD activity is to inform already loaded objects
> that a new object is being processed. Both man-pages and Solaris
> documentation states 'objects are *being added*...', so I think passing
> the cookie of the new allocated is not what the interface is suppose
> to do.
It's the only way to get a matching LA_ACT_DELETE call with the same
cookie. It's not particularly clean, I agree, but I think it's better
than a dropped LA_ACT_ADD.
Thanks,
Florian
On 11/11/2021 09:33, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>>> Hmm. I had a peeked at the Solaris documentation, and it says that
>>> LA_ACT_ADD uses the head link map of the namespace as a cookie.
>>>
>>> I really dislike that we produce a LA_ACT_DELETE without the
>>> corresponding LA_ACT_ADD due to this issue.
>>>
>>> Can we use the link map allocated used _dl_new_object as the cookie if
>>> the namespace is empty? This seems like the right thing to do here.
>>> The allocation happens just a few lines further down.
>>
>> But afaiu the LA_ACT_ADD activity is to inform already loaded objects
>> that a new object is being processed. Both man-pages and Solaris
>> documentation states 'objects are *being added*...', so I think passing
>> the cookie of the new allocated is not what the interface is suppose
>> to do.
>
> It's the only way to get a matching LA_ACT_DELETE call with the same
> cookie. It's not particularly clean, I agree, but I think it's better
> than a dropped LA_ACT_ADD.
I think we can move the la_activity (LA_ACT_ADD) *after*
_dl_add_to_namespace_list() on _dl_map_object_from_fd(), so the
GL(dl_ns)[nsid]._ns_loaded is always valid and it will call both
LA_ACT_ADD and LA_ACT_DELETE will a consistent set of cookies.
@@ -227,6 +227,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
tst-filterobj tst-filterobj-dlopen tst-auxobj tst-auxobj-dlopen \
tst-audit18a tst-audit18b \
tst-audit19 \
+ tst-audit20 \
tst-single_threaded tst-single_threaded-pthread \
tst-tls-ie tst-tls-ie-dlmopen argv0test \
tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
@@ -317,6 +318,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
tst-auditmod9a tst-auditmod9b \
tst-auditmod18a tst-auditmod18b tst-audit18bmod \
tst-auditmod19 \
+ tst-auditmod20 tst-audit20mod \
$(if $(CXX),tst-unique3lib tst-unique3lib2 tst-unique4lib \
tst-nodelete-uniquemod tst-nodelete-rtldmod \
tst-nodelete-zmod \
@@ -1565,6 +1567,10 @@ tst-audit18b-ARGS = -- $(host-test-program-cmd)
$(objpfx)tst-audit19.out: $(objpfx)tst-auditmod19.so
tst-audit19-ENV = LD_AUDIT=$(objpfx)tst-auditmod19.so
+$(objpfx)tst-audit20.out: $(objpfx)tst-auditmod20.so \
+ $(objpfx)tst-audit20mod.so
+tst-audit20-ARGS = -- $(host-test-program-cmd)
+
# tst-sonamemove links against an older implementation of the library.
LDFLAGS-tst-sonamemove-linkmod1.so = \
-Wl,--version-script=tst-sonamemove-linkmod1.map \
@@ -1067,8 +1067,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
&& __glibc_unlikely (GLRO(dl_naudit) > 0))
{
struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
- /* Do not call the functions for any auditing object. */
- if (head->l_auditing == 0)
+ /* Do not call the functions for any auditing object and also do not
+ try to call auditing functions if the namespace is currently
+ empty. This happens when opening the first DSO in a new
+ namespace. */
+ if (head != NULL && head->l_auditing == 0)
{
struct audit_ifaces *afct = GLRO(dl_audit);
for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
new file mode 100644
@@ -0,0 +1,129 @@
+/* Check DT_AUDIT with dlmopen.
+ Copyright (C) 2021 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
+ <https://www.gnu.org/licenses/>. */
+
+#include <array_length.h>
+#include <getopt.h>
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <gnu/lib-names.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+#include <support/xstdio.h>
+#include <support/support.h>
+
+static int restart;
+#define CMDLINE_OPTIONS \
+ { "restart", no_argument, &restart, 1 },
+
+static int
+handle_restart (void)
+{
+ {
+ void *h = xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
+
+ pid_t (*s)(void) = xdlsym (h, "getpid");
+ TEST_COMPARE (s (), getpid ());
+
+ xdlclose (h);
+ }
+
+ {
+ void *h = xdlmopen (LM_ID_NEWLM, "tst-audit20mod.so", RTLD_NOW);
+
+ int (*foo)(void) = xdlsym (h, "foo");
+ TEST_COMPARE (foo (), 10);
+
+ xdlclose (h);
+ }
+
+ return 0;
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+ /* We must have either:
+ - One our fource parameters left if called initially:
+ + path to ld.so optional
+ + "--library-path" optional
+ + the library path optional
+ + the application name */
+
+ if (restart)
+ return handle_restart ();
+
+ char *spargv[9];
+ int i = 0;
+ for (; i < argc - 1; i++)
+ spargv[i] = argv[i + 1];
+ spargv[i++] = (char *) "--direct";
+ spargv[i++] = (char *) "--restart";
+ spargv[i] = NULL;
+
+ setenv ("LD_AUDIT", "tst-auditmod20.so", 0);
+ struct support_capture_subprocess result
+ = support_capture_subprogram (spargv[0], spargv);
+ support_capture_subprocess_check (&result, "tst-audit20", 0, sc_allow_stderr);
+
+ struct
+ {
+ const char *name;
+ bool found;
+ } audit_iface[] =
+ {
+ { "la_version", false },
+ { "la_objsearch", false },
+ { "la_activity", false },
+ { "la_objopen", false },
+ { "la_objclose", false },
+ { "la_preinit", false },
+#if __WORDSIZE == 32
+ { "la_symbind32", false },
+#elif __WORDSIZE == 64
+ { "la_symbind64", false },
+#endif
+ };
+
+ /* Some hooks are called more than once but the test only check if any
+ is called at least once. */
+ FILE *out = fmemopen (result.err.buffer, result.err.length, "r");
+ TEST_VERIFY (out != NULL);
+ char *buffer = NULL;
+ size_t buffer_length = 0;
+ while (xgetline (&buffer, &buffer_length, out))
+ {
+ for (int i = 0; i < array_length (audit_iface); i++)
+ if (strncmp (buffer, audit_iface[i].name,
+ strlen (audit_iface[i].name)) == 0)
+ audit_iface[i].found = true;
+ }
+ free (buffer);
+ xfclose (out);
+
+ for (int i = 0; i < array_length (audit_iface); i++)
+ TEST_COMPARE (audit_iface[i].found, true);
+
+ support_capture_subprocess_free (&result);
+
+ return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
new file mode 100644
@@ -0,0 +1,26 @@
+/* Check DT_AUDIT with dlmopen.
+ Copyright (C) 2021 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
+ <https://www.gnu.org/licenses/>. */
+
+#include <fcntl.h>
+#include <gnu/lib-names.h>
+
+int
+foo (void)
+{
+ return 10;
+}
new file mode 100644
@@ -0,0 +1,73 @@
+/* Check DT_AUDIT with dlmopen.
+ Copyright (C) 2021 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
+ <https://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <link.h>
+
+unsigned int
+la_version (unsigned int version)
+{
+ fprintf (stderr, "%s\n", __func__);
+ return LAV_CURRENT;
+}
+
+char *
+la_objsearch (const char *name, uintptr_t *cookie, unsigned int flag)
+{
+ fprintf (stderr, "%s\n", __func__);
+ return (char *) name;
+}
+
+void
+la_activity (uintptr_t *cookie, unsigned int flag)
+{
+ fprintf (stderr, "%s\n", __func__);
+}
+
+unsigned int
+la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t *cookie)
+{
+ fprintf (stderr, "%s\n", __func__);
+ return LA_FLG_BINDTO | LA_FLG_BINDFROM;
+}
+
+unsigned int
+la_objclose (uintptr_t *cookie)
+{
+ fprintf (stderr, "%s\n", __func__);
+ return 0;
+}
+
+void
+la_preinit (uintptr_t *cookie)
+{
+ fprintf (stderr, "%s\n", __func__);
+}
+
+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
+{
+ fprintf (stderr, "%s\n", __func__);
+ return sym->st_value;
+}