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

  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(-)
  

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. 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
  

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
>
  

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?
  

* 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
  

* 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
  

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. 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
  

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. 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