diff mbox series

[v6,03/20] elf: Add _dl_audit_activity_map and _dl_audit_activity_nsid

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

Commit Message

Adhemerval Zanella Netto Nov. 15, 2021, 6:37 p.m. UTC 
It consolidates the code required to call la_activity() audit
callback.

Also 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, i686-linux-gnu, and aarch64-linux-gnu.
---
 elf/dl-audit.c             | 23 ++++++++++++++++
 elf/dl-close.c             | 54 +++++++-------------------------------
 elf/dl-load.c              | 20 +-------------
 elf/dl-open.c              | 20 +-------------
 elf/rtld.c                 | 31 ++--------------------
 sysdeps/generic/ldsodefs.h |  7 +++++
 6 files changed, 43 insertions(+), 112 deletions(-)

Comments

Florian Weimer Dec. 10, 2021, 1:54 p.m. UTC | #1 
* Adhemerval Zanella:

> diff --git a/elf/dl-close.c b/elf/dl-close.c
> index 4f5cfcc1c3..79dbb41094 100644
> --- a/elf/dl-close.c
> +++ b/elf/dl-close.c

> +  /* Auditing checkpoint: we have deleted all objects.  If head is NULL, the
> +     namespace has become empty, and the audit interface does not give us a
> +     way to signal LA_ACT_CONSISTENT for it because the first loaded module
> +     is used to identify the namespace.
> +
> +     Furthermore, do not notify auditors of the cleanup of a failed audit
> +     module loading attempt.  */
> +  _dl_audit_activity_nsid (nsid, LA_ACT_CONSISTENT);
>  #endif

The comment is not really well-placed here.  I think it belongs to the
head == NULL check in _dl_audit_activity_nsid.

>  
>    if (__builtin_expect (ns->_ns_loaded == NULL, 0)
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 907ab8f5f7..848d3e08eb 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1483,25 +1483,7 @@ 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.  */
> -      if ((mode & __RTLD_AUDIT) == 0
> -	  && __glibc_unlikely (GLRO(dl_naudit) > 0))

The mode & __RTLD_AUDIT check is not included in
_dl_audit_activity_nsid.

> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index dec592bf53..ea481cbe84 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1392,6 +1392,13 @@ link_map_audit_state (struct link_map *l, size_t index)
>      }
>  }
>  
> +/* Call the la_activity() from the audit modules from the link map L
> +   and issues the ACTION argument.  */
> +void _dl_audit_activity_map (struct link_map *l, int action)
> +  attribute_hidden;
> +/* Call the la_activity() from the audit modules from the link map
> +   from the namespace NSSID and issues the ACTION argument.  */
> +void _dl_audit_activity_nsid (Lmid_t nsid, int action);

(), typo: NS[S]ID.  Maybe we should have a general comment here that
these notifications are skipped for auditing namespaces?

Thanks,
Florian
Adhemerval Zanella Netto Dec. 10, 2021, 2:58 p.m. UTC | #2 
On 10/12/2021 10:54, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/elf/dl-close.c b/elf/dl-close.c
>> index 4f5cfcc1c3..79dbb41094 100644
>> --- a/elf/dl-close.c
>> +++ b/elf/dl-close.c
> 
>> +  /* Auditing checkpoint: we have deleted all objects.  If head is NULL, the
>> +     namespace has become empty, and the audit interface does not give us a
>> +     way to signal LA_ACT_CONSISTENT for it because the first loaded module
>> +     is used to identify the namespace.
>> +
>> +     Furthermore, do not notify auditors of the cleanup of a failed audit
>> +     module loading attempt.  */
>> +  _dl_audit_activity_nsid (nsid, LA_ACT_CONSISTENT);
>>  #endif
> 
> The comment is not really well-placed here.  I think it belongs to the
> head == NULL check in _dl_audit_activity_nsid.
> 

Ack.

>>  
>>    if (__builtin_expect (ns->_ns_loaded == NULL, 0)
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 907ab8f5f7..848d3e08eb 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1483,25 +1483,7 @@ 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.  */
>> -      if ((mode & __RTLD_AUDIT) == 0
>> -	  && __glibc_unlikely (GLRO(dl_naudit) > 0))
> 
> The mode & __RTLD_AUDIT check is not included in
> _dl_audit_activity_nsid.

Ack, I have reinstated it on _dl_map_object_from_fd.

> 
>> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
>> index dec592bf53..ea481cbe84 100644
>> --- a/sysdeps/generic/ldsodefs.h
>> +++ b/sysdeps/generic/ldsodefs.h
>> @@ -1392,6 +1392,13 @@ link_map_audit_state (struct link_map *l, size_t index)
>>      }
>>  }
>>  
>> +/* Call the la_activity() from the audit modules from the link map L
>> +   and issues the ACTION argument.  */
>> +void _dl_audit_activity_map (struct link_map *l, int action)
>> +  attribute_hidden;
>> +/* Call the la_activity() from the audit modules from the link map
>> +   from the namespace NSSID and issues the ACTION argument.  */
>> +void _dl_audit_activity_nsid (Lmid_t nsid, int action);
> 
> (), typo: NS[S]ID.  Maybe we should have a general comment here that
> these notifications are skipped for auditing namespaces?

Ack.  Yeah I think mention auditing namespaces is worth here.

> 
> Thanks,
> Florian
>
diff mbox series

Patch

diff --git a/elf/dl-audit.c b/elf/dl-audit.c
index 24d8557f18..5fbc76a36c 100644
--- a/elf/dl-audit.c
+++ b/elf/dl-audit.c
@@ -19,6 +19,29 @@ 
 #include <ldsodefs.h>
 
 #ifdef SHARED
+void
+_dl_audit_activity_map (struct link_map *l, int action)
+{
+  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 (l, cnt)->cookie, action);
+      afct = afct->next;
+    }
+}
+
+void
+_dl_audit_activity_nsid (Lmid_t nsid, int action)
+{
+  struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
+  if (__glibc_likely (GLRO(dl_naudit) == 0)
+      || head == NULL || head->l_auditing)
+    return;
+
+  _dl_audit_activity_map (head, action);
+}
+
 void
 _dl_audit_objopen (struct link_map *l, Lmid_t nsid, bool check_audit)
 {
diff --git a/elf/dl-close.c b/elf/dl-close.c
index 4f5cfcc1c3..79dbb41094 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -472,25 +472,7 @@  _dl_close_worker (struct link_map *map, bool force)
 
 #ifdef SHARED
   /* Auditing checkpoint: we will start deleting objects.  */
-  if (__glibc_unlikely (do_audit))
-    {
-      struct link_map *head = ns->_ns_loaded;
-      struct audit_ifaces *afct = GLRO(dl_audit);
-      /* Do not call the functions for any auditing object.  */
-      if (head->l_auditing == 0)
-	{
-	  for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
-	    {
-	      if (afct->activity != NULL)
-		{
-		  struct auditstate *state = link_map_audit_state (head, cnt);
-		  afct->activity (&state->cookie, LA_ACT_DELETE);
-		}
-
-	      afct = afct->next;
-	    }
-	}
-    }
+  _dl_audit_activity_nsid (nsid, LA_ACT_DELETE);
 #endif
 
   /* Notify the debugger we are about to remove some loaded objects.  */
@@ -785,32 +767,14 @@  _dl_close_worker (struct link_map *map, bool force)
   __rtld_lock_unlock_recursive (GL(dl_load_tls_lock));
 
 #ifdef SHARED
-  /* Auditing checkpoint: we have deleted all objects.  */
-  if (__glibc_unlikely (do_audit))
-    {
-      struct link_map *head = ns->_ns_loaded;
-      /* If head is NULL, the namespace has become empty, and the
-	 audit interface does not give us a way to signal
-	 LA_ACT_CONSISTENT for it because the first loaded module is
-	 used to identify the namespace.
-
-	 Furthermore, do not notify auditors of the cleanup of a
-	 failed audit module loading attempt.  */
-      if (head != NULL && 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)
-		{
-		  struct auditstate *state = link_map_audit_state (head, cnt);
-		  afct->activity (&state->cookie, LA_ACT_CONSISTENT);
-		}
-
-	      afct = afct->next;
-	    }
-	}
-    }
+  /* Auditing checkpoint: we have deleted all objects.  If head is NULL, the
+     namespace has become empty, and the audit interface does not give us a
+     way to signal LA_ACT_CONSISTENT for it because the first loaded module
+     is used to identify the namespace.
+
+     Furthermore, do not notify auditors of the cleanup of a failed audit
+     module loading attempt.  */
+  _dl_audit_activity_nsid (nsid, LA_ACT_CONSISTENT);
 #endif
 
   if (__builtin_expect (ns->_ns_loaded == NULL, 0)
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 907ab8f5f7..848d3e08eb 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1483,25 +1483,7 @@  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.  */
-      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;
-		}
-	    }
-	}
+      _dl_audit_activity_nsid (nsid, LA_ACT_ADD);
 #endif
 
       /* Notify the debugger we have added some objects.  We need to
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 6ea5dd2457..e2f2e713e7 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -611,25 +611,7 @@  dl_open_worker_begin (void *a)
 
 #ifdef SHARED
   /* Auditing checkpoint: we have added all objects.  */
-  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
-    {
-      struct link_map *head = GL(dl_ns)[new->l_ns]._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)
-		{
-		  struct auditstate *state = link_map_audit_state (head, cnt);
-		  afct->activity (&state->cookie, LA_ACT_CONSISTENT);
-		}
-
-	      afct = afct->next;
-	    }
-	}
-    }
+  _dl_audit_activity_nsid (new->l_ns, LA_ACT_CONSISTENT);
 #endif
 
   /* Notify the debugger all new objects are now ready to go.  */
diff --git a/elf/rtld.c b/elf/rtld.c
index 647f476957..dc20643ee9 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1759,18 +1759,7 @@  dl_main (const ElfW(Phdr) *phdr,
 
   /* Auditing checkpoint: we are ready to signal that the initial map
      is being constructed.  */
-  if (__glibc_unlikely (GLRO(dl_naudit) > 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 (main_map, cnt)->cookie,
-			    LA_ACT_ADD);
-
-	  afct = afct->next;
-	}
-    }
+  _dl_audit_activity_map (main_map, LA_ACT_ADD);
 
   /* We have two ways to specify objects to preload: via environment
      variable and via the file /etc/ld.so.preload.  The latter can also
@@ -2451,23 +2440,7 @@  dl_main (const ElfW(Phdr) *phdr,
 
 #ifdef SHARED
   /* Auditing checkpoint: we have added all objects.  */
-  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
-    {
-      struct link_map *head = GL(dl_ns)[LM_ID_BASE]._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_CONSISTENT);
-
-	      afct = afct->next;
-	    }
-	}
-    }
+  _dl_audit_activity_nsid (LM_ID_BASE, LA_ACT_CONSISTENT);
 #endif
 
   /* Notify the debugger all new objects are now ready to go.  We must re-get
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index dec592bf53..ea481cbe84 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -1392,6 +1392,13 @@  link_map_audit_state (struct link_map *l, size_t index)
     }
 }
 
+/* Call the la_activity() from the audit modules from the link map L
+   and issues the ACTION argument.  */
+void _dl_audit_activity_map (struct link_map *l, int action)
+  attribute_hidden;
+/* Call the la_activity() from the audit modules from the link map
+   from the namespace NSSID and issues the ACTION argument.  */
+void _dl_audit_activity_nsid (Lmid_t nsid, int action);
 /* Call the la_objopen() from the audit modules for the link_map L on the
    namespace identification NSID.  If CHECK_AUDIT is set it will also check
    if main mapping of the namespace is a audit modules.  */