[PATCH 2/2] elf: Detect PT_LOAD segments that extend beyond EOF and refuse loading
H.J. Lu
hjl.tools@gmail.com
Fri Nov 5 14:04:48 GMT 2021
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.
More information about the Libc-alpha
mailing list