[PATCH] readelf robustification (part N)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] readelf robustification (part N)

Jakub Jelinek
Hi!

The endless patchset continues.
This patch prevents crashes if readelf is run on objects with
bogus sh_entsize.

2005-06-14  Jakub Jelinek  <[hidden email]>

        * readelf.c (CHECK_ENTSIZE_VALUES, CHECK_ENTSIZE): Define.
        (process_section_headers): Use it.
        (process_relocs): Don't crash if symsec is not SHT_SYMTAB
        or SHT_DYNSYM.
        (process_version_sections): Use 2 instead of sh_entsize.

--- binutils/readelf.c.jj 2005-06-14 12:36:32.000000000 +0200
+++ binutils/readelf.c 2005-06-14 14:06:58.000000000 +0200
@@ -3871,6 +3871,20 @@ process_section_headers (FILE *file)
       break;
     }
 
+#define CHECK_ENTSIZE_VALUES(section, i, size32, size64) \
+  do {    \
+    size_t expected_entsize    \
+      = is_32bit_elf ? size32 : size64;    \
+    if (section->sh_entsize != expected_entsize)    \
+      error (_("Section %d has invalid sh_entsize %lx (expected %lx)\n"),   \
+     i, (unsigned long int) section->sh_entsize,    \
+     (unsigned long int) expected_entsize);    \
+    section->sh_entsize = expected_entsize;    \
+  } while (0)
+#define CHECK_ENTSIZE(section, i, type) \
+  CHECK_ENTSIZE_VALUES (section, i, sizeof (Elf32_External_##type),    \
+ sizeof (Elf64_External_##type))
+
   for (i = 0, section = section_headers;
        i < elf_header.e_shnum;
        i++, section++)
@@ -3885,6 +3899,7 @@ process_section_headers (FILE *file)
       continue;
     }
 
+  CHECK_ENTSIZE (section, i, Sym);
   num_dynamic_syms = section->sh_size / section->sh_entsize;
   dynamic_symbols = GET_ELF_SYMBOLS (file, section);
  }
@@ -3910,6 +3925,14 @@ process_section_headers (FILE *file)
     }
   symtab_shndx_hdr = section;
  }
+      else if (section->sh_type == SHT_SYMTAB)
+ CHECK_ENTSIZE (section, i, Sym);
+      else if (section->sh_type == SHT_GROUP)
+ CHECK_ENTSIZE_VALUES (section, i, 4, 4);
+      else if (section->sh_type == SHT_REL)
+ CHECK_ENTSIZE (section, i, Rel);
+      else if (section->sh_type == SHT_RELA)
+ CHECK_ENTSIZE (section, i, Rela);
       else if ((do_debugging || do_debug_info || do_debug_abbrevs
  || do_debug_lines || do_debug_pubnames || do_debug_aranges
  || do_debug_frames || do_debug_macinfo || do_debug_str
@@ -4488,6 +4511,10 @@ process_relocs (FILE *file)
   char *strtab = NULL;
 
   symsec = SECTION_HEADER (section->sh_link);
+  if (symsec->sh_type != SHT_SYMTAB
+      && symsec->sh_type != SHT_DYNSYM)
+                    continue;
+
   nsyms = symsec->sh_size / symsec->sh_entsize;
   symtab = GET_ELF_SYMBOLS (file, symsec);
 
@@ -6358,7 +6385,7 @@ process_version_sections (FILE *file)
       break;
 
     link_section = SECTION_HEADER (section->sh_link);
-    total = section->sh_size / section->sh_entsize;
+    total = section->sh_size / 2;
 
     if (SECTION_HEADER_INDEX (link_section->sh_link)
  >= elf_header.e_shnum)

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] readelf robustification (part N)

Nick Clifton
Hi Jakub,

> 2005-06-14  Jakub Jelinek  <[hidden email]>
>
> * readelf.c (CHECK_ENTSIZE_VALUES, CHECK_ENTSIZE): Define.
> (process_section_headers): Use it.
> (process_relocs): Don't crash if symsec is not SHT_SYMTAB
> or SHT_DYNSYM.
> (process_version_sections): Use 2 instead of sh_entsize.

This is mostly OK, but:

> +#define CHECK_ENTSIZE_VALUES(section, i, size32, size64) \
> +  do {    \
> +    size_t expected_entsize    \
> +      = is_32bit_elf ? size32 : size64;    \
> +    if (section->sh_entsize != expected_entsize)    \
> +      error (_("Section %d has invalid sh_entsize %lx (expected %lx)\n"),   \
> +     i, (unsigned long int) section->sh_entsize,    \
> +     (unsigned long int) expected_entsize);    \
> +    section->sh_entsize = expected_entsize;    \
> +  } while (0)

Formatting.  Just because this is a macro there is no reason to abandon
the GNU Coding standard.  Please treat the macro like ordinary code and
break out the curly braces to their own lines.


> +      else if (section->sh_type == SHT_GROUP)
> + CHECK_ENTSIZE_VALUES (section, i, 4, 4);

I am slightly worried about the appearance of the "4" here.  I do not
like magic constants in code, although I see that "4" has been used in
this context elsewhere in the readelf sources.  I would be much happier
if we had a macro value here instead. eg:

   #define GROUP_ENTRY_SIZE  4

Then this could be used elsewhere in the group processing code as well.


> -    total = section->sh_size / section->sh_entsize;
> +    total = section->sh_size / 2;

I am worried by this "2" as well.  If you are concerned that sh_entsize
might be invalid here, shouldn't you be checking it ?  Also I see that
the code further on uses "sizeof (short)" instead of "2", but this seems
unsafe to me, since there is no guarantee that the size of a short on
the system running readelf is 2.  (It might be 1 on a 16-bit host...)

I think that we need another defined constant here (eg
VERSYM_ENTRY_SIZE) and a check to make size that "sizeof (*data) >=
VERSYM_ENTRY_SIZE".

Cheers
   Nick