From 2482f30615136c7ed2082561f8e9dba48ee3ee25 Mon Sep 17 00:00:00 2001 From: Alan Modra Date: Fri, 24 Apr 2020 09:51:38 +0930 Subject: [PATCH] readelf: memory leaks in process_dynamic_section This fixes some code that assumed only one PT_LOAD would contain DT_SYMTAB. Which is normally the case, but fuzzers thoroughly mess with object files. * readelf.c (get_num_dynamic_syms): Check for nbuckets and nchains non-zero. (process_dynamic_section): Call get_num_dynamic_syms once rather than in segment loop. Break out of segment loop on a successful load of dynamic symbols. Formatting. (process_object): Return error status from process_dynamic_section. --- binutils/ChangeLog | 9 +++ binutils/readelf.c | 139 ++++++++++++++++++++++++--------------------- 2 files changed, 82 insertions(+), 66 deletions(-) diff --git a/binutils/ChangeLog b/binutils/ChangeLog index 7f7e34020d..ad3846a1c2 100644 --- a/binutils/ChangeLog +++ b/binutils/ChangeLog @@ -1,3 +1,12 @@ +2020-04-24 Alan Modra + + * readelf.c (get_num_dynamic_syms): Check for nbuckets and nchains + non-zero. + (process_dynamic_section): Call get_num_dynamic_syms once rather + than in segment loop. Break out of segment loop on a successful + load of dynamic symbols. Formatting. + (process_object): Return error status from process_dynamic_section. + 2020-04-23 Anton Kolesov * elf-bfd.h (elfcore_write_arc_v2): Add prototype. diff --git a/binutils/readelf.c b/binutils/readelf.c index f3e374dc54..8e8ade8fbe 100644 --- a/binutils/readelf.c +++ b/binutils/readelf.c @@ -9993,14 +9993,16 @@ get_num_dynamic_syms (Filedata * filedata) filedata->nbuckets = byte_get (nb, hash_ent_size); filedata->nchains = byte_get (nc, hash_ent_size); - filedata->buckets = get_dynamic_data (filedata, filedata->nbuckets, - hash_ent_size); - filedata->chains = get_dynamic_data (filedata, filedata->nchains, - hash_ent_size); - - if (filedata->buckets != NULL && filedata->chains != NULL) - num_of_syms = filedata->nchains; + if (filedata->nbuckets != 0 && filedata->nchains != 0) + { + filedata->buckets = get_dynamic_data (filedata, filedata->nbuckets, + hash_ent_size); + filedata->chains = get_dynamic_data (filedata, filedata->nchains, + hash_ent_size); + if (filedata->buckets != NULL && filedata->chains != NULL) + num_of_syms = filedata->nchains; + } no_hash: if (num_of_syms == 0) { @@ -10243,6 +10245,8 @@ process_dynamic_section (Filedata * filedata) /* Find the appropriate symbol table. */ if (filedata->dynamic_symbols == NULL || do_histogram) { + unsigned long num_of_syms; + for (entry = filedata->dynamic_section; entry < filedata->dynamic_section + filedata->dynamic_nent; ++entry) @@ -10262,64 +10266,65 @@ process_dynamic_section (Filedata * filedata) filedata->dynamic_info_DT_GNU_HASH = entry->d_un.d_val; } - if (filedata->dynamic_info[DT_SYMTAB] + num_of_syms = get_num_dynamic_syms (filedata); + + if (num_of_syms != 0 + && filedata->dynamic_symbols == NULL + && filedata->dynamic_info[DT_SYMTAB] && filedata->dynamic_info[DT_SYMENT]) { Elf_Internal_Phdr *seg; - bfd_vma vma = filedata->dynamic_info[DT_SYMTAB]; + bfd_vma vma = filedata->dynamic_info[DT_SYMTAB]; - if (! get_program_headers (filedata)) - { - error (_("Cannot interpret virtual addresses without program headers.\n")); - return FALSE; - } - - for (seg = filedata->program_headers; - seg < filedata->program_headers + filedata->file_header.e_phnum; - ++seg) - { - unsigned long num_of_syms; + if (! get_program_headers (filedata)) + { + error (_("Cannot interpret virtual addresses " + "without program headers.\n")); + return FALSE; + } - if (seg->p_type != PT_LOAD) - continue; + for (seg = filedata->program_headers; + seg < filedata->program_headers + filedata->file_header.e_phnum; + ++seg) + { + if (seg->p_type != PT_LOAD) + continue; - if ((seg->p_offset + seg->p_filesz) - > filedata->file_size) - { - /* See PR 21379 for a reproducer. */ - error (_("Invalid PT_LOAD entry\n")); - return FALSE; - } + if (seg->p_offset + seg->p_filesz > filedata->file_size) + { + /* See PR 21379 for a reproducer. */ + error (_("Invalid PT_LOAD entry\n")); + return FALSE; + } - if (vma >= (seg->p_vaddr & -seg->p_align) - && vma <= seg->p_vaddr + seg->p_filesz - && (num_of_syms = get_num_dynamic_syms (filedata)) != 0 - && filedata->dynamic_symbols == NULL) - { - /* Since we do not know how big the symbol table is, - we default to reading in up to the end of PT_LOAD - segment and processing that. This is overkill, I - know, but it should work. */ - Elf_Internal_Shdr section; - section.sh_offset = (vma - seg->p_vaddr - + seg->p_offset); - section.sh_size = (num_of_syms - * filedata->dynamic_info[DT_SYMENT]); - section.sh_entsize = filedata->dynamic_info[DT_SYMENT]; - section.sh_name = filedata->string_table_length; - filedata->dynamic_symbols - = GET_ELF_SYMBOLS (filedata, §ion, - &filedata->num_dynamic_syms); - if (filedata->dynamic_symbols == NULL - || filedata->num_dynamic_syms != num_of_syms) - { - error (_("Corrupt DT_SYMTAB dynamic entry\n")); - return FALSE; - } - } - } - } - } + if (vma >= (seg->p_vaddr & -seg->p_align) + && vma < seg->p_vaddr + seg->p_filesz) + { + /* Since we do not know how big the symbol table is, + we default to reading in up to the end of PT_LOAD + segment and processing that. This is overkill, I + know, but it should work. */ + Elf_Internal_Shdr section; + section.sh_offset = (vma - seg->p_vaddr + + seg->p_offset); + section.sh_size = (num_of_syms + * filedata->dynamic_info[DT_SYMENT]); + section.sh_entsize = filedata->dynamic_info[DT_SYMENT]; + section.sh_name = filedata->string_table_length; + filedata->dynamic_symbols + = GET_ELF_SYMBOLS (filedata, §ion, + &filedata->num_dynamic_syms); + if (filedata->dynamic_symbols == NULL + || filedata->num_dynamic_syms != num_of_syms) + { + error (_("Corrupt DT_SYMTAB dynamic entry\n")); + return FALSE; + } + break; + } + } + } + } /* Similarly find a string table. */ if (filedata->dynamic_strings == NULL) @@ -10403,14 +10408,17 @@ process_dynamic_section (Filedata * filedata) filedata->dynamic_syminfo = (Elf_Internal_Syminfo *) malloc (syminsz); if (filedata->dynamic_syminfo == NULL) { - error (_("Out of memory allocating %lu byte for dynamic symbol info\n"), + error (_("Out of memory allocating %lu bytes " + "for dynamic symbol info\n"), (unsigned long) syminsz); return FALSE; } - filedata->dynamic_syminfo_nent = syminsz / sizeof (Elf_External_Syminfo); + filedata->dynamic_syminfo_nent + = syminsz / sizeof (Elf_External_Syminfo); for (syminfo = filedata->dynamic_syminfo, extsym = extsyminfo; - syminfo < filedata->dynamic_syminfo + filedata->dynamic_syminfo_nent; + syminfo < (filedata->dynamic_syminfo + + filedata->dynamic_syminfo_nent); ++syminfo, ++extsym) { syminfo->si_boundto = BYTE_GET (extsym->si_boundto); @@ -20120,7 +20128,7 @@ process_object (Filedata * filedata) { bfd_boolean have_separate_files; unsigned int i; - bfd_boolean res = TRUE; + bfd_boolean res; if (! get_file_header (filedata)) { @@ -20176,10 +20184,9 @@ process_object (Filedata * filedata) /* Without loaded section groups we cannot process unwind. */ do_unwind = FALSE; - if (process_program_headers (filedata)) - process_dynamic_section (filedata); - else - res = FALSE; + res = process_program_headers (filedata); + if (res) + res = process_dynamic_section (filedata); if (! process_relocs (filedata)) res = FALSE; -- 2.34.1