diff mbox series

[v7,03/16] elf: Add _dl_audit_objsearch

Message ID 20211222132712.523295-4-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 Dec. 22, 2021, 1:26 p.m. UTC
It consolidates the code required to call la_objsearch audit
callback.

Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.
---
 elf/dl-audit.c             | 22 ++++++++++++
 elf/dl-load.c              | 69 +++++++++++---------------------------
 sysdeps/generic/ldsodefs.h |  7 ++++
 3 files changed, 48 insertions(+), 50 deletions(-)

Comments

Florian Weimer Dec. 24, 2021, 12:05 p.m. UTC | #1
* Adhemerval Zanella:

> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index d1d02fa70e..cf15f85df6 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1602,32 +1602,20 @@ open_verify (const char *name, int fd,

> -      if (fd != -1 && name != original_name && strcmp (name, original_name))
> -        {
> -          /* An audit library changed what we're supposed to open,
> -             so FD no longer matches it.  */
> -          __close_nocancel (fd);
> -          fd = -1;
> -        }
> +       if (fd != -1 && name != original_name && strcmp (name, original_name))
> +	 {
> +	   /* An audit library changed what we're supposed to open,
> +	      so FD no longer matches it.  */
> +	   __close_nocancel (fd);
> +	   fd = -1;
> +	 }

Spurious whitespace change, I think.

> @@ -2066,36 +2054,17 @@ _dl_map_object (struct link_map *loader, const char *name,
>  #ifdef SHARED
>    /* Give the auditing libraries a chance to change the name before we
>       try anything.  */
> -  if (__glibc_unlikely (GLRO(dl_naudit) > 0)
> -      && (loader == NULL || loader->l_auditing == 0))
> +  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
>      {
> -      struct audit_ifaces *afct = GLRO(dl_audit);
> -      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
> +      const char *before = name;
> +      name = _dl_audit_objsearch (name, loader, LA_SER_ORIG);
> +      if (name == NULL)
>  	{
> -	  if (afct->objsearch != NULL)
> -	    {
> -	      const char *before = name;
> -	      struct auditstate *state = link_map_audit_state (loader, cnt);
> -	      name = afct->objsearch (name, &state->cookie, LA_SER_ORIG);
> -	      if (name == NULL)
> -		{
> -		  /* Do not try anything further.  */
> -		  fd = -1;
> -		  goto no_file;
> -		}
> -	      if (before != name && strcmp (before, name) != 0)
> -		{
> -		  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
> -		    _dl_debug_printf ("audit changed filename %s -> %s\n",
> -				      before, name);
> -
> -		  if (origname == NULL)
> -		    origname = before;
> -		}
> -	    }
> -
> -	  afct = afct->next;
> +	  fd = -1;
> +	  goto no_file;
>  	}
> +      if (before != name && strcmp (before, name) != 0)
> +	origname = before;
>      }
>  #endif

I had to stare at this for a bit, but the new handling of originame and
before should give the same result in the end.

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

Thanks,
Florian
Adhemerval Zanella Dec. 27, 2021, 12:20 p.m. UTC | #2
On 24/12/2021 09:05, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index d1d02fa70e..cf15f85df6 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1602,32 +1602,20 @@ open_verify (const char *name, int fd,
> 
>> -      if (fd != -1 && name != original_name && strcmp (name, original_name))
>> -        {
>> -          /* An audit library changed what we're supposed to open,
>> -             so FD no longer matches it.  */
>> -          __close_nocancel (fd);
>> -          fd = -1;
>> -        }
>> +       if (fd != -1 && name != original_name && strcmp (name, original_name))
>> +	 {
>> +	   /* An audit library changed what we're supposed to open,
>> +	      so FD no longer matches it.  */
>> +	   __close_nocancel (fd);
>> +	   fd = -1;
>> +	 }
> 
> Spurious whitespace change, I think.

Ack, it also leads to wrong indentation. I will fix it.

> 
>> @@ -2066,36 +2054,17 @@ _dl_map_object (struct link_map *loader, const char *name,
>>  #ifdef SHARED
>>    /* Give the auditing libraries a chance to change the name before we
>>       try anything.  */
>> -  if (__glibc_unlikely (GLRO(dl_naudit) > 0)
>> -      && (loader == NULL || loader->l_auditing == 0))
>> +  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
>>      {
>> -      struct audit_ifaces *afct = GLRO(dl_audit);
>> -      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>> +      const char *before = name;
>> +      name = _dl_audit_objsearch (name, loader, LA_SER_ORIG);
>> +      if (name == NULL)
>>  	{
>> -	  if (afct->objsearch != NULL)
>> -	    {
>> -	      const char *before = name;
>> -	      struct auditstate *state = link_map_audit_state (loader, cnt);
>> -	      name = afct->objsearch (name, &state->cookie, LA_SER_ORIG);
>> -	      if (name == NULL)
>> -		{
>> -		  /* Do not try anything further.  */
>> -		  fd = -1;
>> -		  goto no_file;
>> -		}
>> -	      if (before != name && strcmp (before, name) != 0)
>> -		{
>> -		  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
>> -		    _dl_debug_printf ("audit changed filename %s -> %s\n",
>> -				      before, name);
>> -
>> -		  if (origname == NULL)
>> -		    origname = before;
>> -		}
>> -	    }
>> -
>> -	  afct = afct->next;
>> +	  fd = -1;
>> +	  goto no_file;
>>  	}
>> +      if (before != name && strcmp (before, name) != 0)
>> +	origname = before;
>>      }
>>  #endif
> 
> I had to stare at this for a bit, but the new handling of originame and
> before should give the same result in the end.
> 
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> 
> Thanks,
> Florian
>
diff mbox series

Patch

diff --git a/elf/dl-audit.c b/elf/dl-audit.c
index b44ecde135..522fc14db1 100644
--- a/elf/dl-audit.c
+++ b/elf/dl-audit.c
@@ -48,6 +48,28 @@  _dl_audit_activity_nsid (Lmid_t nsid, int action)
   _dl_audit_activity_map (head, action);
 }
 
+const char *
+_dl_audit_objsearch (const char *name, struct link_map *l, unsigned int code)
+{
+  if (l == NULL || l->l_auditing || code == 0)
+    return name;
+
+  struct audit_ifaces *afct = GLRO(dl_audit);
+  for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
+    {
+      if (afct->objsearch != NULL)
+	{
+	  struct auditstate *state = link_map_audit_state (l, cnt);
+	  name = afct->objsearch (name, &state->cookie, code);
+	  if (name == NULL)
+	    return NULL;
+	}
+      afct = afct->next;
+   }
+
+  return name;
+}
+
 void
 _dl_audit_objopen (struct link_map *l, Lmid_t nsid)
 {
diff --git a/elf/dl-load.c b/elf/dl-load.c
index d1d02fa70e..cf15f85df6 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1602,32 +1602,20 @@  open_verify (const char *name, int fd,
 
 #ifdef SHARED
   /* Give the auditing libraries a chance.  */
-  if (__glibc_unlikely (GLRO(dl_naudit) > 0) && whatcode != 0
-      && loader->l_auditing == 0)
+  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
     {
       const char *original_name = name;
-      struct audit_ifaces *afct = GLRO(dl_audit);
-      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
-	{
-	  if (afct->objsearch != NULL)
-	    {
-	      struct auditstate *state = link_map_audit_state (loader, cnt);
-	      name = afct->objsearch (name, &state->cookie, whatcode);
-	      if (name == NULL)
-		/* Ignore the path.  */
-		return -1;
-	    }
-
-	  afct = afct->next;
-	}
+      name = _dl_audit_objsearch (name, loader, whatcode);
+      if (name == NULL)
+	return -1;
 
-      if (fd != -1 && name != original_name && strcmp (name, original_name))
-        {
-          /* An audit library changed what we're supposed to open,
-             so FD no longer matches it.  */
-          __close_nocancel (fd);
-          fd = -1;
-        }
+       if (fd != -1 && name != original_name && strcmp (name, original_name))
+	 {
+	   /* An audit library changed what we're supposed to open,
+	      so FD no longer matches it.  */
+	   __close_nocancel (fd);
+	   fd = -1;
+	 }
     }
 #endif
 
@@ -2066,36 +2054,17 @@  _dl_map_object (struct link_map *loader, const char *name,
 #ifdef SHARED
   /* Give the auditing libraries a chance to change the name before we
      try anything.  */
-  if (__glibc_unlikely (GLRO(dl_naudit) > 0)
-      && (loader == NULL || loader->l_auditing == 0))
+  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
     {
-      struct audit_ifaces *afct = GLRO(dl_audit);
-      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
+      const char *before = name;
+      name = _dl_audit_objsearch (name, loader, LA_SER_ORIG);
+      if (name == NULL)
 	{
-	  if (afct->objsearch != NULL)
-	    {
-	      const char *before = name;
-	      struct auditstate *state = link_map_audit_state (loader, cnt);
-	      name = afct->objsearch (name, &state->cookie, LA_SER_ORIG);
-	      if (name == NULL)
-		{
-		  /* Do not try anything further.  */
-		  fd = -1;
-		  goto no_file;
-		}
-	      if (before != name && strcmp (before, name) != 0)
-		{
-		  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
-		    _dl_debug_printf ("audit changed filename %s -> %s\n",
-				      before, name);
-
-		  if (origname == NULL)
-		    origname = before;
-		}
-	    }
-
-	  afct = afct->next;
+	  fd = -1;
+	  goto no_file;
 	}
+      if (before != name && strcmp (before, name) != 0)
+	origname = before;
     }
 #endif
 
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index e54efe65c8..f1c1281f4b 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -1396,6 +1396,13 @@  link_map_audit_state (struct link_map *l, size_t index)
     }
 }
 
+/* Call the la_objsearch from the audit modules from the link map L.  If
+   ORIGNAME is non NULL, it is updated with the revious name prior calling
+   la_objsearch.  */
+const char *_dl_audit_objsearch (const char *name, struct link_map *l,
+				 unsigned int code)
+   attribute_hidden;
+
 /* 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)