[v3] PE/COFF: raise normal PE section limit safely

Message ID 20260705220703.1725-2-oleg.tolmatcev@gmail.com
State New
Headers
Series [v3] PE/COFF: raise normal PE section limit safely |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed

Commit Message

Oleg Tolmatcev July 5, 2026, 10:07 p.m. UTC
  PE/COFF stores symbol section numbers in a 16-bit field.  Binutils used
signed 16-bit handling there, which limited normal PE objects to 32767
sections even though MSVC and Clang already accept a larger unsigned
range.

Raise the normal PE section limit to 65279, while keeping the PE/COFF
special section-number values for undefined, absolute and debug symbols
working correctly.  Do this by decoding and encoding normal PE symbol
section numbers as unsigned values in the ordinary range, while still
mapping the named reserved constants explicitly.

Also reject the remaining reserved PE symbol section-number values as
malformed input when reading symbols, instead of only stumbling over
them later in writer-side assertions.

Add a gas test that exercises a normal PE object above the old
32767-section limit and checks that objdump reports the high section
number correctly, and add a binutils test that corrupts a PE symbol
section number to verify that invalid reserved values are rejected.

bfd/ChangeLog:

        * coffcode.h (COFF_DEFAULT_MAX_NSCNS): Define.
        (bfd_coff_std_swap_table): Use it for the default maximum section
        count.
        (ticoff0_swap_table): Likewise.
        (ticoff1_swap_table): Likewise.
        (fill_comdat_hash): Fail on invalid PE symbol section numbers.
        (coff_set_arch_mach_hook): Likewise.
        * coffgen.c (coff_get_normalized_symtab): Likewise.
        * cofflink.c (coff_link_add_symbols): Likewise.
        (_bfd_coff_link_input_bfd): Likewise.
        * peXXigen.c (pe_decode_sym_section_number): New function.
        (pe_encode_sym_section_number): New function.
        (_bfd_XXi_swap_sym_in): Use pe_decode_sym_section_number.
        (_bfd_XXi_swap_sym_out): Use pe_encode_sym_section_number.

include/ChangeLog:

        * coff/pe.h (IMAGE_SYM_UNDEFINED): Define.
        (IMAGE_SYM_ABSOLUTE): Define.
        (IMAGE_SYM_DEBUG): Define.
        (IMAGE_SYM_SECTION_MAX): Define.

binutils/ChangeLog:

        * testsuite/binutils-all/objcopy.exp
        (objcopy_test_invalid_pe_symbol_section_number): New test.

gas/ChangeLog:

        * testsuite/gas/pe/pe.exp: Run large-obj-normal.
        * testsuite/gas/pe/large-obj-normal.s: New test.
        * testsuite/gas/pe/large-obj-normal.d: New test.

Signed-off-by: Oleg Tolmatcev <oleg.tolmatcev@gmail.com>
---
 bfd/coffcode.h                              | 19 ++++-
 bfd/coffgen.c                               |  2 +
 bfd/cofflink.c                              |  8 +++
 bfd/peXXigen.c                              | 65 ++++++++++++++++-
 binutils/testsuite/binutils-all/objcopy.exp | 77 +++++++++++++++++++++
 gas/testsuite/gas/pe/large-obj-normal.d     | 17 +++++
 gas/testsuite/gas/pe/large-obj-normal.s     | 16 +++++
 gas/testsuite/gas/pe/pe.exp                 |  1 +
 include/coff/pe.h                           |  5 ++
 9 files changed, 205 insertions(+), 5 deletions(-)
 create mode 100644 gas/testsuite/gas/pe/large-obj-normal.d
 create mode 100644 gas/testsuite/gas/pe/large-obj-normal.s
  

Comments

Jan Beulich July 10, 2026, 1:03 p.m. UTC | #1
On 06.07.2026 00:07, Oleg Tolmatcev wrote:
> @@ -927,6 +933,8 @@ fill_comdat_hash (bfd *abfd)
>        flagword sec_flags = SEC_LINK_ONCE;
>  
>        bfd_coff_swap_sym_in (abfd, esym, &isym);
> +      if (bfd_get_error () == bfd_error_bad_value)
> +	return false;

Taking this as example (applies at all use sites): I'm not sure what to say.
What if bfd_coff_swap_sym_in() worked fine and something else left an error
latched in libbfd? This kind of indirect error checking feels pretty
fragile. If already all use sites of bfd_coff_swap_sym_in() need touching
anyway, can't the function be changed to properly report a success indicator?

Having said "all" - you don't really amend all call sites, do you? The
description says nothing as to how the selection was made.

Jan
  
Oleg Tolmatcev July 10, 2026, 1:14 p.m. UTC | #2
Am Fr., 10. Juli 2026 um 15:03 Uhr schrieb Jan Beulich <jbeulich@suse.com>:
>
> On 06.07.2026 00:07, Oleg Tolmatcev wrote:
> > @@ -927,6 +933,8 @@ fill_comdat_hash (bfd *abfd)
> >        flagword sec_flags = SEC_LINK_ONCE;
> >
> >        bfd_coff_swap_sym_in (abfd, esym, &isym);
> > +      if (bfd_get_error () == bfd_error_bad_value)
> > +     return false;
>
> Taking this as example (applies at all use sites): I'm not sure what to say.
> What if bfd_coff_swap_sym_in() worked fine and something else left an error
> latched in libbfd? This kind of indirect error checking feels pretty
> fragile. If already all use sites of bfd_coff_swap_sym_in() need touching
> anyway, can't the function be changed to properly report a success indicator?
>
> Having said "all" - you don't really amend all call sites, do you? The
> description says nothing as to how the selection was made.

I wanted to keep the changes to error handling minimal, but now I think
that I shouldn't have touched it at all. The original code did not do any
error handling either. If the only thing wrong with v1 of the patch
was the assert,
should I just remove the assert from it and be done with the patch? Otherwise
I don't know what to do.

Oleg
  
Jan Beulich July 10, 2026, 1:24 p.m. UTC | #3
On 10.07.2026 15:14, Oleg Tolmatcev wrote:
> Am Fr., 10. Juli 2026 um 15:03 Uhr schrieb Jan Beulich <jbeulich@suse.com>:
>>
>> On 06.07.2026 00:07, Oleg Tolmatcev wrote:
>>> @@ -927,6 +933,8 @@ fill_comdat_hash (bfd *abfd)
>>>        flagword sec_flags = SEC_LINK_ONCE;
>>>
>>>        bfd_coff_swap_sym_in (abfd, esym, &isym);
>>> +      if (bfd_get_error () == bfd_error_bad_value)
>>> +     return false;
>>
>> Taking this as example (applies at all use sites): I'm not sure what to say.
>> What if bfd_coff_swap_sym_in() worked fine and something else left an error
>> latched in libbfd? This kind of indirect error checking feels pretty
>> fragile. If already all use sites of bfd_coff_swap_sym_in() need touching
>> anyway, can't the function be changed to properly report a success indicator?
>>
>> Having said "all" - you don't really amend all call sites, do you? The
>> description says nothing as to how the selection was made.
> 
> I wanted to keep the changes to error handling minimal, but now I think
> that I shouldn't have touched it at all. The original code did not do any
> error handling either. If the only thing wrong with v1 of the patch
> was the assert,
> should I just remove the assert from it and be done with the patch?

I think I could live with that.

> Otherwise I don't know what to do.

Well, I did outline one option. Sooner or later proper error handling will
want adding anyway, imo. But yes, it doesn't need to be you who does it.

Jan
  

Patch

diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index 12cc7c3ca7..7353561407 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -376,6 +376,12 @@  extern const bfd_target TARGET_SYM_BIG;
 # define COFF_WITH_EXTENDED_RELOC_COUNTER
 #endif
 
+#if defined(COFF_WITH_PE) && !defined(COFF_IMAGE_WITH_PE)
+# define COFF_DEFAULT_MAX_NSCNS (IMAGE_SYM_SECTION_MAX + 1)
+#else
+# define COFF_DEFAULT_MAX_NSCNS 32768
+#endif
+
 #if defined (COFF_LONG_SECTION_NAMES)
 /* Needed to expand the inputs to BLANKOR1TOODD.  */
 #define COFFLONGSECTIONCATHELPER(x,y)    x ## y
@@ -927,6 +933,8 @@  fill_comdat_hash (bfd *abfd)
       flagword sec_flags = SEC_LINK_ONCE;
 
       bfd_coff_swap_sym_in (abfd, esym, &isym);
+      if (bfd_get_error () == bfd_error_bad_value)
+	return false;
 
       /* According to the MSVC documentation, the first TWO entries
 	 with the section # are both of interest to us.  The first one
@@ -2315,6 +2323,11 @@  coff_set_arch_mach_hook (bfd *abfd, void * filehdr)
 		if (buf == NULL)
 		  return false;
 		bfd_coff_swap_sym_in (abfd, buf, & sym);
+		if (bfd_get_error () == bfd_error_bad_value)
+		  {
+		    free (buf);
+		    return false;
+		  }
 		if (sym.n_sclass == C_FILE)
 		  cputype = sym.n_type & 0xff;
 		else
@@ -5615,7 +5628,7 @@  static const bfd_coff_backend_data bfd_coff_std_swap_table ATTRIBUTE_UNUSED =
 #else
   2,
 #endif
-  32768,
+  COFF_DEFAULT_MAX_NSCNS,
   coff_SWAP_filehdr_in, coff_SWAP_aouthdr_in, coff_SWAP_scnhdr_in,
   coff_SWAP_reloc_in, coff_bad_format_hook, coff_set_arch_mach_hook,
   coff_mkobject_hook, styp_to_sec_flags, coff_set_alignment_hook,
@@ -5656,7 +5669,7 @@  static const bfd_coff_backend_data ticoff0_swap_table =
 #else
   2,
 #endif
-  32768,
+  COFF_DEFAULT_MAX_NSCNS,
   coff_SWAP_filehdr_in, coff_SWAP_aouthdr_in, coff_SWAP_scnhdr_in,
   coff_swap_reloc_v0_in, ticoff0_bad_format_hook, coff_set_arch_mach_hook,
   coff_mkobject_hook, styp_to_sec_flags, coff_set_alignment_hook,
@@ -5698,7 +5711,7 @@  static const bfd_coff_backend_data ticoff1_swap_table =
 #else
   2,
 #endif
-  32768,
+  COFF_DEFAULT_MAX_NSCNS,
   coff_SWAP_filehdr_in, coff_SWAP_aouthdr_in, coff_SWAP_scnhdr_in,
   coff_SWAP_reloc_in, ticoff1_bad_format_hook, coff_set_arch_mach_hook,
   coff_mkobject_hook, styp_to_sec_flags, coff_set_alignment_hook,
diff --git a/bfd/coffgen.c b/bfd/coffgen.c
index f3060642a5..d2e2e905bc 100644
--- a/bfd/coffgen.c
+++ b/bfd/coffgen.c
@@ -1908,6 +1908,8 @@  coff_get_normalized_symtab (bfd *abfd)
 
       bfd_coff_swap_sym_in (abfd, (void *) raw_src,
 			    (void *) & internal_ptr->u.syment);
+      if (bfd_get_error () == bfd_error_bad_value)
+	return NULL;
       internal_ptr->is_sym = true;
       combined_entry_type *sym = internal_ptr;
 
diff --git a/bfd/cofflink.c b/bfd/cofflink.c
index e5c8a987d6..2de58c57bb 100644
--- a/bfd/cofflink.c
+++ b/bfd/cofflink.c
@@ -315,6 +315,8 @@  coff_link_add_symbols (bfd *abfd,
       bool copy;
 
       bfd_coff_swap_sym_in (abfd, esym, &sym);
+      if (bfd_get_error () == bfd_error_bad_value)
+	goto error_return;
 
       classification = bfd_coff_classify_symbol (abfd, &sym);
       if (classification != COFF_SYMBOL_LOCAL)
@@ -1485,6 +1487,8 @@  _bfd_coff_link_input_bfd (struct coff_final_link_info *flaginfo, bfd *input_bfd)
       int add;
 
       bfd_coff_swap_sym_in (input_bfd, esym, isymp);
+      if (bfd_get_error () == bfd_error_bad_value)
+	return false;
 
       /* Make a copy of *isymp so that the relocate_section function
 	 always sees the original values.  This is more reliable than
@@ -1684,6 +1688,8 @@  _bfd_coff_link_input_bfd (struct coff_final_link_info *flaginfo, bfd *input_bfd)
 	      char *name_copy;
 
 	      bfd_coff_swap_sym_in (input_bfd, esl, islp);
+	      if (bfd_get_error () == bfd_error_bad_value)
+		return false;
 
 	      amt = sizeof (struct coff_debug_merge_element);
 	      *epp = (struct coff_debug_merge_element *)
@@ -2278,6 +2284,8 @@  _bfd_coff_link_input_bfd (struct coff_final_link_info *flaginfo, bfd *input_bfd)
 					    (flaginfo->outsyms
 					     + ((indx - syment_base)
 						* osymesz)), &is);
+		      if (bfd_get_error () == bfd_error_bad_value)
+			return false;
 		      if ((ISFCN (is.n_type)
 			   || is.n_sclass == C_BLOCK)
 			  && is.n_numaux >= 1)
diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index bdb23dcbca..01f68052a7 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -109,12 +109,73 @@ 
 #define SetHighBit(val)      ((val) | 0x80000000)
 #define WithoutHighBit(val)  ((val) & 0x7fffffff)
 
+static bool
+pe_decode_sym_section_number (bfd *abfd, const char *raw_scnum, int *scnump)
+{
+  unsigned int scnum = H_GET_16 (abfd, raw_scnum);
+
+  switch (scnum)
+    {
+    case IMAGE_SYM_UNDEFINED:
+      *scnump = N_UNDEF;
+      return true;
+    case IMAGE_SYM_ABSOLUTE:
+      *scnump = N_ABS;
+      return true;
+    case IMAGE_SYM_DEBUG:
+      *scnump = N_DEBUG;
+      return true;
+    }
+
+  if (scnum > IMAGE_SYM_SECTION_MAX)
+    {
+      *scnump = N_UNDEF;
+      _bfd_error_handler
+	/* xgettext:c-format */
+	(_("%pB: unrecognized PE symbol section number %#x"), abfd, scnum);
+      if (!abfd->is_linker_input)
+	abfd->read_only = 1;
+      bfd_set_error (bfd_error_bad_value);
+      return true;
+    }
+
+  *scnump = scnum;
+  return true;
+}
+
+static void
+pe_encode_sym_section_number (bfd *abfd, int scnum, char *raw_scnum)
+{
+  unsigned int encoded_scnum;
+
+  switch (scnum)
+    {
+    case N_UNDEF:
+      encoded_scnum = IMAGE_SYM_UNDEFINED;
+      break;
+    case N_ABS:
+      encoded_scnum = IMAGE_SYM_ABSOLUTE;
+      break;
+    case N_DEBUG:
+      encoded_scnum = IMAGE_SYM_DEBUG;
+      break;
+    default:
+      BFD_ASSERT (scnum > 0 && (unsigned int) scnum <= IMAGE_SYM_SECTION_MAX);
+      encoded_scnum = scnum;
+      break;
+    }
+
+  H_PUT_16 (abfd, encoded_scnum, raw_scnum);
+}
+
 void
 _bfd_XXi_swap_sym_in (bfd * abfd, void * ext1, void * in1)
 {
   SYMENT *ext = (SYMENT *) ext1;
   struct internal_syment *in = (struct internal_syment *) in1;
 
+  memset (in, 0, sizeof (*in));
+
   if (ext->e.e_name[0] == 0)
     {
       in->_n._n_n._n_zeroes = 0;
@@ -124,7 +185,7 @@  _bfd_XXi_swap_sym_in (bfd * abfd, void * ext1, void * in1)
     memcpy (in->_n._n_name, ext->e.e_name, SYMNMLEN);
 
   in->n_value = H_GET_32 (abfd, ext->e_value);
-  in->n_scnum = (short) H_GET_16 (abfd, ext->e_scnum);
+  pe_decode_sym_section_number (abfd, ext->e_scnum, &in->n_scnum);
 
   if (sizeof (ext->e_type) == 2)
     in->n_type = H_GET_16 (abfd, ext->e_type);
@@ -262,7 +323,7 @@  _bfd_XXi_swap_sym_out (bfd * abfd, void * inp, void * extp)
     }
 
   H_PUT_32 (abfd, in->n_value, ext->e_value);
-  H_PUT_16 (abfd, in->n_scnum, ext->e_scnum);
+  pe_encode_sym_section_number (abfd, in->n_scnum, ext->e_scnum);
 
   if (sizeof (ext->e_type) == 2)
     H_PUT_16 (abfd, in->n_type, ext->e_type);
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index 25e2a44096..23648ff700 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1293,6 +1293,81 @@  proc objcopy_test_elf_common_symbols {} {
     }
 }
 
+proc objcopy_test_invalid_pe_symbol_section_number {} {
+    global OBJCOPY
+    global OBJCOPYFLAGS
+    global OBJDUMP
+    global OBJDUMPFLAGS
+    global srcdir
+    global subdir
+    global binutils_run_status
+
+    set test "objdump rejects unrecognized PE symbol section number"
+
+    if { ![is_pecoff_format] } {
+	return
+    }
+
+    if { [is_remote host] } {
+	untested $test
+	return
+    }
+
+    set objfile tmpdir/invalid-pe-scnum.o
+    if { ![binutils_assemble $srcdir/$subdir/bintest.s $objfile] } then {
+	unsupported $test
+	return
+    }
+
+    set file [open $objfile r+b]
+    fconfigure $file -translation binary
+    set data [read $file]
+
+    binary scan [string range $data 8 11] cu4 bytes
+    set symptr [expr {([lindex $bytes 0] << 0)
+		      | ([lindex $bytes 1] << 8)
+		      | ([lindex $bytes 2] << 16)
+		      | ([lindex $bytes 3] << 24)}]
+    set scnum_off [expr {$symptr + 12}]
+
+    set data [string replace $data $scnum_off [expr {$scnum_off + 1}] "\xFD\xFF"]
+
+    seek $file 0 start
+    puts -nonewline $file $data
+    close $file
+
+    set got [binutils_run $OBJDUMP "$OBJDUMPFLAGS -t $objfile"]
+    if { $binutils_run_status == 0 } {
+	fail $test
+	return
+    }
+
+    if { ![regexp "unrecognized PE symbol section number 0xfffd" $got] } {
+	send_log "$got\n"
+	fail $test
+	return
+    }
+
+    pass $test
+
+    set test "objcopy rejects unrecognized PE symbol section number"
+    set copy tmpdir/invalid-pe-scnum-copy.o
+    set got [binutils_run $OBJCOPY "$OBJCOPYFLAGS $objfile $copy"]
+    if { $binutils_run_status == 0 } {
+	send_log "$got\n"
+	fail $test
+	return
+    }
+
+    if { ![regexp {invalid-pe-scnum\.o: bad value} $got] } {
+	send_log "$got\n"
+	fail $test
+	return
+    }
+
+    pass $test
+}
+
 # ia64 specific tests
 if { ([istarget "ia64-*-elf*"]
        || [istarget "ia64-*-linux*"]) } {
@@ -1581,6 +1656,8 @@  if { ![is_xcoff_format] } {
 
 run_dump_test "rename-section-01"
 
+objcopy_test_invalid_pe_symbol_section_number
+
 proc objcopy_tek2bin {} {
     global OBJCOPY
     global OBJDUMP
diff --git a/gas/testsuite/gas/pe/large-obj-normal.d b/gas/testsuite/gas/pe/large-obj-normal.d
new file mode 100644
index 0000000000..e9009e5795
--- /dev/null
+++ b/gas/testsuite/gas/pe/large-obj-normal.d
@@ -0,0 +1,17 @@ 
+#objdump: -h -t
+#name: PE large normal object after raising the normal section limit
+
+# This only becomes meaningful once normal PE/COFF is allowed past the
+# old 32767-section cutoff.  Before that, BFD errors out or auto-promotes
+# to bigobj before the normal 16-bit symbol-section encoding is exercised.
+
+.*: *file format pe-(aarch64-little|i386|x86-64)
+
+Sections:
+#...
+40002 +\.data\$a39999  .*
+                  CONTENTS, ALLOC, LOAD, DATA
+
+SYMBOL TABLE:
+#...
+.*\(sec 40003\).*\(scl +2\).*\) 0x[0-9a-f]+ a39999$
diff --git a/gas/testsuite/gas/pe/large-obj-normal.s b/gas/testsuite/gas/pe/large-obj-normal.s
new file mode 100644
index 0000000000..254733023d
--- /dev/null
+++ b/gas/testsuite/gas/pe/large-obj-normal.s
@@ -0,0 +1,16 @@ 
+	.file "large-obj-normal.s"
+
+	.irp n,0,1,2,3
+	.irp m,0,1,2,3,4,5,6,7,8,9
+	.irp c,0,1,2,3,4,5,6,7,8,9
+	.irp d,0,1,2,3,4,5,6,7,8,9
+	.irp u,0,1,2,3,4,5,6,7,8,9
+	.globl a\n\m\c\d\u
+	.section .data$a\n\m\c\d\u,"w"
+a\n\m\c\d\u :
+	.byte 1
+	.endr
+	.endr
+	.endr
+	.endr
+	.endr
diff --git a/gas/testsuite/gas/pe/pe.exp b/gas/testsuite/gas/pe/pe.exp
index 82f217cf94..600fd2425a 100644
--- a/gas/testsuite/gas/pe/pe.exp
+++ b/gas/testsuite/gas/pe/pe.exp
@@ -71,6 +71,7 @@  if {[istarget "aarch64-*-pe*"] || [istarget "aarch64-*-mingw*"]} {
 if { [istarget "aarch64-*-*"]
      || ([istarget "i*86-*-*"] && ![istarget "*-*-interix*"])
      || [istarget "x86_64-*-*"] } then {
+	run_dump_test "large-obj-normal"
 	run_dump_test "big-obj"
 	run_dump_test "big-obj-auto"
 }
diff --git a/include/coff/pe.h b/include/coff/pe.h
index ece4bf25db..133548ff4b 100644
--- a/include/coff/pe.h
+++ b/include/coff/pe.h
@@ -388,6 +388,11 @@  struct external_SYMBOL_EX
   char e_numaux[1];
 } ATTRIBUTE_PACKED ;
 
+#define IMAGE_SYM_UNDEFINED      0
+#define IMAGE_SYM_ABSOLUTE       0xffff
+#define IMAGE_SYM_DEBUG          0xfffe
+#define IMAGE_SYM_SECTION_MAX    0xfeff
+
 #define	SYMENT_BIGOBJ	struct external_SYMBOL_EX
 #define	SYMESZ_BIGOBJ	20