Re: pr21665

[Alan Modra <amodra at gmail dot com>]

On Fri, Jun 30, 2017 at 03:54:18PM +0100, Nick Clifton wrote:
> Hi Alan,
> 
> > I haven't looked at the bug in detail, but since the testcases are
> > 64-bit, is the problem that on a 32-bit target we're not catching a
> > size_t overflow?
> 
> No - the problem is that the testcase has a pathological .init section:
> 
>   % readelf --wide -S POC2
>   ...
>   [11] .init             PROGBITS        0000000000401ab0 001ab0 800000001a 00  AX  0   0  4
>   ...
> 
> Note the size - 0x8000000001a - this is too much for xmalloc() to handle,
> (at least on my system), and it triggers an error report if run with 
> address sanitization enabled.
> 
> I do not think that we have to worry about overflow since datasize's type
> is bfd_size_type, which is always going to be at least an unsigned long,
> right ?

This is how the failure happens:

1) The fuzzed .init section size of 0x8000000001a is in a 64-bit
   bfd_size_type.
2) This value is passed to xmalloc, which on a 32-bit host takes a
   32-bit size_t, allocating just 26 bytes.
3) bfd_get_section_contents returns with a failure due to finding
   count != (size_t) count.  The error status is ignored by objdump
   (fixed on mainline by my pr21415 patch).
4) disassemble_bytes scans the buffer for zeros and runs off the end
   of the buffer, triggering asan.

There was no need for any patch for the pr21665 POC2 testcase on
mainline!

However, since I spent time analyzing the problem, it deserves a
patch.  It's nicer to use bfd_malloc_and_get_section rather than
xmalloc followed by bfd_get_section_contents, since xmalloc exits on
failure and needs a check that its size_t arg doesn't lose high bits
when converted from bfd_size_type.  This way, a corrupted .init
section header won't stop objdump dead.  You'll get an error about
.init, but the rest of the object can be dumped.
    
    	PR binutils/21665
    	* objdump.c (strtab): Make var a bfd_byte*.
    	(disassemble_section): Don't limit malloc size.  Instead, use
    	bfd_malloc_and_get_section.
    	(read_section_stabs): Use bfd_malloc_and_get_section.  Return
    	bfd_byte*.
    	(find_stabs_section): Remove now unnecessary cast.
    	* objcopy.c (copy_object): Use bfd_malloc_and_get_section.  Free
    	contents on error return.
    	* nlmconv.c (copy_sections): Use bfd_malloc_and_get_section.

diff --git a/binutils/nlmconv.c b/binutils/nlmconv.c
index 4cc659f..7e1eaac 100644
--- a/binutils/nlmconv.c
+++ b/binutils/nlmconv.c
@@ -1224,7 +1224,7 @@ copy_sections (bfd *inbfd, asection *insec, void *data_ptr)
   const char *inname;
   asection *outsec;
   bfd_size_type size;
-  void *contents;
+  bfd_byte *contents;
   long reloc_size;
   bfd_byte buf[4];
   bfd_size_type add;
@@ -1240,9 +1240,7 @@ copy_sections (bfd *inbfd, asection *insec, void *data_ptr)
     contents = NULL;
   else
     {
-      contents = xmalloc (size);
-      if (! bfd_get_section_contents (inbfd, insec, contents,
-				      (file_ptr) 0, size))
+      if (!bfd_malloc_and_get_section (inbfd, insec, &contents))
 	bfd_fatal (bfd_get_filename (inbfd));
     }
 
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 4f48190..23a949d 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2650,14 +2650,15 @@ copy_object (bfd *ibfd, bfd *obfd, const bfd_arch_info_type *input_arch)
 	      continue;
 	    }
 
-	  bfd_byte * contents = xmalloc (size);
-	  if (bfd_get_section_contents (ibfd, osec, contents, 0, size))
+	  bfd_byte *contents;
+	  if (bfd_malloc_and_get_section (ibfd, osec, &contents))
 	    {
 	      if (fwrite (contents, 1, size, f) != size)
 		{
 		  non_fatal (_("error writing section contents to %s (error: %s)"),
 			     pdump->filename,
 			     strerror (errno));
+		  free (contents);
 		  return FALSE;
 		}
 	    }
diff --git a/binutils/objdump.c b/binutils/objdump.c
index d70882e..3c5defa 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -181,7 +181,7 @@ static long dynsymcount = 0;
 static bfd_byte *stabs;
 static bfd_size_type stab_size;
 
-static char *strtab;
+static bfd_byte *strtab;
 static bfd_size_type stabstr_size;
 
 static bfd_boolean is_relocatable = FALSE;
@@ -2178,32 +2178,7 @@ disassemble_section (bfd *abfd, asection *section, void *inf)
     }
   rel_ppend = rel_pp + rel_count;
 
-  /* PR 21665: Check for overlarge datasizes.
-     Note - we used to check for "datasize > bfd_get_file_size (abfd)" but
-     this fails when using compressed sections or compressed file formats
-     (eg MMO, tekhex).
-
-     The call to xmalloc below will fail if too much memory is requested,
-     which will catch the problem in the normal use case.  But if a memory
-     checker is in use, eg valgrind or sanitize, then an exception will
-     be still generated, so we try to catch the problem first.
-
-     Unfortunately there is no simple way to determine how much memory can
-     be allocated by calling xmalloc.  So instead we use a simple, arbitrary
-     limit of 2Gb.  Hopefully this should be enough for most users.  If
-     someone does start trying to disassemble sections larger then 2Gb in
-     size they will doubtless complain and we can increase the limit.  */
-#define MAX_XMALLOC (1024 * 1024 * 1024 * 2UL) /* 2Gb */
-  if (datasize > MAX_XMALLOC)
-    {
-      non_fatal (_("Reading section %s failed because it is too big (%#lx)"),
-		 section->name, (unsigned long) datasize);
-      return;
-    }
-
-  data = (bfd_byte *) xmalloc (datasize);
-
-  if (!bfd_get_section_contents (abfd, section, data, 0, datasize))
+  if (!bfd_malloc_and_get_section (abfd, section, &data))
     {
       non_fatal (_("Reading section %s failed because: %s"),
 		 section->name, bfd_errmsg (bfd_get_error ()));
@@ -2725,12 +2700,11 @@ dump_dwarf (bfd *abfd)
 /* Read ABFD's stabs section STABSECT_NAME, and return a pointer to
    it.  Return NULL on failure.   */
 
-static char *
+static bfd_byte *
 read_section_stabs (bfd *abfd, const char *sect_name, bfd_size_type *size_ptr)
 {
   asection *stabsect;
-  bfd_size_type size;
-  char *contents;
+  bfd_byte *contents;
 
   stabsect = bfd_get_section_by_name (abfd, sect_name);
   if (stabsect == NULL)
@@ -2739,10 +2713,7 @@ read_section_stabs (bfd *abfd, const char *sect_name, bfd_size_type *size_ptr)
       return FALSE;
     }
 
-  size = bfd_section_size (abfd, stabsect);
-  contents  = (char *) xmalloc (size);
-
-  if (! bfd_get_section_contents (abfd, stabsect, contents, 0, size))
+  if (!bfd_malloc_and_get_section (abfd, stabsect, &contents))
     {
       non_fatal (_("reading %s section of %s failed: %s"),
 		 sect_name, bfd_get_filename (abfd),
@@ -2752,7 +2723,7 @@ read_section_stabs (bfd *abfd, const char *sect_name, bfd_size_type *size_ptr)
       return NULL;
     }
 
-  *size_ptr = size;
+  *size_ptr = bfd_section_size (abfd, stabsect);
 
   return contents;
 }
@@ -2879,8 +2850,7 @@ find_stabs_section (bfd *abfd, asection *section, void *names)
 
       if (strtab)
 	{
-	  stabs = (bfd_byte *) read_section_stabs (abfd, section->name,
-						   &stab_size);
+	  stabs = read_section_stabs (abfd, section->name, &stab_size);
 	  if (stabs)
 	    print_section_stabs (abfd, section->name, &sought->string_offset);
 	}

-- 
Alan Modra
Australia Development Lab, IBM