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

["H.J. Lu" <hjl.tools@gmail.com>]

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.