diff mbox series

[v6,01/20] elf: Suppress audit calls when a (new) namespace is empty (BZ #28062)

Message ID 20211115183734.531155-2-adhemerval.zanella@linaro.org
State Committed
Headers show
Series Multiple rtld-audit fixes | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Nov. 15, 2021, 6:37 p.m. UTC
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.
---
 elf/Makefile         |   9 ++-
 elf/dl-load.c        |  72 ++++++++++++------------
 elf/tst-audit18.c    | 129 +++++++++++++++++++++++++++++++++++++++++++
 elf/tst-audit18mod.c |  23 ++++++++
 elf/tst-auditmod18.c |  73 ++++++++++++++++++++++++
 5 files changed, 269 insertions(+), 37 deletions(-)
 create mode 100644 elf/tst-audit18.c
 create mode 100644 elf/tst-audit18mod.c
 create mode 100644 elf/tst-auditmod18.c

Comments

Florian Weimer Nov. 15, 2021, 7:01 p.m. UTC | #1
* Adhemerval Zanella:

> 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.

Looks like you forgot to update the commit message.

My patch linter also flags these:

+    pid_t (*s)(void) = xdlsym (h, "getpid");
+    int (*foo)(void) = xdlsym (h, "foo");

Missing space before “(void)”.

> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 9f4fa9617d..9538bcb7dc 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c

> @@ -1515,6 +1479,42 @@ cannot enable executable stack as shared object requires");
>    /* Now that the object is fully initialized add it to the object list.  */
>    _dl_add_to_namespace_list (l, nsid);
>  
> +  /* Signal that we are going to add new objects.  */
> +  if (r->r_state == RT_CONSISTENT)
> +    {
> +#ifdef SHARED
> +      /* Auditing checkpoint: we are going to add new objects.  */
> +      if ((mode & __RTLD_AUDIT) == 0
> +	  && __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)
> +	    {
> +	      struct audit_ifaces *afct = GLRO(dl_audit);
> +	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
> +		{
> +		  if (afct->activity != NULL)
> +		    afct->activity (&link_map_audit_state (head, cnt)->cookie,
> +				    LA_ACT_ADD);
> +
> +		  afct = afct->next;
> +		}
> +	    }
> +	}
> +#endif

Please add a comment that this happens after _dl_add_to_namespace_list,
so that the namespace is guaranteed not to be empty.

Thanks,
Florian
Adhemerval Zanella Nov. 16, 2021, 1:14 p.m. UTC | #2
On 15/11/2021 16:01, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> 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.
> 
> Looks like you forgot to update the commit message.

Indeed, the title is also misleading.  I have changed to:

  elf: Move la_activity (LA_ACT_ADD) after _dl_add_to_namespace_list() (BZ #28062)

  It ensures that the the namespace is guaranteed to not be empty.

> 
> My patch linter also flags these:
> 
> +    pid_t (*s)(void) = xdlsym (h, "getpid");
> +    int (*foo)(void) = xdlsym (h, "foo");
> 
> Missing space before “(void)”.

Ack.

> 
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 9f4fa9617d..9538bcb7dc 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
> 
>> @@ -1515,6 +1479,42 @@ cannot enable executable stack as shared object requires");
>>    /* Now that the object is fully initialized add it to the object list.  */
>>    _dl_add_to_namespace_list (l, nsid);
>>  
>> +  /* Signal that we are going to add new objects.  */
>> +  if (r->r_state == RT_CONSISTENT)
>> +    {
>> +#ifdef SHARED
>> +      /* Auditing checkpoint: we are going to add new objects.  */
>> +      if ((mode & __RTLD_AUDIT) == 0
>> +	  && __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)
>> +	    {
>> +	      struct audit_ifaces *afct = GLRO(dl_audit);
>> +	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>> +		{
>> +		  if (afct->activity != NULL)
>> +		    afct->activity (&link_map_audit_state (head, cnt)->cookie,
>> +				    LA_ACT_ADD);
>> +
>> +		  afct = afct->next;
>> +		}
>> +	    }
>> +	}
>> +#endif
> 
> Please add a comment that this happens after _dl_add_to_namespace_list,
> so that the namespace is guaranteed not to be empty.

I have changed to:

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 9538bcb7dc..240b5efb45 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1483,7 +1483,9 @@ cannot enable executable stack as shared object requires");
   if (r->r_state == RT_CONSISTENT)
     {
 #ifdef SHARED
-      /* Auditing checkpoint: we are going to add new objects.  */
+      /* Auditing checkpoint: we are going to add new objects.  Since this
+         is called after _dl_add_to_namespace_list() the namespace is
+        guaranteed to not be empty.  */
       if ((mode & __RTLD_AUDIT) == 0
          && __glibc_unlikely (GLRO(dl_naudit) > 0))
        {
Florian Weimer Nov. 16, 2021, 1:15 p.m. UTC | #3
* Adhemerval Zanella:

> On 15/11/2021 16:01, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> 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.
>> 
>> Looks like you forgot to update the commit message.
>
> Indeed, the title is also misleading.  I have changed to:
>
>   elf: Move la_activity (LA_ACT_ADD) after _dl_add_to_namespace_list() (BZ #28062)
>
>   It ensures that the the namespace is guaranteed to not be empty.

Looks good with these changes.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian
Andreas Schwab Nov. 16, 2021, 1:45 p.m. UTC | #4
On Nov 16 2021, Adhemerval Zanella via Libc-alpha wrote:

> +      /* Auditing checkpoint: we are going to add new objects.  Since this
> +         is called after _dl_add_to_namespace_list() the namespace is

Not a function call here.

Andreas.
Florian Weimer Nov. 16, 2021, 1:48 p.m. UTC | #5
* Andreas Schwab:

> On Nov 16 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> +      /* Auditing checkpoint: we are going to add new objects.  Since this
>> +         is called after _dl_add_to_namespace_list() the namespace is
>
> Not a function call here.

I've stopped pointing those out in patch reviews.  Is this GNU
convention really useful?  If there is no other markup, it often
increases readability, particular for identifiers that are also bona
fide English words, such as read and close.

Thanks,
Florian
Andreas Schwab Nov. 16, 2021, 2:16 p.m. UTC | #6
On Nov 16 2021, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Nov 16 2021, Adhemerval Zanella via Libc-alpha wrote:
>>
>>> +      /* Auditing checkpoint: we are going to add new objects.  Since this
>>> +         is called after _dl_add_to_namespace_list() the namespace is
>>
>> Not a function call here.
>
> I've stopped pointing those out in patch reviews.  Is this GNU
> convention really useful?  If there is no other markup, it often
> increases readability,

You can always write "the foo function".

> particular for identifiers that are also bona fide English words, such
> as read and close.

Which only proves my point.

Andreas.
Adhemerval Zanella Nov. 18, 2021, 7:58 p.m. UTC | #7
On 16/11/2021 11:16, Andreas Schwab wrote:
> On Nov 16 2021, Florian Weimer wrote:
> 
>> * Andreas Schwab:
>>
>>> On Nov 16 2021, Adhemerval Zanella via Libc-alpha wrote:
>>>
>>>> +      /* Auditing checkpoint: we are going to add new objects.  Since this
>>>> +         is called after _dl_add_to_namespace_list() the namespace is
>>>
>>> Not a function call here.
>>
>> I've stopped pointing those out in patch reviews.  Is this GNU
>> convention really useful?  If there is no other markup, it often
>> increases readability,
> 
> You can always write "the foo function".
> 
>> particular for identifiers that are also bona fide English words, such
>> as read and close.
> 
> Which only proves my point.

Ok, I will remove the '()' and reference to just the function name.
diff mbox series

Patch

diff --git a/elf/Makefile b/elf/Makefile
index a311c3e23c..8f3e3a3602 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -229,7 +229,8 @@  tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-tls-ie tst-tls-ie-dlmopen argv0test \
 	 tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
 	 tst-tls20 tst-tls21 tst-dlmopen-dlerror tst-dlmopen-gethostbyname \
-	 tst-dl-is_dso tst-ro-dynamic
+	 tst-dl-is_dso tst-ro-dynamic \
+	 tst-audit18 \
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -370,6 +371,8 @@  modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-tls20mod-bad tst-tls21mod tst-dlmopen-dlerror-mod \
 		tst-auxvalmod \
 		tst-dlmopen-gethostbyname-mod tst-ro-dynamic-mod \
+		tst-auditmod18 \
+		tst-audit18mod \
 
 # Most modules build with _ISOMAC defined, but those filtered out
 # depend on internal headers.
@@ -1537,6 +1540,10 @@  $(objpfx)tst-audit16-cmp.out: tst-audit16.exp $(objpfx)tst-audit16.out
 	cmp $^ > $@; \
 	$(evaluate-test)
 
+$(objpfx)tst-audit18.out: $(objpfx)tst-auditmod18.so \
+			  $(objpfx)tst-audit18mod.so
+tst-audit18-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 \
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 9f4fa9617d..9538bcb7dc 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1058,42 +1058,6 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
   /* This is the ELF header.  We read it in `open_verify'.  */
   header = (void *) fbp->buf;
 
-  /* Signal that we are going to add new objects.  */
-  if (r->r_state == RT_CONSISTENT)
-    {
-#ifdef SHARED
-      /* Auditing checkpoint: we are going to add new objects.  */
-      if ((mode & __RTLD_AUDIT) == 0
-	  && __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)
-	    {
-	      struct audit_ifaces *afct = GLRO(dl_audit);
-	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
-		{
-		  if (afct->activity != NULL)
-		    afct->activity (&link_map_audit_state (head, cnt)->cookie,
-				    LA_ACT_ADD);
-
-		  afct = afct->next;
-		}
-	    }
-	}
-#endif
-
-      /* Notify the debugger we have added some objects.  We need to
-	 call _dl_debug_initialize in a static program in case dynamic
-	 linking has not been used before.  */
-      r->r_state = RT_ADD;
-      _dl_debug_state ();
-      LIBC_PROBE (map_start, 2, nsid, r);
-      make_consistent = true;
-    }
-  else
-    assert (r->r_state == RT_ADD);
-
   /* Enter the new object in the list of loaded objects.  */
   l = _dl_new_object (realname, name, l_type, loader, mode, nsid);
   if (__glibc_unlikely (l == NULL))
@@ -1515,6 +1479,42 @@  cannot enable executable stack as shared object requires");
   /* Now that the object is fully initialized add it to the object list.  */
   _dl_add_to_namespace_list (l, nsid);
 
+  /* Signal that we are going to add new objects.  */
+  if (r->r_state == RT_CONSISTENT)
+    {
+#ifdef SHARED
+      /* Auditing checkpoint: we are going to add new objects.  */
+      if ((mode & __RTLD_AUDIT) == 0
+	  && __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)
+	    {
+	      struct audit_ifaces *afct = GLRO(dl_audit);
+	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
+		{
+		  if (afct->activity != NULL)
+		    afct->activity (&link_map_audit_state (head, cnt)->cookie,
+				    LA_ACT_ADD);
+
+		  afct = afct->next;
+		}
+	    }
+	}
+#endif
+
+      /* Notify the debugger we have added some objects.  We need to
+	 call _dl_debug_initialize in a static program in case dynamic
+	 linking has not been used before.  */
+      r->r_state = RT_ADD;
+      _dl_debug_state ();
+      LIBC_PROBE (map_start, 2, nsid, r);
+      make_consistent = true;
+    }
+  else
+    assert (r->r_state == RT_ADD);
+
 #ifdef SHARED
   /* Auditing checkpoint: we have a new object.  */
   if (__glibc_unlikely (GLRO(dl_naudit) > 0)
diff --git a/elf/tst-audit18.c b/elf/tst-audit18.c
new file mode 100644
index 0000000000..b5ed15d07f
--- /dev/null
+++ b/elf/tst-audit18.c
@@ -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-audit18mod.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-auditmod18.so", 0);
+  struct support_capture_subprocess result
+    = support_capture_subprogram (spargv[0], spargv);
+  support_capture_subprocess_check (&result, "tst-audit18", 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>
diff --git a/elf/tst-audit18mod.c b/elf/tst-audit18mod.c
new file mode 100644
index 0000000000..096a9167c9
--- /dev/null
+++ b/elf/tst-audit18mod.c
@@ -0,0 +1,23 @@ 
+/* 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/>.  */
+
+int
+foo (void)
+{
+  return 10;
+}
diff --git a/elf/tst-auditmod18.c b/elf/tst-auditmod18.c
new file mode 100644
index 0000000000..182992e9fd
--- /dev/null
+++ b/elf/tst-auditmod18.c
@@ -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;
+}