elf: Earlier missing dynamic segment check in _dl_map_object_from_fd
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
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(-)
Comments
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
@@ -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)
+ {
+ 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);