PR28055, segfault in bpf special reloc function

PR28055, segfault in bpf special reloc function

[Alan Modra]

The testcase in this PR tickled two bugs fixed here.  output_bfd is
NULL when a reloc special_function is called for final linking and
when called from bfd_generic_get_relocated_section_contents.  Clearly
using output_bfd is wrong as it results in segfaults.  Not only that,
the endianness of the reloc field really should be that of the input.
The second bug was not checking that the entire reloc field was
contained in the section contents.

I was going to add the following too, but it looks like this would
only be dead code since ld -r isn't supported in combination of input
and output that would see this special_function called.

  /* If this is a relocatable link (output_bfd test tells us), just
     call the generic function.  */
  if (abfd != output_bfd && output_bfd != NULL)
    return bfd_elf_generic_reloc (abfd, reloc_entry, symbol, data,
				  input_section, output_bfd, error_message);


	PR 28055
	* elf64-bpf.c (bpf_elf_generic_reloc): Use correct bfd for bfd_put
	and bfd_put_32 calls.  Correct section limit checks.

diff --git a/bfd/elf64-bpf.c b/bfd/elf64-bpf.c
index 28c1543c0b6..243df93ae82 100644
--- a/bfd/elf64-bpf.c
+++ b/bfd/elf64-bpf.c
@@ -589,14 +589,15 @@ elf64_bpf_merge_private_bfd_data (bfd *ibfd, struct bfd_link_info *info)
 }
 
 /* A generic howto special function for installing BPF relocations.
-   This function will be called by the assembler (via bfd_install_relocation).
+   This function will be called by the assembler (via bfd_install_relocation),
+   and by various get_relocated_section_contents functions.
    At link time, bpf_elf_relocate_section will resolve the final relocations.
 
    BPF instructions are always big endian, and this approach avoids problems in
    bfd_install_relocation.  */
 
 static bfd_reloc_status_type
-bpf_elf_generic_reloc (bfd * abfd, arelent *reloc_entry, asymbol *symbol,
+bpf_elf_generic_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
 		       void *data, asection *input_section,
 		       bfd *output_bfd,
 		       char **error_message ATTRIBUTE_UNUSED)
@@ -607,7 +608,15 @@ bpf_elf_generic_reloc (bfd * abfd, arelent *reloc_entry, asymbol *symbol,
   bfd_byte *where;
 
   /* Sanity check that the address is in range.  */
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  if (reloc_entry->howto->type == R_BPF_INSN_64)
+    {
+      bfd_size_type end = bfd_get_section_limit_octets (abfd, input_section);
+      if (reloc_entry->address > end
+	  || end - reloc_entry->address < 16)
+	return bfd_reloc_outofrange;
+    }
+  else if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd, input_section,
+				       reloc_entry->address))
     return bfd_reloc_outofrange;
 
   /*  Get the symbol value.  */
@@ -640,15 +649,15 @@ bpf_elf_generic_reloc (bfd * abfd, arelent *reloc_entry, asymbol *symbol,
 	 instructions, and the upper 32 bits placed at the very end of the
 	 instruction. that is, there are 32 unused bits between them. */
 
-      bfd_put_32 (output_bfd, (relocation & 0xFFFFFFFF), where + 4);
-      bfd_put_32 (output_bfd, (relocation >> 32), where + 12);
+      bfd_put_32 (abfd, (relocation & 0xFFFFFFFF), where + 4);
+      bfd_put_32 (abfd, (relocation >> 32), where + 12);
     }
   else
     {
       /* For other kinds of relocations, the relocated value simply goes
 	 BITPOS bits from the start of the entry. This is always a multiple
 	 of 8, i.e. whole bytes.  */
-      bfd_put (reloc_entry->howto->bitsize, output_bfd, relocation,
+      bfd_put (reloc_entry->howto->bitsize, abfd, relocation,
 	       where + reloc_entry->howto->bitpos / 8);
     }
 
 

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR28055, segfault in bpf special reloc function

[Simon Marchi]

On 2021-07-05 8:22 a.m., Alan Modra wrote:
> The testcase in this PR tickled two bugs fixed here.  output_bfd is
> NULL when a reloc special_function is called for final linking and
> when called from bfd_generic_get_relocated_section_contents.  Clearly
> using output_bfd is wrong as it results in segfaults.  Not only that,
> the endianness of the reloc field really should be that of the input.
> The second bug was not checking that the entire reloc field was
> contained in the section contents.
> 
> I was going to add the following too, but it looks like this would
> only be dead code since ld -r isn't supported in combination of input
> and output that would see this special_function called.
> 
>   /* If this is a relocatable link (output_bfd test tells us), just
>      call the generic function.  */
>   if (abfd != output_bfd && output_bfd != NULL)
>     return bfd_elf_generic_reloc (abfd, reloc_entry, symbol, data,
> 				  input_section, output_bfd, error_message);
> 
> 
> 	PR 28055
> 	* elf64-bpf.c (bpf_elf_generic_reloc): Use correct bfd for bfd_put
> 	and bfd_put_32 calls.  Correct section limit checks.

Hi Alan,

After this patch, I get:

/bin/sh ./libtool  --tag=CC   --mode=compile ccache gcc -DHAVE_CONFIG_H -I. -I/home/simark/src/binutils-gdb/bfd  -DBINDIR='"/usr/local/bin"' -DLIBDIR='"/usr/local/lib"' -I. -I/home/simark/src/binutils-gdb/bfd -I/home/simark/src/binutils-gdb/bfd/../include  -DHAVE_all_vecs   -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -Werror  -g3 -O0 -fsanitize=address -fmax-errors=1 -fdiagnostics-color=always     -MT elf64-bpf.lo -MD -MP -MF .deps/elf64-bpf.Tpo -c -o elf64-bpf.lo /home/simark/src/binutils-gdb/bfd/elf64-bpf.c
libtool: compile:  ccache gcc -DHAVE_CONFIG_H -I. -I/home/simark/src/binutils-gdb/bfd -DBINDIR=\"/usr/local/bin\" -DLIBDIR=\"/usr/local/lib\" -I. -I/home/simark/src/binutils-gdb/bfd -I/home/simark/src/binutils-gdb/bfd/../include -DHAVE_all_vecs -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -Werror -g3 -O0 -fsanitize=address -fmax-errors=1 -fdiagnostics-color=always -MT elf64-bpf.lo -MD -MP -MF .deps/elf64-bpf.Tpo -c /home/simark/src/binutils-gdb/bfd/elf64-bpf.c -o elf64-bpf.o
/home/simark/src/binutils-gdb/bfd/elf64-bpf.c: In function ‘bpf_elf_generic_reloc’:
/home/simark/src/binutils-gdb/bfd/elf64-bpf.c:602:29: error: unused parameter ‘output_bfd’ [-Werror=unused-parameter]
  602 |                        bfd *output_bfd,
      |                        ~~~~~^~~~~~~~~~


Simon

Re: PR28055, segfault in bpf special reloc function

[Alan Modra]

On Mon, Jul 05, 2021 at 10:37:35AM -0400, Simon Marchi wrote:
> On 2021-07-05 8:22 a.m., Alan Modra wrote:
> > The testcase in this PR tickled two bugs fixed here.  output_bfd is
> > NULL when a reloc special_function is called for final linking and
> > when called from bfd_generic_get_relocated_section_contents.  Clearly
> > using output_bfd is wrong as it results in segfaults.  Not only that,
> > the endianness of the reloc field really should be that of the input.
> > The second bug was not checking that the entire reloc field was
> > contained in the section contents.
> > 
> > I was going to add the following too, but it looks like this would
> > only be dead code since ld -r isn't supported in combination of input
> > and output that would see this special_function called.
> > 
> >   /* If this is a relocatable link (output_bfd test tells us), just
> >      call the generic function.  */
> >   if (abfd != output_bfd && output_bfd != NULL)
> >     return bfd_elf_generic_reloc (abfd, reloc_entry, symbol, data,
> > 				  input_section, output_bfd, error_message);
> > 
> > 
> > 	PR 28055
> > 	* elf64-bpf.c (bpf_elf_generic_reloc): Use correct bfd for bfd_put
> > 	and bfd_put_32 calls.  Correct section limit checks.
> 
> /home/simark/src/binutils-gdb/bfd/elf64-bpf.c: In function ‘bpf_elf_generic_reloc’:
> /home/simark/src/binutils-gdb/bfd/elf64-bpf.c:602:29: error: unused parameter ‘output_bfd’ [-Werror=unused-parameter]
>   602 |                        bfd *output_bfd,
>       |                        ~~~~~^~~~~~~~~~
> 

Oops.

	PR 28055
	* elf64-bpf.c (bpf_elf_generic_reloc): Add missing ATTRIBUTE_UNUSED.

diff --git a/bfd/elf64-bpf.c b/bfd/elf64-bpf.c
index 243df93ae82..beabad79aff 100644
--- a/bfd/elf64-bpf.c
+++ b/bfd/elf64-bpf.c
@@ -599,7 +599,7 @@ elf64_bpf_merge_private_bfd_data (bfd *ibfd, struct bfd_link_info *info)
 static bfd_reloc_status_type
 bpf_elf_generic_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
 		       void *data, asection *input_section,
-		       bfd *output_bfd,
+		       bfd *output_bfd ATTRIBUTE_UNUSED,
 		       char **error_message ATTRIBUTE_UNUSED)
 {



-- 
Alan Modra
Australia Development Lab, IBM

Re: PR28055, segfault in bpf special reloc function

[Jose E. Marchesi]

Hi Alan.

> The testcase in this PR tickled two bugs fixed here.  output_bfd is
> NULL when a reloc special_function is called for final linking and
> when called from bfd_generic_get_relocated_section_contents.  Clearly
> using output_bfd is wrong as it results in segfaults.  Not only that,
> the endianness of the reloc field really should be that of the input.
> The second bug was not checking that the entire reloc field was
> contained in the section contents.

Thanks for fixing this.

> I was going to add the following too, but it looks like this would
> only be dead code since ld -r isn't supported in combination of input
> and output that would see this special_function called.
>
>   /* If this is a relocatable link (output_bfd test tells us), just
>      call the generic function.  */
>   if (abfd != output_bfd && output_bfd != NULL)
>     return bfd_elf_generic_reloc (abfd, reloc_entry, symbol, data,
> 				  input_section, output_bfd, error_message);
>
>
> 	PR 28055
> 	* elf64-bpf.c (bpf_elf_generic_reloc): Use correct bfd for bfd_put
> 	and bfd_put_32 calls.  Correct section limit checks.
>
> diff --git a/bfd/elf64-bpf.c b/bfd/elf64-bpf.c
> index 28c1543c0b6..243df93ae82 100644
> --- a/bfd/elf64-bpf.c
> +++ b/bfd/elf64-bpf.c
> @@ -589,14 +589,15 @@ elf64_bpf_merge_private_bfd_data (bfd *ibfd, struct bfd_link_info *info)
>  }
>  
>  /* A generic howto special function for installing BPF relocations.
> -   This function will be called by the assembler (via bfd_install_relocation).
> +   This function will be called by the assembler (via bfd_install_relocation),
> +   and by various get_relocated_section_contents functions.
>     At link time, bpf_elf_relocate_section will resolve the final relocations.
>  
>     BPF instructions are always big endian, and this approach avoids problems in
>     bfd_install_relocation.  */
>  
>  static bfd_reloc_status_type
> -bpf_elf_generic_reloc (bfd * abfd, arelent *reloc_entry, asymbol *symbol,
> +bpf_elf_generic_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
>  		       void *data, asection *input_section,
>  		       bfd *output_bfd,
>  		       char **error_message ATTRIBUTE_UNUSED)
> @@ -607,7 +608,15 @@ bpf_elf_generic_reloc (bfd * abfd, arelent *reloc_entry, asymbol *symbol,
>    bfd_byte *where;
>  
>    /* Sanity check that the address is in range.  */
> -  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
> +  if (reloc_entry->howto->type == R_BPF_INSN_64)
> +    {
> +      bfd_size_type end = bfd_get_section_limit_octets (abfd, input_section);
> +      if (reloc_entry->address > end
> +	  || end - reloc_entry->address < 16)
> +	return bfd_reloc_outofrange;
> +    }
> +  else if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd, input_section,
> +				       reloc_entry->address))
>      return bfd_reloc_outofrange;
>  
>    /*  Get the symbol value.  */
> @@ -640,15 +649,15 @@ bpf_elf_generic_reloc (bfd * abfd, arelent *reloc_entry, asymbol *symbol,
>  	 instructions, and the upper 32 bits placed at the very end of the
>  	 instruction. that is, there are 32 unused bits between them. */
>  
> -      bfd_put_32 (output_bfd, (relocation & 0xFFFFFFFF), where + 4);
> -      bfd_put_32 (output_bfd, (relocation >> 32), where + 12);
> +      bfd_put_32 (abfd, (relocation & 0xFFFFFFFF), where + 4);
> +      bfd_put_32 (abfd, (relocation >> 32), where + 12);
>      }
>    else
>      {
>        /* For other kinds of relocations, the relocated value simply goes
>  	 BITPOS bits from the start of the entry. This is always a multiple
>  	 of 8, i.e. whole bytes.  */
> -      bfd_put (reloc_entry->howto->bitsize, output_bfd, relocation,
> +      bfd_put (reloc_entry->howto->bitsize, abfd, relocation,
>  	       where + reloc_entry->howto->bitpos / 8);
>      }