[PATCH 0/2] Avoid some crashes when loading separate debuginfo

[PATCH 0/2] Avoid some crashes when loading separate debuginfo

[Florian Weimer]

This mini-series adds a small check to detect invalid LOAD segments.

Tested on i686-linux-gnu, x86_64-linux-gnu.  Built with
build-many-glibcs.py.

Florian Weimer (2):
  Use sysdeps/posix/dl-fileid.h as the generic and only implementation
  elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

 elf/dl-load.c               | 78 +++++++++++++++++++++----------------
 sysdeps/generic/dl-fileid.h | 35 ++++++++++-------
 sysdeps/posix/dl-fileid.h   | 50 ------------------------
 3 files changed, 65 insertions(+), 98 deletions(-)
 delete mode 100644 sysdeps/posix/dl-fileid.h


base-commit: ff012870b2c02a62598c04daa1e54632e020fd7d
-- 
2.33.1

[PATCH 1/2] Use sysdeps/posix/dl-fileid.h as the generic and only implementation

[Florian Weimer]

---
 sysdeps/generic/dl-fileid.h | 30 ++++++++++++----------
 sysdeps/posix/dl-fileid.h   | 50 -------------------------------------
 2 files changed, 17 insertions(+), 63 deletions(-)
 delete mode 100644 sysdeps/posix/dl-fileid.h

diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h
index fb1e1c788b..bf437f3d71 100644
--- a/sysdeps/generic/dl-fileid.h
+++ b/sysdeps/generic/dl-fileid.h
@@ -1,4 +1,4 @@
-/* File identity for the dynamic linker.  Stub version.
+/* File identity for the dynamic linker.  Generic POSIX.1 version.
    Copyright (C) 2015-2021 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -17,30 +17,34 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <stdbool.h>
+#include <sys/stat.h>
 
-/* This type stores whatever information is fetched by _dl_get_file_id
-   and compared by _dl_file_id_match_p.  */
+/* For POSIX.1 systems, the pair of st_dev and st_ino constitute
+   a unique identifier for a file.  */
 struct r_file_id
   {
-    /* In the stub version, we don't record anything at all.  */
+    dev_t dev;
+    ino64_t ino;
   };
 
 /* Sample FD to fill in *ID.  Returns true on success.
    On error, returns false, with errno set.  */
 static inline bool
-_dl_get_file_id (int fd __attribute__ ((unused)),
-		 struct r_file_id *id __attribute__ ((unused)))
+_dl_get_file_id (int fd, struct r_file_id *id)
 {
+  struct __stat64_t64 st;
+
+  if (__glibc_unlikely (__fstat64_time64 (fd, &st) < 0))
+    return false;
+
+  id->dev = st.st_dev;
+  id->ino = st.st_ino;
   return true;
 }
 
-/* Compare two results from _dl_get_file_id for equality.
-   It's crucial that this never return false-positive matches.
-   It's ideal that it never return false-negative mismatches either,
-   but lack of matches is survivable.  */
+/* Compare two results from _dl_get_file_id for equality.  */
 static inline bool
-_dl_file_id_match_p (const struct r_file_id *a __attribute__ ((unused)),
-		     const struct r_file_id *b __attribute__ ((unused)))
+_dl_file_id_match_p (const struct r_file_id *a, const struct r_file_id *b)
 {
-  return false;
+  return a->dev == b->dev && a->ino == b->ino;
 }
diff --git a/sysdeps/posix/dl-fileid.h b/sysdeps/posix/dl-fileid.h
deleted file mode 100644
index bf437f3d71..0000000000
--- a/sysdeps/posix/dl-fileid.h
+++ /dev/null
@@ -1,50 +0,0 @@
-/* File identity for the dynamic linker.  Generic POSIX.1 version.
-   Copyright (C) 2015-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 <stdbool.h>
-#include <sys/stat.h>
-
-/* For POSIX.1 systems, the pair of st_dev and st_ino constitute
-   a unique identifier for a file.  */
-struct r_file_id
-  {
-    dev_t dev;
-    ino64_t ino;
-  };
-
-/* Sample FD to fill in *ID.  Returns true on success.
-   On error, returns false, with errno set.  */
-static inline bool
-_dl_get_file_id (int fd, struct r_file_id *id)
-{
-  struct __stat64_t64 st;
-
-  if (__glibc_unlikely (__fstat64_time64 (fd, &st) < 0))
-    return false;
-
-  id->dev = st.st_dev;
-  id->ino = st.st_ino;
-  return true;
-}
-
-/* Compare two results from _dl_get_file_id for equality.  */
-static inline bool
-_dl_file_id_match_p (const struct r_file_id *a, const struct r_file_id *b)
-{
-  return a->dev == b->dev && a->ino == b->ino;
-}
-- 
2.33.1

[PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Florian Weimer]

We occasionally see elf/tst-debug1 failing with a SIGBUS error
with some toolchain versions (recently on aarch64 only).  This test
tries to emulate loading separated debuginfo, but whether the test
object triggers the crash depends on many factors.  Accurately
rejected separated debuginfo in dlopen probably needs ELF markup,
at least if this is to be done efficiently using program headers
only.  But this change still improves user experience slightly.
We already obtain the file size from the kernel in most cases,
so no additional system call is added.

Based on earlier downstream-only patch by Jeff Law.
---
 elf/dl-load.c               | 78 +++++++++++++++++++++----------------
 sysdeps/generic/dl-fileid.h |  7 ++--
 2 files changed, 49 insertions(+), 36 deletions(-)

diff --git a/elf/dl-load.c b/elf/dl-load.c
index a1f1682188..a758bed9af 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -953,47 +953,48 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
   struct r_debug *r = _dl_debug_update (nsid);
   bool make_consistent = false;
 
-  /* Get file information.  To match the kernel behavior, do not fill
-     in this information for the executable in case of an explicit
-     loader invocation.  */
+  /* Get file information.  */
   struct r_file_id id;
+  off64_t file_size;
+  if (__glibc_unlikely (!_dl_get_file_id (fd, &id, &file_size)))
+    {
+      errstring = N_("cannot stat shared object");
+    lose_errno:
+      errval = errno;
+    lose:
+      /* The file might already be closed.  */
+      if (fd != -1)
+	__close_nocancel (fd);
+      if (l != NULL && l->l_map_start != 0)
+	_dl_unmap_segments (l);
+      if (l != NULL && l->l_origin != (char *) -1l)
+	free ((char *) l->l_origin);
+      if (l != NULL && !l->l_libname->dont_free)
+	free (l->l_libname);
+      if (l != NULL && l->l_phdr_allocated)
+	free ((void *) l->l_phdr);
+      free (l);
+      free (realname);
+
+      if (make_consistent && r != NULL)
+	{
+	  r->r_state = RT_CONSISTENT;
+	  _dl_debug_state ();
+	  LIBC_PROBE (map_failed, 2, nsid, r);
+	}
+
+      _dl_signal_error (errval, name, NULL, errstring);
+    }
+
   if (mode & __RTLD_OPENEXEC)
     {
       assert (nsid == LM_ID_BASE);
+      /* To match the kernel behavior, do not fill in this information
+	 for the executable in case of an explicit loader invocation.  */
       memset (&id, 0, sizeof (id));
     }
   else
     {
-      if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
-	{
-	  errstring = N_("cannot stat shared object");
-	lose_errno:
-	  errval = errno;
-	lose:
-	  /* The file might already be closed.  */
-	  if (fd != -1)
-	    __close_nocancel (fd);
-	  if (l != NULL && l->l_map_start != 0)
-	    _dl_unmap_segments (l);
-	  if (l != NULL && l->l_origin != (char *) -1l)
-	    free ((char *) l->l_origin);
-	  if (l != NULL && !l->l_libname->dont_free)
-	    free (l->l_libname);
-	  if (l != NULL && l->l_phdr_allocated)
-	    free ((void *) l->l_phdr);
-	  free (l);
-	  free (realname);
-
-	  if (make_consistent && r != NULL)
-	    {
-	      r->r_state = RT_CONSISTENT;
-	      _dl_debug_state ();
-	      LIBC_PROBE (map_failed, 2, nsid, r);
-	    }
-
-	  _dl_signal_error (errval, name, NULL, errstring);
-	}
-
       /* Look again to see if the real name matched another already loaded.  */
       for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
 	if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
@@ -1177,6 +1178,17 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 		= N_("ELF load command address/offset not properly aligned");
 	      goto lose;
 	    }
+         if (__glibc_unlikely (ph->p_offset + ph->p_filesz > file_size))
+           {
+             /* If the segment is not fully backed by the file,
+		accessing memory beyond the last full page results in
+		SIGBUS.  This often happens with non-loadable ELF
+		objects containing separated debugging information
+		(which have load segments that match the original ELF
+		file).  */
+             errstring = N_("ELF load command past end of file");
+             goto lose;
+           }
 
 	  struct loadcmd *c = &loadcmds[nloadcmds++];
 	  c->mapstart = ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h
index bf437f3d71..c59627429c 100644
--- a/sysdeps/generic/dl-fileid.h
+++ b/sysdeps/generic/dl-fileid.h
@@ -27,10 +27,10 @@ struct r_file_id
     ino64_t ino;
   };
 
-/* Sample FD to fill in *ID.  Returns true on success.
-   On error, returns false, with errno set.  */
+/* Sample FD to fill in *ID, *SIZE.  Returns true on success.  On
+   error, returns false, with errno set.  */
 static inline bool
-_dl_get_file_id (int fd, struct r_file_id *id)
+_dl_get_file_id (int fd, struct r_file_id *id, off64_t *size)
 {
   struct __stat64_t64 st;
 
@@ -39,6 +39,7 @@ _dl_get_file_id (int fd, struct r_file_id *id)
 
   id->dev = st.st_dev;
   id->ino = st.st_ino;
+  *size = st.st_size;
   return true;
 }
 
-- 
2.33.1

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[H.J. Lu]

On Fri, Nov 5, 2021 at 7:01 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> We occasionally see elf/tst-debug1 failing with a SIGBUS error
> with some toolchain versions (recently on aarch64 only).  This test
> tries to emulate loading separated debuginfo, but whether the test
> object triggers the crash depends on many factors.  Accurately
> rejected separated debuginfo in dlopen probably needs ELF markup,
> at least if this is to be done efficiently using program headers
> only.  But this change still improves user experience slightly.
> We already obtain the file size from the kernel in most cases,
> so no additional system call is added.
>
> Based on earlier downstream-only patch by Jeff Law.
> ---
>  elf/dl-load.c               | 78 +++++++++++++++++++++----------------
>  sysdeps/generic/dl-fileid.h |  7 ++--
>  2 files changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a1f1682188..a758bed9af 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -953,47 +953,48 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>    struct r_debug *r = _dl_debug_update (nsid);
>    bool make_consistent = false;
>
> -  /* Get file information.  To match the kernel behavior, do not fill
> -     in this information for the executable in case of an explicit
> -     loader invocation.  */
> +  /* Get file information.  */
>    struct r_file_id id;
> +  off64_t file_size;
> +  if (__glibc_unlikely (!_dl_get_file_id (fd, &id, &file_size)))
> +    {
> +      errstring = N_("cannot stat shared object");
> +    lose_errno:
> +      errval = errno;
> +    lose:
> +      /* The file might already be closed.  */
> +      if (fd != -1)
> +       __close_nocancel (fd);
> +      if (l != NULL && l->l_map_start != 0)
> +       _dl_unmap_segments (l);
> +      if (l != NULL && l->l_origin != (char *) -1l)
> +       free ((char *) l->l_origin);
> +      if (l != NULL && !l->l_libname->dont_free)
> +       free (l->l_libname);
> +      if (l != NULL && l->l_phdr_allocated)
> +       free ((void *) l->l_phdr);
> +      free (l);
> +      free (realname);
> +
> +      if (make_consistent && r != NULL)
> +       {
> +         r->r_state = RT_CONSISTENT;
> +         _dl_debug_state ();
> +         LIBC_PROBE (map_failed, 2, nsid, r);
> +       }
> +
> +      _dl_signal_error (errval, name, NULL, errstring);
> +    }
> +
>    if (mode & __RTLD_OPENEXEC)
>      {
>        assert (nsid == LM_ID_BASE);
> +      /* To match the kernel behavior, do not fill in this information
> +        for the executable in case of an explicit loader invocation.  */
>        memset (&id, 0, sizeof (id));
>      }
>    else
>      {
> -      if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
> -       {
> -         errstring = N_("cannot stat shared object");
> -       lose_errno:
> -         errval = errno;
> -       lose:
> -         /* The file might already be closed.  */
> -         if (fd != -1)
> -           __close_nocancel (fd);
> -         if (l != NULL && l->l_map_start != 0)
> -           _dl_unmap_segments (l);
> -         if (l != NULL && l->l_origin != (char *) -1l)
> -           free ((char *) l->l_origin);
> -         if (l != NULL && !l->l_libname->dont_free)
> -           free (l->l_libname);
> -         if (l != NULL && l->l_phdr_allocated)
> -           free ((void *) l->l_phdr);
> -         free (l);
> -         free (realname);
> -
> -         if (make_consistent && r != NULL)
> -           {
> -             r->r_state = RT_CONSISTENT;
> -             _dl_debug_state ();
> -             LIBC_PROBE (map_failed, 2, nsid, r);
> -           }
> -
> -         _dl_signal_error (errval, name, NULL, errstring);
> -       }
> -
>        /* Look again to see if the real name matched another already loaded.  */
>        for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
>         if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
> @@ -1177,6 +1178,17 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>                 = N_("ELF load command address/offset not properly aligned");
>               goto lose;
>             }
> +         if (__glibc_unlikely (ph->p_offset + ph->p_filesz > file_size))
> +           {
> +             /* If the segment is not fully backed by the file,
> +               accessing memory beyond the last full page results in
> +               SIGBUS.  This often happens with non-loadable ELF
> +               objects containing separated debugging information
> +               (which have load segments that match the original ELF
> +               file).  */
> +             errstring = N_("ELF load command past end of file");
> +             goto lose;
> +           }
>
>           struct loadcmd *c = &loadcmds[nloadcmds++];
>           c->mapstart = ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
> diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h
> index bf437f3d71..c59627429c 100644
> --- a/sysdeps/generic/dl-fileid.h
> +++ b/sysdeps/generic/dl-fileid.h
> @@ -27,10 +27,10 @@ struct r_file_id
>      ino64_t ino;
>    };
>
> -/* Sample FD to fill in *ID.  Returns true on success.
> -   On error, returns false, with errno set.  */
> +/* Sample FD to fill in *ID, *SIZE.  Returns true on success.  On
> +   error, returns false, with errno set.  */
>  static inline bool
> -_dl_get_file_id (int fd, struct r_file_id *id)
> +_dl_get_file_id (int fd, struct r_file_id *id, off64_t *size)
>  {
>    struct __stat64_t64 st;
>
> @@ -39,6 +39,7 @@ _dl_get_file_id (int fd, struct r_file_id *id)
>
>    id->dev = st.st_dev;
>    id->ino = st.st_ino;
> +  *size = st.st_size;

Why not add a size field to struct r_file_id?

>    return true;
>  }
>
> --
> 2.33.1
>


-- 
H.J.

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Florian Weimer]

* H. J. Lu:

>> diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h
>> index bf437f3d71..c59627429c 100644
>> --- a/sysdeps/generic/dl-fileid.h
>> +++ b/sysdeps/generic/dl-fileid.h
>> @@ -27,10 +27,10 @@ struct r_file_id
>>      ino64_t ino;
>>    };
>>
>> -/* Sample FD to fill in *ID.  Returns true on success.
>> -   On error, returns false, with errno set.  */
>> +/* Sample FD to fill in *ID, *SIZE.  Returns true on success.  On
>> +   error, returns false, with errno set.  */
>>  static inline bool
>> -_dl_get_file_id (int fd, struct r_file_id *id)
>> +_dl_get_file_id (int fd, struct r_file_id *id, off64_t *size)
>>  {
>>    struct __stat64_t64 st;
>>
>> @@ -39,6 +39,7 @@ _dl_get_file_id (int fd, struct r_file_id *id)
>>
>>    id->dev = st.st_dev;
>>    id->ino = st.st_ino;
>> +  *size = st.st_size;
>
> Why not add a size field to struct r_file_id?

struct r_file_d is stored in the link map, but we don't need the size
information there.

Thanks,
Florian

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[H.J. Lu]

On Fri, Nov 5, 2021 at 7:01 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> We occasionally see elf/tst-debug1 failing with a SIGBUS error
> with some toolchain versions (recently on aarch64 only).  This test
> tries to emulate loading separated debuginfo, but whether the test
> object triggers the crash depends on many factors.  Accurately
> rejected separated debuginfo in dlopen probably needs ELF markup,
> at least if this is to be done efficiently using program headers
> only.  But this change still improves user experience slightly.
> We already obtain the file size from the kernel in most cases,
> so no additional system call is added.

Under what conditions does the test trigger SIGBUS?  Does your
patch makes the test pass or just turn SIGBUS into a different
failure?

>
> Based on earlier downstream-only patch by Jeff Law.
> ---
>  elf/dl-load.c               | 78 +++++++++++++++++++++----------------
>  sysdeps/generic/dl-fileid.h |  7 ++--
>  2 files changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a1f1682188..a758bed9af 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -953,47 +953,48 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>    struct r_debug *r = _dl_debug_update (nsid);
>    bool make_consistent = false;
>
> -  /* Get file information.  To match the kernel behavior, do not fill
> -     in this information for the executable in case of an explicit
> -     loader invocation.  */
> +  /* Get file information.  */
>    struct r_file_id id;
> +  off64_t file_size;
> +  if (__glibc_unlikely (!_dl_get_file_id (fd, &id, &file_size)))
> +    {
> +      errstring = N_("cannot stat shared object");
> +    lose_errno:
> +      errval = errno;
> +    lose:
> +      /* The file might already be closed.  */
> +      if (fd != -1)
> +       __close_nocancel (fd);
> +      if (l != NULL && l->l_map_start != 0)
> +       _dl_unmap_segments (l);
> +      if (l != NULL && l->l_origin != (char *) -1l)
> +       free ((char *) l->l_origin);
> +      if (l != NULL && !l->l_libname->dont_free)
> +       free (l->l_libname);
> +      if (l != NULL && l->l_phdr_allocated)
> +       free ((void *) l->l_phdr);
> +      free (l);
> +      free (realname);
> +
> +      if (make_consistent && r != NULL)
> +       {
> +         r->r_state = RT_CONSISTENT;
> +         _dl_debug_state ();
> +         LIBC_PROBE (map_failed, 2, nsid, r);
> +       }
> +
> +      _dl_signal_error (errval, name, NULL, errstring);
> +    }
> +
>    if (mode & __RTLD_OPENEXEC)
>      {
>        assert (nsid == LM_ID_BASE);
> +      /* To match the kernel behavior, do not fill in this information
> +        for the executable in case of an explicit loader invocation.  */
>        memset (&id, 0, sizeof (id));
>      }
>    else
>      {
> -      if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
> -       {
> -         errstring = N_("cannot stat shared object");
> -       lose_errno:
> -         errval = errno;
> -       lose:
> -         /* The file might already be closed.  */
> -         if (fd != -1)
> -           __close_nocancel (fd);
> -         if (l != NULL && l->l_map_start != 0)
> -           _dl_unmap_segments (l);
> -         if (l != NULL && l->l_origin != (char *) -1l)
> -           free ((char *) l->l_origin);
> -         if (l != NULL && !l->l_libname->dont_free)
> -           free (l->l_libname);
> -         if (l != NULL && l->l_phdr_allocated)
> -           free ((void *) l->l_phdr);
> -         free (l);
> -         free (realname);
> -
> -         if (make_consistent && r != NULL)
> -           {
> -             r->r_state = RT_CONSISTENT;
> -             _dl_debug_state ();
> -             LIBC_PROBE (map_failed, 2, nsid, r);
> -           }
> -
> -         _dl_signal_error (errval, name, NULL, errstring);
> -       }
> -
>        /* Look again to see if the real name matched another already loaded.  */
>        for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
>         if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
> @@ -1177,6 +1178,17 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>                 = N_("ELF load command address/offset not properly aligned");
>               goto lose;
>             }
> +         if (__glibc_unlikely (ph->p_offset + ph->p_filesz > file_size))
> +           {
> +             /* If the segment is not fully backed by the file,
> +               accessing memory beyond the last full page results in
> +               SIGBUS.  This often happens with non-loadable ELF
> +               objects containing separated debugging information
> +               (which have load segments that match the original ELF
> +               file).  */
> +             errstring = N_("ELF load command past end of file");
> +             goto lose;
> +           }
>
>           struct loadcmd *c = &loadcmds[nloadcmds++];
>           c->mapstart = ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
> diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h
> index bf437f3d71..c59627429c 100644
> --- a/sysdeps/generic/dl-fileid.h
> +++ b/sysdeps/generic/dl-fileid.h
> @@ -27,10 +27,10 @@ struct r_file_id
>      ino64_t ino;
>    };
>
> -/* Sample FD to fill in *ID.  Returns true on success.
> -   On error, returns false, with errno set.  */
> +/* Sample FD to fill in *ID, *SIZE.  Returns true on success.  On
> +   error, returns false, with errno set.  */
>  static inline bool
> -_dl_get_file_id (int fd, struct r_file_id *id)
> +_dl_get_file_id (int fd, struct r_file_id *id, off64_t *size)
>  {
>    struct __stat64_t64 st;
>
> @@ -39,6 +39,7 @@ _dl_get_file_id (int fd, struct r_file_id *id)
>
>    id->dev = st.st_dev;
>    id->ino = st.st_ino;
> +  *size = st.st_size;
>    return true;
>  }
>
> --
> 2.33.1
>


-- 
H.J.

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Adhemerval Zanella]

On 05/11/2021 10:59, Florian Weimer via Libc-alpha wrote:

> @@ -1177,6 +1178,17 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  		= N_("ELF load command address/offset not properly aligned");
>  	      goto lose;
>  	    }
> +         if (__glibc_unlikely (ph->p_offset + ph->p_filesz > file_size))
> +           {
> +             /* If the segment is not fully backed by the file,
> +		accessing memory beyond the last full page results in
> +		SIGBUS.  This often happens with non-loadable ELF
> +		objects containing separated debugging information
> +		(which have load segments that match the original ELF
> +		file).  */
> +             errstring = N_("ELF load command past end of file");
> +             goto lose;
> +           }

Are these still valid objects? How does this object are created exactly?
You state that it happens with 'some toolchain versions', is this 
something new or has it be fixed (if it is an issue) upstream?

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Florian Weimer]

* H. J. Lu:

> On Fri, Nov 5, 2021 at 7:01 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> We occasionally see elf/tst-debug1 failing with a SIGBUS error
>> with some toolchain versions (recently on aarch64 only).  This test
>> tries to emulate loading separated debuginfo, but whether the test
>> object triggers the crash depends on many factors.  Accurately
>> rejected separated debuginfo in dlopen probably needs ELF markup,
>> at least if this is to be done efficiently using program headers
>> only.  But this change still improves user experience slightly.
>> We already obtain the file size from the kernel in most cases,
>> so no additional system call is added.
>
> Under what conditions does the test trigger SIGBUS?

If the separated debuginfo object is shorter than the original object,
and the dynamic loader tries to access something in the load segment
that extends beyond the end of the file.  I suspect triggering the
actual crash depends on placement of the dynamic segments and of
relocation targets.

> Does your patch makes the test pass or just turn SIGBUS into a
> different failure?

It makes the test pass because the dlopen reports the new error for this
particular error condition.  Of course, the test can still crash due to
invalid data in the dynamic segment if we are unlucky.

Thanks,
Florian

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Florian Weimer]

* Adhemerval Zanella:

> On 05/11/2021 10:59, Florian Weimer via Libc-alpha wrote:
>
>> @@ -1177,6 +1178,17 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>  		= N_("ELF load command address/offset not properly aligned");
>>  	      goto lose;
>>  	    }
>> +         if (__glibc_unlikely (ph->p_offset + ph->p_filesz > file_size))
>> +           {
>> +             /* If the segment is not fully backed by the file,
>> +		accessing memory beyond the last full page results in
>> +		SIGBUS.  This often happens with non-loadable ELF
>> +		objects containing separated debugging information
>> +		(which have load segments that match the original ELF
>> +		file).  */
>> +             errstring = N_("ELF load command past end of file");
>> +             goto lose;
>> +           }
>
> Are these still valid objects?

They are not, but we have a test that attempts to load such an object:
elf/tst-debug1.

There have been occasional user reports about crashes involving
separated debuginfo, hence the test.

> How does this object are created exactly?

Seperated debuginfo:

$(objpfx)tst-debug1mod1.so: $(objpfx)testobj1.so
	$(OBJCOPY) --only-keep-debug $< $@

> You state that it happens with 'some toolchain versions', is this 
> something new or has it be fixed (if it is an issue) upstream?

This test is very sensitive to ELF file layout.  The layout changes
depending on configuration (e.g., if annobin is used).  This part is
expected; it is not a bug.

Thanks,
Florian

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[H.J. Lu]

On Fri, Nov 5, 2021 at 7:31 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Fri, Nov 5, 2021 at 7:01 AM Florian Weimer via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >> We occasionally see elf/tst-debug1 failing with a SIGBUS error
> >> with some toolchain versions (recently on aarch64 only).  This test
> >> tries to emulate loading separated debuginfo, but whether the test
> >> object triggers the crash depends on many factors.  Accurately
> >> rejected separated debuginfo in dlopen probably needs ELF markup,
> >> at least if this is to be done efficiently using program headers
> >> only.  But this change still improves user experience slightly.
> >> We already obtain the file size from the kernel in most cases,
> >> so no additional system call is added.
> >
> > Under what conditions does the test trigger SIGBUS?
>
> If the separated debuginfo object is shorter than the original object,
> and the dynamic loader tries to access something in the load segment
> that extends beyond the end of the file.  I suspect triggering the
> actual crash depends on placement of the dynamic segments and of
> relocation targets.
>
> > Does your patch makes the test pass or just turn SIGBUS into a
> > different failure?
>
> It makes the test pass because the dlopen reports the new error for this
> particular error condition.  Of course, the test can still crash due to
> invalid data in the dynamic segment if we are unlucky.

So dlopen should reject it.   Can you identify the broken tools which
generate such input files and black list them for this test?  Of course,
ld.so can improve sanity checks.   But we need really broken inputs
for such checks.

-- 
H.J.

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Florian Weimer]

* H. J. Lu:

> So dlopen should reject it.   Can you identify the broken tools which
> generate such input files and black list them for this test?

It's objcopy --only-keep-debug, and it behaves as expted Separated
debuginfo is broken by design.  The program headers do not correspond to
the file contents, but match the original ELF file.

> Of course, ld.so can improve sanity checks.  But we need really broken
> inputs for such checks.

elf/tst-debug1 deliberately uses a broken input file.

Thanks,
Florian

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[H.J. Lu]

On Fri, Nov 5, 2021 at 7:41 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > So dlopen should reject it.   Can you identify the broken tools which
> > generate such input files and black list them for this test?
>
> It's objcopy --only-keep-debug, and it behaves as expted Separated
> debuginfo is broken by design.  The program headers do not correspond to
> the file contents, but match the original ELF file.

So the current checks aren't sufficient and your patch also checks
file size.  On x86-64, where is the first failed check? Why doesn't it
need to check file size?

> > Of course, ld.so can improve sanity checks.  But we need really broken
> > inputs for such checks.
>
> elf/tst-debug1 deliberately uses a broken input file.
>
> Thanks,
> Florian
>


-- 
H.J.

Re: [PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading

[Florian Weimer]

* H. J. Lu:

> On Fri, Nov 5, 2021 at 7:41 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > So dlopen should reject it.   Can you identify the broken tools which
>> > generate such input files and black list them for this test?
>>
>> It's objcopy --only-keep-debug, and it behaves as expted Separated
>> debuginfo is broken by design.  The program headers do not correspond to
>> the file contents, but match the original ELF file.
>
> So the current checks aren't sufficient and your patch also checks
> file size.  On x86-64, where is the first failed check? Why doesn't it
> need to check file size?

Excellent question.  I should have checked this as the first thing.

On x86-64, dlopen fails with “tst-debug1mod1.so: object file has no
dynamic section.”  This is because the file size is 0:

  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
  DYNAMIC        0x000df8 0x0000000000003e10 0x0000000000003e10 0x000000 0x0001d0 RW  0x8

The dynamic segment has file size zero on aarch64, as well.  But
_dl_map_segments is called before the l->l_ld check in
_dl_map_object_from_fd.  Due to 64 KiB page alignment, it is more likely
that we hit the memset call there to clear a partially mapped page,
hence the crash.

We I think we can move the l->l_ld check earlier and avoid the crash.
I'll send a completely different patch.

Thanks,
Florian

Re: [PATCH 1/2] Use sysdeps/posix/dl-fileid.h as the generic and only implementation

[H.J. Lu]

On Fri, Nov 5, 2021 at 7:02 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> ---
>  sysdeps/generic/dl-fileid.h | 30 ++++++++++++----------
>  sysdeps/posix/dl-fileid.h   | 50 -------------------------------------
>  2 files changed, 17 insertions(+), 63 deletions(-)
>  delete mode 100644 sysdeps/posix/dl-fileid.h
>
> diff --git a/sysdeps/generic/dl-fileid.h b/sysdeps/generic/dl-fileid.h
> index fb1e1c788b..bf437f3d71 100644
> --- a/sysdeps/generic/dl-fileid.h
> +++ b/sysdeps/generic/dl-fileid.h
> @@ -1,4 +1,4 @@
> -/* File identity for the dynamic linker.  Stub version.
> +/* File identity for the dynamic linker.  Generic POSIX.1 version.
>     Copyright (C) 2015-2021 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>
> @@ -17,30 +17,34 @@
>     <https://www.gnu.org/licenses/>.  */
>
>  #include <stdbool.h>
> +#include <sys/stat.h>
>
> -/* This type stores whatever information is fetched by _dl_get_file_id
> -   and compared by _dl_file_id_match_p.  */
> +/* For POSIX.1 systems, the pair of st_dev and st_ino constitute
> +   a unique identifier for a file.  */
>  struct r_file_id
>    {
> -    /* In the stub version, we don't record anything at all.  */
> +    dev_t dev;
> +    ino64_t ino;
>    };
>
>  /* Sample FD to fill in *ID.  Returns true on success.
>     On error, returns false, with errno set.  */
>  static inline bool
> -_dl_get_file_id (int fd __attribute__ ((unused)),
> -                struct r_file_id *id __attribute__ ((unused)))
> +_dl_get_file_id (int fd, struct r_file_id *id)
>  {
> +  struct __stat64_t64 st;
> +
> +  if (__glibc_unlikely (__fstat64_time64 (fd, &st) < 0))
> +    return false;
> +
> +  id->dev = st.st_dev;
> +  id->ino = st.st_ino;
>    return true;
>  }
>
> -/* Compare two results from _dl_get_file_id for equality.
> -   It's crucial that this never return false-positive matches.
> -   It's ideal that it never return false-negative mismatches either,
> -   but lack of matches is survivable.  */
> +/* Compare two results from _dl_get_file_id for equality.  */
>  static inline bool
> -_dl_file_id_match_p (const struct r_file_id *a __attribute__ ((unused)),
> -                    const struct r_file_id *b __attribute__ ((unused)))
> +_dl_file_id_match_p (const struct r_file_id *a, const struct r_file_id *b)
>  {
> -  return false;
> +  return a->dev == b->dev && a->ino == b->ino;
>  }
> diff --git a/sysdeps/posix/dl-fileid.h b/sysdeps/posix/dl-fileid.h
> deleted file mode 100644
> index bf437f3d71..0000000000
> --- a/sysdeps/posix/dl-fileid.h
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/* File identity for the dynamic linker.  Generic POSIX.1 version.
> -   Copyright (C) 2015-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 <stdbool.h>
> -#include <sys/stat.h>
> -
> -/* For POSIX.1 systems, the pair of st_dev and st_ino constitute
> -   a unique identifier for a file.  */
> -struct r_file_id
> -  {
> -    dev_t dev;
> -    ino64_t ino;
> -  };
> -
> -/* Sample FD to fill in *ID.  Returns true on success.
> -   On error, returns false, with errno set.  */
> -static inline bool
> -_dl_get_file_id (int fd, struct r_file_id *id)
> -{
> -  struct __stat64_t64 st;
> -
> -  if (__glibc_unlikely (__fstat64_time64 (fd, &st) < 0))
> -    return false;
> -
> -  id->dev = st.st_dev;
> -  id->ino = st.st_ino;
> -  return true;
> -}
> -
> -/* Compare two results from _dl_get_file_id for equality.  */
> -static inline bool
> -_dl_file_id_match_p (const struct r_file_id *a, const struct r_file_id *b)
> -{
> -  return a->dev == b->dev && a->ino == b->ino;
> -}
> --
> 2.33.1
>
>

Please submit the v2 patch with the updated cover letter to indicate
that this patch set doesn't fix any bug, but is a cleanup.

Thanks.

-- 
H.J.