PATCH: Check number of sections overflow

PATCH: Check number of sections overflow

[H.J. Lu]

Hi,

SHN_LORESERVE is defined as

#define SHN_LORESERVE	0xFF00

externally and defined as

#define SHN_LORESERVE	(-0x100u)

internally.  It may overflow when number of sections >= (-0x100u).  It
is very unlikely to happen sine -0x100u is 0xffffff00.  This patch
adds a check just in case.  OK to install?

Thanks.


H.J.
---
2008-03-12  H.J. Lu  <hongjiu.lu@intel.com>

	* elf.c (assign_section_numbers): Check if number of sections
	>= SHN_LORESERVE.
	* elfcode.h (elf_object_p): Likewise.

--- bfd/elf.c.64k	2008-03-12 12:32:53.000000000 -0700
+++ bfd/elf.c	2008-03-12 14:06:17.000000000 -0700
@@ -2831,6 +2831,13 @@ assign_section_numbers (bfd *abfd, struc
       _bfd_elf_strtab_addref (elf_shstrtab (abfd), t->strtab_hdr.sh_name);
     }
 
+  if (section_number >= SHN_LORESERVE)
+    {
+      _bfd_error_handler (_("%B: too many sections: %u"),
+			  abfd, section_number);
+      return FALSE;
+    }
+
   _bfd_elf_strtab_finalize (elf_shstrtab (abfd));
   t->shstrtab_hdr.sh_size = _bfd_elf_strtab_size (elf_shstrtab (abfd));
 
--- bfd/elfcode.h.64k	2008-03-12 12:32:05.000000000 -0700
+++ bfd/elfcode.h	2008-03-12 15:30:51.000000000 -0700
@@ -684,8 +684,14 @@ elf_object_p (bfd *abfd)
       if (i_ehdrp->e_shnum == SHN_UNDEF)
 	{
 	  i_ehdrp->e_shnum = i_shdr.sh_size;
-	  if (i_ehdrp->e_shnum != i_shdr.sh_size
-	      || i_ehdrp->e_shnum == 0)
+	  if (i_ehdrp->e_shnum >= SHN_LORESERVE)
+	    {
+	      _bfd_error_handler (_("%B: too many sections: %u"),
+				  abfd, i_ehdrp->e_shnum);
+	      abort ();
+	    }
+	  else if (i_ehdrp->e_shnum != i_shdr.sh_size
+		   || i_ehdrp->e_shnum  == 0)
 	    goto got_wrong_format_error;
 	}
 

Re: PATCH: Check number of sections overflow

[Alan Modra]

On Tue, Jul 03, 2012 at 11:36:21AM -0700, H.J. Lu wrote:
> 2008-03-12  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	* elf.c (assign_section_numbers): Check if number of sections
> 	>= SHN_LORESERVE.
> 	* elfcode.h (elf_object_p): Likewise.

OK, but

> --- bfd/elfcode.h.64k	2008-03-12 12:32:05.000000000 -0700
> +++ bfd/elfcode.h	2008-03-12 15:30:51.000000000 -0700
> @@ -684,8 +684,14 @@ elf_object_p (bfd *abfd)
>        if (i_ehdrp->e_shnum == SHN_UNDEF)
>  	{
>  	  i_ehdrp->e_shnum = i_shdr.sh_size;
> -	  if (i_ehdrp->e_shnum != i_shdr.sh_size
> -	      || i_ehdrp->e_shnum == 0)
> +	  if (i_ehdrp->e_shnum >= SHN_LORESERVE)
> +	    {
> +	      _bfd_error_handler (_("%B: too many sections: %u"),
> +				  abfd, i_ehdrp->e_shnum);
> +	      abort ();
> +	    }
> +	  else if (i_ehdrp->e_shnum != i_shdr.sh_size
> +		   || i_ehdrp->e_shnum  == 0)
>  	    goto got_wrong_format_error;
>  	}
>  

this should not abort, and you won't ever hit the error message except
on a corrupted file, so I'd prefer the extra test just
goto got_wrong_format_error

Testcase?  Grins.  Just kidding.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: Check number of sections overflow

[H.J. Lu]

On Tue, Jul 3, 2012 at 5:06 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Jul 03, 2012 at 11:36:21AM -0700, H.J. Lu wrote:
>> 2008-03-12  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>       * elf.c (assign_section_numbers): Check if number of sections
>>       >= SHN_LORESERVE.
>>       * elfcode.h (elf_object_p): Likewise.
>
> OK, but
>
>> --- bfd/elfcode.h.64k 2008-03-12 12:32:05.000000000 -0700
>> +++ bfd/elfcode.h     2008-03-12 15:30:51.000000000 -0700
>> @@ -684,8 +684,14 @@ elf_object_p (bfd *abfd)
>>        if (i_ehdrp->e_shnum == SHN_UNDEF)
>>       {
>>         i_ehdrp->e_shnum = i_shdr.sh_size;
>> -       if (i_ehdrp->e_shnum != i_shdr.sh_size
>> -           || i_ehdrp->e_shnum == 0)
>> +       if (i_ehdrp->e_shnum >= SHN_LORESERVE)
>> +         {
>> +           _bfd_error_handler (_("%B: too many sections: %u"),
>> +                               abfd, i_ehdrp->e_shnum);
>> +           abort ();
>> +         }
>> +       else if (i_ehdrp->e_shnum != i_shdr.sh_size
>> +                || i_ehdrp->e_shnum  == 0)
>>           goto got_wrong_format_error;
>>       }
>>
>
> this should not abort, and you won't ever hit the error message except
> on a corrupted file, so I'd prefer the extra test just
> goto got_wrong_format_error
>
> Testcase?  Grins.  Just kidding.
>

This is what I checked in with the same ChangeLog entry.

Thanks.

-- 
H.J.
---
diff --git a/bfd/elf.c b/bfd/elf.c
index 532c7f9..48e5d68 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -3014,6 +3014,13 @@ assign_section_numbers (bfd *abfd, struct
bfd_link_info *link_info)
       _bfd_elf_strtab_addref (elf_shstrtab (abfd), t->strtab_hdr.sh_name);
     }

+  if (section_number >= SHN_LORESERVE)
+    {
+      _bfd_error_handler (_("%B: too many sections: %u"),
+			  abfd, section_number);
+      return FALSE;
+    }
+
   _bfd_elf_strtab_finalize (elf_shstrtab (abfd));
   t->shstrtab_hdr.sh_size = _bfd_elf_strtab_size (elf_shstrtab (abfd));

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index cc55c86..30bda73 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -633,8 +633,9 @@ elf_object_p (bfd *abfd)
       if (i_ehdrp->e_shnum == SHN_UNDEF)
 	{
 	  i_ehdrp->e_shnum = i_shdr.sh_size;
-	  if (i_ehdrp->e_shnum != i_shdr.sh_size
-	      || i_ehdrp->e_shnum == 0)
+	  if (i_ehdrp->e_shnum >= SHN_LORESERVE
+	      || i_ehdrp->e_shnum != i_shdr.sh_size
+	      || i_ehdrp->e_shnum  == 0)
 	    goto got_wrong_format_error;
 	}