This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: two patches for bugs in BFD/peXXigen.c


On Fri, Sep 03, 2010 at 01:24:34AM +0200, Marcus Brinkmann wrote:
> Hi,
> 
> while working on free software ports to Windows CE I noticed two bugs in
> binutils' BFD support for some PE files (for example, kmail-mobile.exe built
> with MSVC).  Fixes for both are included below.  Copyright assignments by g10
> Code GmbH are on file at the FSF.  If you need anything else, just let me
> know.  The file that triggers these bugs can be found at:
> 
> ftp://ftp.g10code.com/people/marcus/kmail-mobile-binutils-test.exe.gz
> 
> The first issue concerns import tables where the thunk table is found in a
> different section.  In this case, BFD tries to load DATASIZE bytes from that
> section at the beginning of the thunk array, but DATASIZE is the remaining
> bytes in the import table section starting from the beginning of the import
> table.  This number is in no way related to the size of the thunk table or the
> section in which this thunk table is to be found.  So, my patch introduces a
> new size, limit_size, which is correctly calculated and used in the
> appropriate places.  Without the patch, no import symbols would be shown for
> kmail-mobile.exe (Visual Studio 2008, Windows CE), because the data section is
> in this case not large enough to read DATASIZE bytes from it.  With the patch,
> loading LIMIT_SIZE bytes succeeds and all import symbols are shown correctly.
> 
> The second issue concerns the support for compressed pdata support for Windows
> CE.  In this code is a simple memory leak.  First, the whole section is
> malloced and copied to TDATA, then immediately TDATA is overwritten with a
> much smaller buffer to which only the required section data is copied, leaking
> memory in the size of the section for each entry in the table.  For
> kmail-mobile.exe, the table is very large (hundreds of entries), leaking
> Gigabytes of memory quickly and basically creating denial of service attack.
[snip]
> 2010-09-03  Marcus Brinkmann  <marcus@g10code.de>
> 
> 	* peXXigen.c (pe_print_idata): Use correct size limit to load
>           thunk table from different section.
> 
[snip]
> 2010-09-03  Marcus Brinkmann  <marcus@g10code.de>
> 
> 	* peXXigen.c (_bfd_XX_print_ce_compressed_pdata): Fix memory leak.
> 

Thank you for reporting these problems, but both of your patches
contain errors.  The first one results in
peigen.c: In function âpe_print_idataâ:
peigen.c:1229: error: âlimit_sizeâ may be used uninitialized in this function
The second results in
peigen.c: In function â_bfd_pe_print_ce_compressed_pdataâ:
peigen.c:1876: error: expected expression before âsym_cacheâ

In your first patch I think this hunk, and the following one, is
incorrect
@@ -1285,7 +1288,7 @@ pe_print_idata (bfd * abfd, void * vfile
 
 	  /* Print HintName vector entries.  */
 #ifdef COFF_WITH_pex64
-	  for (j = 0; j < datasize; j += 8)
+	  for (j = 0; j < limit_size; j += 8)
 	    {
 	      unsigned long member = bfd_get_32 (abfd, data + idx + j);
 	      unsigned long member_high = bfd_get_32 (abfd, data + idx + j + 4);

Won't changing the loop endpoint possibly affect reading "member"?

In the second patch, you still leave a potential memory leak if tdata
is allocated but bfd_get_section_contents returns an error.

The following is a revision of your patch with these problems fixed,
but even though I'm a blanket write binutils maintainer I probably
shouldn't be let loose in peXXigen.c!  So I'll leave this for one of
the PE/COFF maintainers to handle.

Index: bfd/peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.64
diff -u -p -r1.64 peXXigen.c
--- bfd/peXXigen.c	27 Jun 2010 04:07:53 -0000	1.64
+++ bfd/peXXigen.c	6 Sep 2010 04:39:44 -0000
@@ -550,7 +550,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, v
   PEAOUTHDR *aouthdr_out = (PEAOUTHDR *) out;
   bfd_vma sa, fa, ib;
   IMAGE_DATA_DIRECTORY idata2, idata5, tls;
-  
+
   sa = extra->SectionAlignment;
   fa = extra->FileAlignment;
   ib = extra->ImageBase;
@@ -558,7 +558,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, v
   idata2 = pe->pe_opthdr.DataDirectory[PE_IMPORT_TABLE];
   idata5 = pe->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE];
   tls = pe->pe_opthdr.DataDirectory[PE_TLS_TABLE];
-  
+
   if (aouthdr_in->tsize)
     {
       aouthdr_in->text_start -= ib;
@@ -615,7 +615,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, v
     /* Until other .idata fixes are made (pending patch), the entry for
        .idata is needed for backwards compatibility.  FIXME.  */
     add_data_entry (abfd, extra, 1, ".idata", ib);
-    
+
   /* For some reason, the virtual size (which is what's set by
      add_data_entry) for .reloc is not the same as the size recorded
      in this slot by MSVC; it doesn't seem to cause problems (so far),
@@ -926,7 +926,7 @@ _bfd_XXi_swap_scnhdr_out (bfd * abfd, vo
        (0x02000000).  Also, the resource data should also be read and
        writable.  */
 
-    /* FIXME: Alignment is also encoded in this field, at least on PPC and 
+    /* FIXME: Alignment is also encoded in this field, at least on PPC and
        ARM-WINCE.  Although - how do we get the original alignment field
        back ?  */
 
@@ -936,7 +936,7 @@ _bfd_XXi_swap_scnhdr_out (bfd * abfd, vo
       unsigned long	must_have;
     }
     pe_required_section_flags;
-    
+
     pe_required_section_flags known_sections [] =
       {
 	{ ".arch",  IMAGE_SCN_MEM_READ | IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_DISCARDABLE | IMAGE_SCN_ALIGN_8BYTES },
@@ -1223,18 +1223,19 @@ pe_print_idata (bfd * abfd, void * vfile
 	  bfd_byte *ft_data;
 	  asection *ft_section;
 	  bfd_vma ft_addr;
-	  bfd_size_type ft_datasize;
+	  bfd_size_type ft_datasize = 0;
 	  int ft_idx;
 	  int ft_allocated = 0;
 
 	  fprintf (file, _("\tvma:  Hint/Ord Member-Name Bound-To\n"));
 
 	  idx = hint_addr - adj;
-	  
+
 	  ft_addr = first_thunk + extra->ImageBase;
 	  ft_data = data;
 	  ft_idx = first_thunk - adj;
-	  ft_allocated = 0; 
+	  ft_datasize = section->size - ft_idx;
+	  ft_allocated = 0;
 
 	  if (first_thunk != hint_addr)
 	    {
@@ -1242,12 +1243,9 @@ pe_print_idata (bfd * abfd, void * vfile
 	      for (ft_section = abfd->sections;
 		   ft_section != NULL;
 		   ft_section = ft_section->next)
-		{
-		  ft_datasize = ft_section->size;
-		  if (ft_addr >= ft_section->vma
-		      && ft_addr < ft_section->vma + ft_datasize)
-		    break;
-		}
+		if (ft_addr >= ft_section->vma
+		    && ft_addr < ft_section->vma + ft_section->size)
+		  break;
 
 	      if (ft_section == NULL)
 		{
@@ -1258,21 +1256,17 @@ pe_print_idata (bfd * abfd, void * vfile
 
 	      /* Now check to see if this section is the same as our current
 		 section.  If it is not then we will have to load its data in.  */
-	      if (ft_section == section)
-		{
-		  ft_data = data;
-		  ft_idx = first_thunk - adj;
-		}
-	      else
+	      if (ft_section != section)
 		{
 		  ft_idx = first_thunk - (ft_section->vma - extra->ImageBase);
-		  ft_data = (bfd_byte *) bfd_malloc (datasize);
+		  ft_datasize = ft_section->size - ft_idx;
+		  ft_data = (bfd_byte *) bfd_malloc (ft_datasize);
 		  if (ft_data == NULL)
 		    continue;
 
-		  /* Read datasize bfd_bytes starting at offset ft_idx.  */
-		  if (! bfd_get_section_contents
-		      (abfd, ft_section, ft_data, (bfd_vma) ft_idx, datasize))
+		  /* Read ft_datasize bytes starting at offset ft_idx.  */
+		  if (!bfd_get_section_contents (abfd, ft_section, ft_data,
+						 (bfd_vma) ft_idx, ft_datasize))
 		    {
 		      free (ft_data);
 		      continue;
@@ -1310,7 +1304,8 @@ pe_print_idata (bfd * abfd, void * vfile
 		 table holds actual addresses.  */
 	      if (time_stamp != 0
 		  && first_thunk != 0
-		  && first_thunk != hint_addr)
+		  && first_thunk != hint_addr
+		  && j + 4 <= ft_datasize)
 		fprintf (file, "\t%04lx",
 			 (unsigned long) bfd_get_32 (abfd, ft_data + ft_idx + j));
 	      fprintf (file, "\n");
@@ -1320,7 +1315,7 @@ pe_print_idata (bfd * abfd, void * vfile
 	    {
 	      unsigned long member = bfd_get_32 (abfd, data + idx + j);
 
-	      /* Print single IMAGE_IMPORT_BY_NAME vector.  */ 
+	      /* Print single IMAGE_IMPORT_BY_NAME vector.  */
 	      if (member == 0)
 		break;
 
@@ -1342,7 +1337,8 @@ pe_print_idata (bfd * abfd, void * vfile
 		 table holds actual addresses.  */
 	      if (time_stamp != 0
 		  && first_thunk != 0
-		  && first_thunk != hint_addr)
+		  && first_thunk != hint_addr
+		  && j + 4 <= ft_datasize)
 		fprintf (file, "\t%04lx",
 			 (unsigned long) bfd_get_32 (abfd, ft_data + ft_idx + j));
 
@@ -1583,7 +1579,7 @@ pe_print_edata (bfd * abfd, void * vfile
 /* This really is architecture dependent.  On IA-64, a .pdata entry
    consists of three dwords containing relative virtual addresses that
    specify the start and end address of the code range the entry
-   covers and the address of the corresponding unwind info data. 
+   covers and the address of the corresponding unwind info data.
 
    On ARM and SH-4, a compressed PDATA structure is used :
    _IMAGE_CE_RUNTIME_FUNCTION_ENTRY, whereas MIPS is documented to use
@@ -1828,7 +1824,6 @@ _bfd_XX_print_ce_compressed_pdata (bfd *
       bfd_vma other_data;
       bfd_vma prolog_length, function_length;
       int flag32bit, exception_flag;
-      bfd_byte *tdata = 0;
       asection *tsection;
 
       if (i + PDATA_ROW_SIZE > stop)
@@ -1860,12 +1855,14 @@ _bfd_XX_print_ce_compressed_pdata (bfd *
       if (tsection && coff_section_data (abfd, tsection)
 	  && pei_section_data (abfd, tsection))
 	{
-	  if (bfd_malloc_and_get_section (abfd, tsection, & tdata))
-	    {
-	      int xx = (begin_addr - 8) - tsection->vma;
+	  int xx = (begin_addr - 8) - tsection->vma;
+	  bfd_byte *tdata;
 
-	      tdata = (bfd_byte *) bfd_malloc (8);
-	      if (bfd_get_section_contents (abfd, tsection, tdata, (bfd_vma) xx, 8))
+	  tdata = (bfd_byte *) bfd_malloc (8);
+	  if (tdata)
+	    {
+	      if (bfd_get_section_contents (abfd, tsection,
+					    tdata, (bfd_vma) xx, 8))
 		{
 		  bfd_vma eh, eh_data;
 
@@ -1883,11 +1880,6 @@ _bfd_XX_print_ce_compressed_pdata (bfd *
 		}
 	      free (tdata);
 	    }
-	  else
-	    {
-	      if (tdata)
-		free (tdata);
-	    }
 	}
 
       fprintf (file, "\n");
@@ -2194,7 +2186,7 @@ _bfd_XX_bfd_copy_private_bfd_data_common
 
   ipe = pe_data (ibfd);
   ope = pe_data (obfd);
- 
+
   /* pe_opthdr is copied in copy_object.  */
   ope->dll = ipe->dll;
 
@@ -2288,7 +2280,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
 			      ".idata$2", FALSE, FALSE, TRUE);
   if (h1 != NULL)
     {
-      /* PR ld/2729: We cannot rely upon all the output sections having been 
+      /* PR ld/2729: We cannot rely upon all the output sections having been
 	 created properly, so check before referencing them.  Issue a warning
 	 message for any sections tht could not be found.  */
       if ((h1->root.type == bfd_link_hash_defined
@@ -2302,7 +2294,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
       else
 	{
 	  _bfd_error_handler
-	    (_("%B: unable to fill in DataDictionary[1] because .idata$2 is missing"), 
+	    (_("%B: unable to fill in DataDictionary[1] because .idata$2 is missing"),
 	     abfd);
 	  result = FALSE;
 	}
@@ -2322,7 +2314,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
       else
 	{
 	  _bfd_error_handler
-	    (_("%B: unable to fill in DataDictionary[1] because .idata$4 is missing"), 
+	    (_("%B: unable to fill in DataDictionary[1] because .idata$4 is missing"),
 	     abfd);
 	  result = FALSE;
 	}
@@ -2343,7 +2335,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
       else
 	{
 	  _bfd_error_handler
-	    (_("%B: unable to fill in DataDictionary[12] because .idata$5 is missing"), 
+	    (_("%B: unable to fill in DataDictionary[12] because .idata$5 is missing"),
 	     abfd);
 	  result = FALSE;
 	}
@@ -2359,11 +2351,11 @@ _bfd_XXi_final_link_postscript (bfd * ab
 	  ((h1->root.u.def.value
 	    + h1->root.u.def.section->output_section->vma
 	    + h1->root.u.def.section->output_offset)
-	   - pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE].VirtualAddress);      
+	   - pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE].VirtualAddress);
       else
 	{
 	  _bfd_error_handler
-	    (_("%B: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE (12)] because .idata$6 is missing"), 
+	    (_("%B: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE (12)] because .idata$6 is missing"),
 	     abfd);
 	  result = FALSE;
 	}
@@ -2385,7 +2377,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
       else
 	{
 	  _bfd_error_handler
-	    (_("%B: unable to fill in DataDictionary[9] because __tls_used is missing"), 
+	    (_("%B: unable to fill in DataDictionary[9] because __tls_used is missing"),
 	     abfd);
 	  result = FALSE;
 	}

-- 
Alan Modra
Australia Development Lab, IBM


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]