Re: [patch bfd]: Prevent possible buffer overflow on pdata-section sorting

[Kai Tietz <ktietz70@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

2011/4/9 Alan Modra <amodra@gmail.com>:
> On Thu, Apr 07, 2011 at 04:31:45PM +0200, Kai Tietz wrote:
>> 2011/4/7 Alan Modra <amodra@gmail.com>:
>> > On Thu, Apr 07, 2011 at 08:15:42AM +0200, Kai Tietz wrote:
>> >> Hmm, not sure.
>> >
>> > Well, I'm 99% sure. :-)  rawsize on an output section, if non-zero, is
>> > just a stale size at bfd_final_link.
>>
>> So this 1% hits. I changed locally to use here just sec->size and I
>> found that pdata section doesn't get sorted proper anymore. (you can
>> verify this by objdump -x and it prints warnings about not ascending
>> data).
>
> Ah, what I missed seeing is that coff_compute_section_file_positions
> is bumping the section size here:
>
> #ifdef COFF_IMAGE_WITH_PE
>      /* Set the padded size.  */
>      current->size = (current->size + page_size -1) & -page_size;
> #endif
>
> Obviously, you do want the size of data before this padding is added,
> but it's only a fluke that rawsize happens to be set correctly.  (You
> get it from the lang_reset_memory_regions call during preliminary
> section sizing in ldlang.c:strip_excluded_output_sections.)  That
> seems a little unreliable to me.  I'd be happier if in
> coff_compute_section_file_positions you always set rawsize in the loop
> that is padding section size (do it before any block of code that
> changes section size!).  Then just use sec->rawsize in your peXXigen.c
> patch.  I'll preapprove those changes.
>
> --
> Alan Modra
> Australia Development Lab, IBM
>

Ok, AFAICS it is enough here to set rawsize at one place.  Just for
bss-section we don't want to set rawsize.

ChangeLog

2011-04-09  Kai Tietz

       * peXXigen.c (_bfd_XXi_final_link_postscripte): Sort pdata in temporary
       buffer and use rawsize for sorting.
       * coffcode.h (coff_compute_section_file_positions): Set rawsize
before doing alignment.

Tested for x86_64-w64-mingw32. Ok for apply?

Regards,
Kai

[-- Attachment #2: pdata_x64_sort.txt --]
[-- Type: text/plain, Size: 2065 bytes --]

Index: src/bfd/peXXigen.c
===================================================================
--- src.orig/bfd/peXXigen.c	2011-04-08 21:08:20.230411500 +0200
+++ src/bfd/peXXigen.c	2011-04-09 11:46:00.611507900 +0200
@@ -2458,15 +2458,23 @@ _bfd_XXi_final_link_postscript (bfd * ab
 
     if (sec)
       {
-	bfd_size_type x = sec->rawsize ? sec->rawsize : sec->size;
+	bfd_size_type x = sec->rawsize;
+	bfd_byte *tmp_data = NULL;
 
-	if (x && bfd_get_section_contents (abfd, sec, pfinfo->contents, 0, x))
+	if (x)
+	  tmp_data = bfd_malloc (x);
+
+	if (tmp_data != NULL)
 	  {
-	    qsort (pfinfo->contents,
-	    	   (size_t) ((sec->size <x ? sec->size : x) / 12),
-	    	   12, sort_x64_pdata);
-	    bfd_set_section_contents (pfinfo->output_bfd, sec,
-	    			      pfinfo->contents, 0, x);
+	    if (bfd_get_section_contents (abfd, sec, tmp_data, 0, x))
+	      {
+		qsort (tmp_data,
+		       (size_t) (x / 12),
+		       12, sort_x64_pdata);
+		bfd_set_section_contents (pfinfo->output_bfd, sec,
+					  tmp_data, 0, x);
+	      }
+	    free (tmp_data);
 	  }
       }
   }
Index: src/bfd/coffcode.h
===================================================================
--- src.orig/bfd/coffcode.h	2011-04-09 10:29:19.000000000 +0200
+++ src/bfd/coffcode.h	2011-04-09 11:46:45.938100500 +0200
@@ -3297,6 +3297,11 @@ coff_compute_section_file_positions (bfd
       if (!(current->flags & SEC_HAS_CONTENTS))
 	continue;
 
+      /* Set rawsize for each section before we are doing alignment.  But
+         don't set rawsize for BSS section.  */
+      if (strcmp (current->name, _BSS) != 0)
+        current->rawsize = current->size;
+
 #ifdef COFF_IMAGE_WITH_PE
       /* Make sure we skip empty sections in a PE image.  */
       if (current->size == 0)
@@ -3363,7 +3368,7 @@ coff_compute_section_file_positions (bfd
 
 #ifdef COFF_IMAGE_WITH_PE
       /* Set the padded size.  */
-      current->size = (current->size + page_size -1) & -page_size;
+      current->size = (current->size + page_size - 1) & -page_size;
 #endif
 
       sofar += current->size;