elf: Earlier missing dynamic segment check in _dl_map_object_from_fd

  Separated debuginfo files have PT_DYNAMIC with p_filesz == 0.  We
need to check for that before the _dl_map_segments call because
that could attempt to write to mappings that extend beyond the end
of the file, resulting in SIGBUS.

Tested on i686-linux-gnu, x86_64-linux-gnu.  It fixes the elf/tst-debug1
failure on aarch64 for me, too.  build-many-glibcs.py is still running,
but initial results look good.

Thanks,
Florian
---
 elf/dl-load.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
  

On Fri, Nov 5, 2021 at 9:29 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Separated debuginfo files have PT_DYNAMIC with p_filesz == 0.  We
> need to check for that before the _dl_map_segments call because
> that could attempt to write to mappings that extend beyond the end
> of the file, resulting in SIGBUS.
>
> Tested on i686-linux-gnu, x86_64-linux-gnu.  It fixes the elf/tst-debug1
> failure on aarch64 for me, too.  build-many-glibcs.py is still running,
> but initial results look good.
>
> Thanks,
> Florian
> ---
>  elf/dl-load.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a1f1682188..a51b115a84 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1135,6 +1135,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      struct loadcmd loadcmds[l->l_phnum];
>      size_t nloadcmds = 0;
>      bool has_holes = false;
> +    bool empty_dynamic = false;
>
>      /* The struct is initialized to zero so this is not necessary:
>      l->l_ld = 0;
> @@ -1147,7 +1148,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>              segments are mapped in.  We record the addresses it says
>              verbatim, and later correct for the run-time load address.  */
>         case PT_DYNAMIC:
> -         if (ph->p_filesz)
> +         if (ph->p_filesz == 0)
> +           empty_dynamic = true; /* Usually separate debuginfo.  */
> +         else
>             {
>               /* Debuginfo only files from "objcopy --only-keep-debug"
>                  contain a PT_DYNAMIC segment with p_filesz == 0.  Skip
> @@ -1270,6 +1273,13 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>         goto lose;
>        }
>
> +    /* This check recognizes most separate debuginfo files.  */
> +    if (__glibc_unlikely (l->l_ld == 0 && type == ET_DYN) || empty_dynamic)

Shouldn't it be

 if (__glibc_unlikely ((l->l_ld == 0 && type == ET_DYN) || empty_dynamic))

> +      {
> +       errstring = N_("object file has no dynamic section");
> +       goto lose;
> +      }
> +
>      /* Length of the sections to be loaded.  */
>      maplength = loadcmds[nloadcmds - 1].allocend - loadcmds[0].mapstart;
>
> @@ -1287,15 +1297,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>        }
>    }
>
> -  if (l->l_ld == 0)
> -    {
> -      if (__glibc_unlikely (type == ET_DYN))
> -       {
> -         errstring = N_("object file has no dynamic section");
> -         goto lose;
> -       }
> -    }
> -  else
> +  if (l->l_ld != 0)
>      l->l_ld = (ElfW(Dyn) *) ((ElfW(Addr)) l->l_ld + l->l_addr);
>
>    elf_get_dynamic_info (l, false, false);
>
  

* H. J. Lu:

> On Fri, Nov 5, 2021 at 9:29 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> Separated debuginfo files have PT_DYNAMIC with p_filesz == 0.  We
>> need to check for that before the _dl_map_segments call because
>> that could attempt to write to mappings that extend beyond the end
>> of the file, resulting in SIGBUS.
>>
>> Tested on i686-linux-gnu, x86_64-linux-gnu.  It fixes the elf/tst-debug1
>> failure on aarch64 for me, too.  build-many-glibcs.py is still running,
>> but initial results look good.
>>
>> Thanks,
>> Florian
>> ---
>>  elf/dl-load.c | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index a1f1682188..a51b115a84 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1135,6 +1135,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>      struct loadcmd loadcmds[l->l_phnum];
>>      size_t nloadcmds = 0;
>>      bool has_holes = false;
>> +    bool empty_dynamic = false;
>>
>>      /* The struct is initialized to zero so this is not necessary:
>>      l->l_ld = 0;
>> @@ -1147,7 +1148,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>              segments are mapped in.  We record the addresses it says
>>              verbatim, and later correct for the run-time load address.  */
>>         case PT_DYNAMIC:
>> -         if (ph->p_filesz)
>> +         if (ph->p_filesz == 0)
>> +           empty_dynamic = true; /* Usually separate debuginfo.  */
>> +         else
>>             {
>>               /* Debuginfo only files from "objcopy --only-keep-debug"
>>                  contain a PT_DYNAMIC segment with p_filesz == 0.  Skip
>> @@ -1270,6 +1273,13 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>         goto lose;
>>        }
>>
>> +    /* This check recognizes most separate debuginfo files.  */
>> +    if (__glibc_unlikely (l->l_ld == 0 && type == ET_DYN) || empty_dynamic)
>
> Shouldn't it be
>
>  if (__glibc_unlikely ((l->l_ld == 0 && type == ET_DYN) || empty_dynamic))

Hmm, right.  Both cases are unlikely.  I'm going to send a v2.

Thanks,
Florian