[PATCH 0/2] LoongArch: Fix two bugs breaking IFUNC in static PIE

[PATCH 0/2] LoongArch: Fix two bugs breaking IFUNC in static PIE

[Xi Ruoyao]

To generate a static PIE, the compiler should pass "-static -pie
--no-dynamic-linker -z text" options to the linker [1].  But, LoongArch
BFD linker currently produces wrong result (missing R_LARCH_IRELATIVE
relocs) with IFUNC and those options, causing test failures in Glibc if
static PIE is enabled.  Fix them now.

For the detailed analysis of the bugs see the commit messages.

[1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601541.html

Xi Ruoyao (2):
  LoongArch: Avoid heap-buffer-overflow in
    loongarch_elf_relocate_section
  LoongArch: Fix R_LARCH_IRELATIVE insertion after elf_link_sort_relocs

 bfd/elfnn-loongarch.c | 59 +++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

-- 
2.37.0

[PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[Xi Ruoyao]

If a and b are different sections, we cannot access something in b with
"a->contents + (offset from a)" because "a->contents" and "b->contents"
are heap buffers allocated separately, not slices of a large buffer.

The issue was found during an attempt to add static-pie support to the
toolchain with ASAN.
---
 bfd/elfnn-loongarch.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index ed42b8b6770..4b408b1db72 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -3128,6 +3128,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 	      unresolved_reloc = false;
 	      BFD_ASSERT (rel->r_addend == 0);
 
+	      asection *my_got = got;
 	      bfd_vma got_off = 0;
 	      if (h != NULL)
 		{
@@ -3145,17 +3146,14 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 			{
 			  idx = (h->plt.offset - PLT_HEADER_SIZE)
 			    / PLT_ENTRY_SIZE;
-			  got_off = sec_addr (htab->elf.sgotplt)
-			    + GOTPLT_HEADER_SIZE
-			    + (idx * GOT_ENTRY_SIZE)
-			    - sec_addr (htab->elf.sgot);
+			  my_got = htab->elf.sgotplt;
+			  got_off = GOTPLT_HEADER_SIZE + idx * GOT_ENTRY_SIZE;
 			}
 		      else
 			{
 			  idx = h->plt.offset / PLT_ENTRY_SIZE;
-			  got_off = sec_addr (htab->elf.sgotplt)
-			    + (idx * GOT_ENTRY_SIZE)
-			    - sec_addr (htab->elf.sgot);
+			  my_got = htab->elf.sgotplt;
+			  got_off = idx * GOT_ENTRY_SIZE;
 			}
 		    }
 
@@ -3172,7 +3170,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 			  && SYMBOL_REFERENCES_LOCAL (info, h))
 			{
 			  Elf_Internal_Rela rela;
-			  rela.r_offset = sec_addr (got) + got_off;
+			  rela.r_offset = sec_addr (my_got) + got_off;
 			  rela.r_info = ELFNN_R_INFO (0, R_LARCH_RELATIVE);
 			  rela.r_addend = relocation;
 			  loongarch_elf_append_rela (output_bfd,
@@ -3202,9 +3200,9 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		    }
 		}
 
-	      bfd_put_NN (output_bfd, relocation, got->contents + got_off);
+	      bfd_put_NN (output_bfd, relocation, my_got->contents + got_off);
 
-	      relocation = got_off + sec_addr (got);
+	      relocation = got_off + sec_addr (my_got);
 	    }
 
 	  if (r_type == R_LARCH_GOT_PC_HI20)
-- 
2.37.0

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[liuzhensong]

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


在 2022/9/13 下午11:44, Xi Ruoyao 写道:
> If a and b are different sections, we cannot access something in b with
> "a->contents + (offset from a)" because "a->contents" and "b->contents"
> are heap buffers allocated separately, not slices of a large buffer.
>
> The issue was found during an attempt to add static-pie support to the
> toolchain with ASAN.

Can you provide compile parameters?

> ---
>   bfd/elfnn-loongarch.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index ed42b8b6770..4b408b1db72 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -3128,6 +3128,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   	      unresolved_reloc = false;
>   	      BFD_ASSERT (rel->r_addend == 0);
>   
> +	      asection *my_got = got;
>   	      bfd_vma got_off = 0;
>   	      if (h != NULL)
>   		{
> @@ -3145,17 +3146,14 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   			{
>   			  idx = (h->plt.offset - PLT_HEADER_SIZE)
>   			    / PLT_ENTRY_SIZE;
> -			  got_off = sec_addr (htab->elf.sgotplt)
> -			    + GOTPLT_HEADER_SIZE
> -			    + (idx * GOT_ENTRY_SIZE)
> -			    - sec_addr (htab->elf.sgot);
> +			  my_got = htab->elf.sgotplt;
> +			  got_off = GOTPLT_HEADER_SIZE + idx * GOT_ENTRY_SIZE;
>   			}
>   		      else
>   			{
>   			  idx = h->plt.offset / PLT_ENTRY_SIZE;
> -			  got_off = sec_addr (htab->elf.sgotplt)
> -			    + (idx * GOT_ENTRY_SIZE)
> -			    - sec_addr (htab->elf.sgot);
> +			  my_got = htab->elf.sgotplt;
> +			  got_off = idx * GOT_ENTRY_SIZE;
>   			}
>   		    }
>   
> @@ -3172,7 +3170,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   			  && SYMBOL_REFERENCES_LOCAL (info, h))
>   			{
>   			  Elf_Internal_Rela rela;
> -			  rela.r_offset = sec_addr (got) + got_off;
> +			  rela.r_offset = sec_addr (my_got) + got_off;
>   			  rela.r_info = ELFNN_R_INFO (0, R_LARCH_RELATIVE);
>   			  rela.r_addend = relocation;
>   			  loongarch_elf_append_rela (output_bfd,
> @@ -3202,9 +3200,9 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		    }
>   		}
>   
> -	      bfd_put_NN (output_bfd, relocation, got->contents + got_off);
> +	      bfd_put_NN (output_bfd, relocation, my_got->contents + got_off);
>   
> -	      relocation = got_off + sec_addr (got);
> +	      relocation = got_off + sec_addr (my_got);
>   	    }
>   
>   	  if (r_type == R_LARCH_GOT_PC_HI20)


This may be the reason of overflow.

Shouldn't write to got table when using hidden ifunc.


diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index ed42b8b6770..9278faa91aa 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -3179,6 +3179,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, 
struct bfd_link_info *info,
htab->elf.srelgot, &rela);
                         }
                       h->got.offset |= 1;
+                     bfd_put_NN (output_bfd, relocation, got->contents 
+ got_off);
                     }
                 }
               else
@@ -3200,10 +3201,9 @@ loongarch_elf_relocate_section (bfd *output_bfd, 
struct bfd_link_info *info,
                         }
                       local_got_offsets[r_symndx] |= 1;
                     }
+                 bfd_put_NN (output_bfd, relocation, got->contents + 
got_off);
                 }

-             bfd_put_NN (output_bfd, relocation, got->contents + got_off);
-
               relocation = got_off + sec_addr (got);
             }

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[Xi Ruoyao]

On Wed, 2022-09-14 at 16:57 +0800, liuzhensong wrote:
> 在 2022/9/13 下午11:44, Xi Ruoyao 写道:
>  
> > If a and b are different sections, we cannot access something in b
> > with
> > "a->contents + (offset from a)" because "a->contents" and "b-
> > >contents"
> > are heap buffers allocated separately, not slices of a large buffer.
> > 
> > The issue was found during an attempt to add static-pie support to
> > the
> > toolchain with ASAN.
> Can you provide compile parameters?

To reproduce it easily, add a check to detect the heap buffer overflow:

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index a9bb66a1e04..716e3d5a246 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -3202,6 +3202,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
                    }
                }
 
+             BFD_ASSERT (got_off < got->size);
              bfd_put_NN (output_bfd, relocation, got->contents + got_off);
 
              relocation = got_off + sec_addr (got);

Then

$ cat test.S
.text
.align 2

.local ifunc
.type ifunc, @gnu_indirect_function
.set ifunc, resolver

resolver:
  la.local $a0, impl
  jr $ra

impl:
  li.w $a0, 42
  jr $ra

.global test
.type test, @function
test:
  move $s0, $ra
  la.got $t0, ifunc
  jirl $ra, $t0, 0
  xori $a0, $a0, 42
  jr $s0
$ cc test.S -c
$ ld/ld-new test.o -shared
ld/ld-new: BFD (GNU Binutils) 2.39.50.20220914 assertion fail elfnn-loongarch.c:3205

And if GDB is used with a breakpoint at bfd_assert, we can see got_off
is "18446744073709551608" (-8).

> Shouldn't write to got table when using hidden ifunc.

Perhaps it's true, using RELA to resolve a GOT entry should not depend
on any "initial" value of the entry...
-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[Xi Ruoyao]

On Wed, 2022-09-14 at 18:15 +0800, Xi Ruoyao via Binutils wrote:

> > Shouldn't write to got table when using hidden ifunc.
> 
> Perhaps it's true, using RELA to resolve a GOT entry should not depend
> on any "initial" value of the entry...


How about this?  We don't need to write into the GOT if R_LARCH_RELATIVE
or R_LARCH_IRELATIVE will be used:

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index a9bb66a1e04..1e8ecb2b8e2 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -3129,6 +3129,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 	      BFD_ASSERT (rel->r_addend == 0);
 
 	      bfd_vma got_off = 0;
+	      bool fill_got_entry = true;
 	      if (h != NULL)
 		{
 		  /* GOT ref or ifunc.  */
@@ -3141,6 +3142,10 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		  if (h->got.offset == MINUS_ONE && h->type == STT_GNU_IFUNC)
 		    {
 		      bfd_vma idx;
+
+		      /* An IFUNC is always resolved at runtime.  */
+		      fill_got_entry = false;
+
 		      if (htab->elf.splt != NULL)
 			{
 			  idx = (h->plt.offset - PLT_HEADER_SIZE)
@@ -3177,6 +3182,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 			  rela.r_addend = relocation;
 			  loongarch_elf_append_rela (output_bfd,
 						     htab->elf.srelgot, &rela);
+			  fill_got_entry = false;
 			}
 		      h->got.offset |= 1;
 		    }
@@ -3197,12 +3203,14 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 			  rela.r_addend = relocation;
 			  loongarch_elf_append_rela (output_bfd,
 						     htab->elf.srelgot, &rela);
+			  fill_got_entry = false;
 			}
 		      local_got_offsets[r_symndx] |= 1;
 		    }
 		}
 
-	      bfd_put_NN (output_bfd, relocation, got->contents + got_off);
+	      if (fill_got_entry)
+		bfd_put_NN (output_bfd, relocation, got->contents + got_off);
 
 	      relocation = got_off + sec_addr (got);
 	    }

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[liuzhensong]

在 2022/9/14 下午7:15, Xi Ruoyao 写道:
> On Wed, 2022-09-14 at 18:15 +0800, Xi Ruoyao via Binutils wrote:
>
>>> Shouldn't write to got table when using hidden ifunc.
>> Perhaps it's true, using RELA to resolve a GOT entry should not depend
>> on any "initial" value of the entry...
>
> How about this?  We don't need to write into the GOT if R_LARCH_RELATIVE
> or R_LARCH_IRELATIVE will be used:
 > "We don't need to write into the GOT if R_LARCH_RELATIVE or 
R_LARCH_IRELATIVE will be used:"

Not only this, you can refer to the implementation of the function 
_bfd_elf_allocate_ifunc_dyn_relocs for details.

>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index a9bb66a1e04..1e8ecb2b8e2 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -3129,6 +3129,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   	      BFD_ASSERT (rel->r_addend == 0);
>   
>   	      bfd_vma got_off = 0;
> +	      bool fill_got_entry = true;
>   	      if (h != NULL)
>   		{
>   		  /* GOT ref or ifunc.  */
> @@ -3141,6 +3142,10 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		  if (h->got.offset == MINUS_ONE && h->type == STT_GNU_IFUNC)
>   		    {
>   		      bfd_vma idx;
> +
> +		      /* An IFUNC is always resolved at runtime.  */
> +		      fill_got_entry = false;
> +
>   		      if (htab->elf.splt != NULL)
>   			{
>   			  idx = (h->plt.offset - PLT_HEADER_SIZE)
> @@ -3177,6 +3182,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   			  rela.r_addend = relocation;
>   			  loongarch_elf_append_rela (output_bfd,
>   						     htab->elf.srelgot, &rela);
> +			  fill_got_entry = false;
>   			}
>   		      h->got.offset |= 1;
>   		    }
> @@ -3197,12 +3203,14 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   			  rela.r_addend = relocation;
>   			  loongarch_elf_append_rela (output_bfd,
>   						     htab->elf.srelgot, &rela);
> +			  fill_got_entry = false;
>   			}
>   		      local_got_offsets[r_symndx] |= 1;
>   		    }
>   		}
>   
> -	      bfd_put_NN (output_bfd, relocation, got->contents + got_off);
> +	      if (fill_got_entry)
> +		bfd_put_NN (output_bfd, relocation, got->contents + got_off);
>   
>   	      relocation = got_off + sec_addr (got);
>   	    }
>

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[Xi Ruoyao]

On Thu, 2022-09-15 at 09:47 +0800, liuzhensong wrote:
> > How about this?  We don't need to write into the GOT if
> > R_LARCH_RELATIVE
> > or R_LARCH_IRELATIVE will be used:
>  > "We don't need to write into the GOT if R_LARCH_RELATIVE or 
> R_LARCH_IRELATIVE will be used:"
> 
> Not only this, you can refer to the implementation of the function 
> _bfd_elf_allocate_ifunc_dyn_relocs for details.

I don't think _bfd_elf_allocate_ifunc_dyn_relocs (or
local_allocate_ifunc_dyn_relocs for us) and
loongarch_elf_relocate_section are strictly aligned.  We need to
allocate the space for GOT entry, but no need to fill in the content if
the entry will be resolved by R_LARCH_RELATIVE or R_LARCH_IRELATIVE
because they are RELA and the "origin" value of the entry is not used
during the resolution.

By the way, the code paths for REL can be also removed from
local_allocate_ifunc_dyn_relocs because LoongArch does not use REL at
all.

And the change passes ld testsuite, the resulted ld passes glibc
testsuite.

> > 
> > diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> > index a9bb66a1e04..1e8ecb2b8e2 100644
> > --- a/bfd/elfnn-loongarch.c
> > +++ b/bfd/elfnn-loongarch.c
> > @@ -3129,6 +3129,7 @@ loongarch_elf_relocate_section (bfd
> > *output_bfd, struct bfd_link_info *info,
> >               BFD_ASSERT (rel->r_addend == 0);
> >    
> >               bfd_vma got_off = 0;
> > +             bool fill_got_entry = true;
> >               if (h != NULL)
> >                 {
> >                   /* GOT ref or ifunc.  */
> > @@ -3141,6 +3142,10 @@ loongarch_elf_relocate_section (bfd
> > *output_bfd, struct bfd_link_info *info,
> >                   if (h->got.offset == MINUS_ONE && h->type ==
> > STT_GNU_IFUNC)
> >                     {
> >                       bfd_vma idx;
> > +
> > +                     /* An IFUNC is always resolved at runtime.  */
> > +                     fill_got_entry = false;
> > +
> >                       if (htab->elf.splt != NULL)
> >                         {
> >                           idx = (h->plt.offset - PLT_HEADER_SIZE)
> > @@ -3177,6 +3182,7 @@ loongarch_elf_relocate_section (bfd
> > *output_bfd, struct bfd_link_info *info,
> >                           rela.r_addend = relocation;
> >                           loongarch_elf_append_rela (output_bfd,
> >                                                      htab-
> > >elf.srelgot, &rela);
> > +                         fill_got_entry = false;
> >                         }
> >                       h->got.offset |= 1;
> >                     }
> > @@ -3197,12 +3203,14 @@ loongarch_elf_relocate_section (bfd
> > *output_bfd, struct bfd_link_info *info,
> >                           rela.r_addend = relocation;
> >                           loongarch_elf_append_rela (output_bfd,
> >                                                      htab-
> > >elf.srelgot, &rela);
> > +                         fill_got_entry = false;
> >                         }
> >                       local_got_offsets[r_symndx] |= 1;
> >                     }
> >                 }
> >    
> > -             bfd_put_NN (output_bfd, relocation, got->contents +
> > got_off);
> > +             if (fill_got_entry)
> > +               bfd_put_NN (output_bfd, relocation, got->contents +
> > got_off);
> >    
> >               relocation = got_off + sec_addr (got);
> >             }
> > 

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[liuzhensong]

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


在 2022/9/15 上午10:56, Xi Ruoyao 写道:
> On Thu, 2022-09-15 at 09:47 +0800, liuzhensong wrote:
>>> How about this?  We don't need to write into the GOT if
>>> R_LARCH_RELATIVE
>>> or R_LARCH_IRELATIVE will be used:
>>   > "We don't need to write into the GOT if R_LARCH_RELATIVE or
>> R_LARCH_IRELATIVE will be used:"
>>
>> Not only this, you can refer to the implementation of the function
>> _bfd_elf_allocate_ifunc_dyn_relocs for details.
> I don't think _bfd_elf_allocate_ifunc_dyn_relocs (or
> local_allocate_ifunc_dyn_relocs for us) and
> loongarch_elf_relocate_section are strictly aligned.  We need to
> allocate the space for GOT entry, but no need to fill in the content if
> the entry will be resolved by R_LARCH_RELATIVE or R_LARCH_IRELATIVE
> because they are RELA and the "origin" value of the entry is not used
> during the resolution.

Some reasons of overflow.

>
> By the way, the code paths for REL can be also removed from
> local_allocate_ifunc_dyn_relocs because LoongArch does not use REL at
> all.
>
> And the change passes ld testsuite, the resulted ld passes glibc
> testsuite.
>
>>> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
>>> index a9bb66a1e04..1e8ecb2b8e2 100644
>>> --- a/bfd/elfnn-loongarch.c
>>> +++ b/bfd/elfnn-loongarch.c
>>> @@ -3129,6 +3129,7 @@ loongarch_elf_relocate_section (bfd
>>> *output_bfd, struct bfd_link_info *info,
>>>                BFD_ASSERT (rel->r_addend == 0);
>>>     
>>>                bfd_vma got_off = 0;
>>> +             bool fill_got_entry = true;
>>>                if (h != NULL)
>>>                  {
>>>                    /* GOT ref or ifunc.  */
>>> @@ -3141,6 +3142,10 @@ loongarch_elf_relocate_section (bfd
>>> *output_bfd, struct bfd_link_info *info,
>>>                    if (h->got.offset == MINUS_ONE && h->type ==
>>> STT_GNU_IFUNC)
>>>                      {
>>>                        bfd_vma idx;
>>> +
>>> +                     /* An IFUNC is always resolved at runtime.  */

Not correct, anything can be filled if h->got.offset != MINUS_ONE.

There is no space for ifunc to fill got.

>>> +                     fill_got_entry = false;
>>> +
>>>                        if (htab->elf.splt != NULL)
>>>                          {
>>>                            idx = (h->plt.offset - PLT_HEADER_SIZE)
>>> @@ -3177,6 +3182,7 @@ loongarch_elf_relocate_section (bfd
>>> *output_bfd, struct bfd_link_info *info,
>>>                            rela.r_addend = relocation;
>>>                            loongarch_elf_append_rela (output_bfd,
>>>                                                       htab-
>>>> elf.srelgot, &rela);
>>> +                         fill_got_entry = false;
>>>                          }
>>>                        h->got.offset |= 1;
>>>                      }
>>> @@ -3197,12 +3203,14 @@ loongarch_elf_relocate_section (bfd
>>> *output_bfd, struct bfd_link_info *info,
>>>                            rela.r_addend = relocation;
>>>                            loongarch_elf_append_rela (output_bfd,
>>>                                                       htab-
>>>> elf.srelgot, &rela);
>>> +                         fill_got_entry = false;
>>>                          }
>>>                        local_got_offsets[r_symndx] |= 1;
>>>                      }
>>>                  }
>>>     
>>> -             bfd_put_NN (output_bfd, relocation, got->contents +
>>> got_off);
>>> +             if (fill_got_entry)
>>> +               bfd_put_NN (output_bfd, relocation, got->contents +
>>> got_off);
>>>     
>>>                relocation = got_off + sec_addr (got);
>>>              }
>>>

Re: [PATCH 1/2] LoongArch: Avoid heap-buffer-overflow in loongarch_elf_relocate_section

[Xi Ruoyao]

On Thu, 2022-09-15 at 11:54 +0800, liuzhensong wrote:

> > > > diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> > > > index a9bb66a1e04..1e8ecb2b8e2 100644
> > > > --- a/bfd/elfnn-loongarch.c
> > > > +++ b/bfd/elfnn-loongarch.c
> > > > @@ -3129,6 +3129,7 @@ loongarch_elf_relocate_section (bfd
> > > > *output_bfd, struct bfd_link_info *info,
> > > >               BFD_ASSERT (rel->r_addend == 0);
> > > >    
> > > >               bfd_vma got_off = 0;
> > > > +             bool fill_got_entry = true;
> > > >               if (h != NULL)
> > > >                 {
> > > >                   /* GOT ref or ifunc.  */
> > > > @@ -3141,6 +3142,10 @@ loongarch_elf_relocate_section (bfd
> > > > *output_bfd, struct bfd_link_info *info,
> > > >                   if (h->got.offset == MINUS_ONE && h->type ==
> > > > STT_GNU_IFUNC)
> > > >                     {
> > > >                       bfd_vma idx;
> > > > +
> > > > +                     /* An IFUNC is always resolved at
> > > > runtime.  */
> Not correct, anything can be filled if h->got.offset != MINUS_ONE.
> There is no space for ifunc to fill got.

Alright, I'm not familiar with BFD and I don't want to spend too much
time (and brain cell deaths) learning it.  So I'll test your change and
if it works let's just do it in your way...

In the meantime please take a look of the 2nd patch, they are different
(and unrelated) issues.


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

[PATCH 2/2] LoongArch: Fix R_LARCH_IRELATIVE insertion after elf_link_sort_relocs

[Xi Ruoyao]

loongarch_elf_finish_dynamic_symbol is called after elf_link_sort_relocs
if -z combreloc.  elf_link_sort_relocs redistributes the contents of
.rela.* sections those would be merged into .rela.dyn, so the slot for
R_LARCH_IRELATIVE may be out of relplt->contents now.

To make things worse, the boundary check

    dyn < dyn + relplt->size / sizeof (*dyn)

is obviously wrong ("x + 10 < x"? :), causing the issue undetected
during the linking process and the resulted executable suddenly crashes
at runtime.

The issue was found during an attempt to add static-pie support to the
toolchain.

Fix it by iterating through the inputs of .rela.dyn to find the slot.
---
 bfd/elfnn-loongarch.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index 4b408b1db72..1de9dbfecfa 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -3511,6 +3511,12 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
 {
   struct loongarch_elf_link_hash_table *htab = loongarch_elf_hash_table (info);
   const struct elf_backend_data *bed = get_elf_backend_data (output_bfd);
+  asection *rela_dyn = bfd_get_section_by_name (output_bfd, ".rela.dyn");
+  struct bfd_link_order *lo = NULL;
+  Elf_Internal_Rela *slot = NULL, *last_slot = NULL;
+
+  if (rela_dyn)
+    lo = rela_dyn->map_head.link_order;
 
   if (h->plt.offset != MINUS_ONE)
     {
@@ -3520,6 +3526,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
       uint32_t plt_entry[PLT_ENTRY_INSNS];
       bfd_byte *loc;
       Elf_Internal_Rela rela;
+      asection *rela_sec = NULL;
 
       if (htab->elf.splt)
 	{
@@ -3572,31 +3579,31 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
 	  && (relplt == htab->elf.srelgot
 	      || relplt == htab->elf.irelplt))
 	{
-	    {
-	      rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
-	      rela.r_addend = (h->root.u.def.value
+	  rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
+	  rela.r_addend = (h->root.u.def.value
 			       + h->root.u.def.section->output_section->vma
 			       + h->root.u.def.section->output_offset);
-	    }
 
-	    /* Find the space after dyn sort.  */
+	  /* Find the space after dyn sort.  */
+	  while (slot == last_slot || slot->r_offset != 0)
 	    {
-	      Elf_Internal_Rela *dyn = (Elf_Internal_Rela *)relplt->contents;
-	      bool fill = false;
-	      for (;dyn < dyn + relplt->size / sizeof (*dyn); dyn++)
+	      if (slot != last_slot)
 		{
-		  if (0 == dyn->r_offset)
-		    {
-		      bed->s->swap_reloca_out (output_bfd, &rela,
-					       (bfd_byte *)dyn);
-		      relplt->reloc_count++;
-		      fill = true;
-		      break;
-		    }
+		  slot++;
+		  continue;
 		}
-	      BFD_ASSERT (fill);
+
+	      BFD_ASSERT (lo != NULL);
+	      rela_sec = lo->u.indirect.section;
+	      lo = lo->next;
+
+	      slot = (Elf_Internal_Rela *)rela_sec->contents;
+	      last_slot = (Elf_Internal_Rela *)(rela_sec->contents +
+						rela_sec->size);
 	    }
 
+	  bed->s->swap_reloca_out (output_bfd, &rela, (bfd_byte *)slot);
+	  rela_sec->reloc_count++;
 	}
       else
 	{
-- 
2.37.0

Re: [PATCH 2/2] LoongArch: Fix R_LARCH_IRELATIVE insertion after elf_link_sort_relocs

[liuzhensong]

在 2022/9/13 下午11:44, Xi Ruoyao 写道:
> loongarch_elf_finish_dynamic_symbol is called after elf_link_sort_relocs
> if -z combreloc.  elf_link_sort_relocs redistributes the contents of
> .rela.* sections those would be merged into .rela.dyn, so the slot for
> R_LARCH_IRELATIVE may be out of relplt->contents now.

It May be unnecessary find the slot in section of .rela.dyn.

The slot of R_LARCH_IRELATIVE is within the scope relplt->contents (the 
space

is calculated in function local_allocate_ifunc_dyn_relocs).

We can find the slot just in relplt->contents(Only for dynamic ifunc. I 
did not test for static ifunc.).

>
> To make things worse, the boundary check
>
>      dyn < dyn + relplt->size / sizeof (*dyn)
>
> is obviously wrong ("x + 10 < x"? :), causing the issue undetected
> during the linking process and the resulted executable suddenly crashes
> at runtime.
>
> The issue was found during an attempt to add static-pie support to the
> toolchain.
>
> Fix it by iterating through the inputs of .rela.dyn to find the slot.
> ---
>   bfd/elfnn-loongarch.c | 41 ++++++++++++++++++++++++-----------------
>   1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index 4b408b1db72..1de9dbfecfa 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -3511,6 +3511,12 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>   {
>     struct loongarch_elf_link_hash_table *htab = loongarch_elf_hash_table (info);
>     const struct elf_backend_data *bed = get_elf_backend_data (output_bfd);
> +  asection *rela_dyn = bfd_get_section_by_name (output_bfd, ".rela.dyn");
> +  struct bfd_link_order *lo = NULL;
> +  Elf_Internal_Rela *slot = NULL, *last_slot = NULL;
> +
> +  if (rela_dyn)
> +    lo = rela_dyn->map_head.link_order;
>   
>     if (h->plt.offset != MINUS_ONE)
>       {
> @@ -3520,6 +3526,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>         uint32_t plt_entry[PLT_ENTRY_INSNS];
>         bfd_byte *loc;
>         Elf_Internal_Rela rela;
> +      asection *rela_sec = NULL;
>   
>         if (htab->elf.splt)
>   	{
> @@ -3572,31 +3579,31 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>   	  && (relplt == htab->elf.srelgot
>   	      || relplt == htab->elf.irelplt))
>   	{
> -	    {
> -	      rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
> -	      rela.r_addend = (h->root.u.def.value
> +	  rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
> +	  rela.r_addend = (h->root.u.def.value
>   			       + h->root.u.def.section->output_section->vma
>   			       + h->root.u.def.section->output_offset);
> -	    }
>   
> -	    /* Find the space after dyn sort.  */
> +	  /* Find the space after dyn sort.  */
> +	  while (slot == last_slot || slot->r_offset != 0)
>   	    {
> -	      Elf_Internal_Rela *dyn = (Elf_Internal_Rela *)relplt->contents;
> -	      bool fill = false;
> -	      for (;dyn < dyn + relplt->size / sizeof (*dyn); dyn++)
My mistake.
> +	      if (slot != last_slot)
>   		{
> -		  if (0 == dyn->r_offset)
> -		    {
> -		      bed->s->swap_reloca_out (output_bfd, &rela,
> -					       (bfd_byte *)dyn);
> -		      relplt->reloc_count++;
> -		      fill = true;
> -		      break;
> -		    }
> +		  slot++;
> +		  continue;
>   		}
> -	      BFD_ASSERT (fill);
> +
> +	      BFD_ASSERT (lo != NULL);
> +	      rela_sec = lo->u.indirect.section;
> +	      lo = lo->next;
> +
> +	      slot = (Elf_Internal_Rela *)rela_sec->contents;
> +	      last_slot = (Elf_Internal_Rela *)(rela_sec->contents +
> +						rela_sec->size);
>   	    }
>   
> +	  bed->s->swap_reloca_out (output_bfd, &rela, (bfd_byte *)slot);
> +	  rela_sec->reloc_count++;
>   	}
>         else
>   	{

Re: [PATCH 2/2] LoongArch: Fix R_LARCH_IRELATIVE insertion after elf_link_sort_relocs

[Xi Ruoyao]

On Thu, 2022-09-15 at 21:03 +0800, liuzhensong wrote:
> It May be unnecessary find the slot in section of .rela.dyn.
> 
> The slot of R_LARCH_IRELATIVE is within the scope relplt->contents (the 
> space
> 
> is calculated in function local_allocate_ifunc_dyn_relocs).
> 
> We can find the slot just in relplt->contents(Only for dynamic ifunc. I 
> did not test for static ifunc.).

The problem is loongarch_elf_finish_dynamic_symbol runs after
elf_link_sort_relocs.  elf_link_sort_relocs basically does:

(pseudo code)

buffer = []
for section in {input of .rela.dyn}
    buffer += (section.content) as [RELA]

buffer.sort_by(some_comparator)

buffer = buffer as [byte]
for section in {input of .rela.dyn}
    section.content = buffer[..section.content.len()]
    buffer = buffer[section.content.len()..]

So a "slot" in relplt->contents can end up elsewhere.  You can read the
code of elf_link_sort_relocs to see it...

I can try to write a test case for this, but it's 04:00 AM now so I'd do
it after a sleep...
-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH 2/2] LoongArch: Fix R_LARCH_IRELATIVE insertion after elf_link_sort_relocs

[Xi Ruoyao]

On Sat, 2022-09-17 at 04:11 +0800, Xi Ruoyao via Binutils wrote:

> The problem is loongarch_elf_finish_dynamic_symbol runs after
> elf_link_sort_relocs.  elf_link_sort_relocs basically does:
> 
> (pseudo code)
> 
> buffer = []
> for section in {input of .rela.dyn}
>     buffer += (section.content) as [RELA]
> 
> buffer.sort_by(some_comparator)
> 
> buffer = buffer as [byte]
> for section in {input of .rela.dyn}
>     section.content = buffer[..section.content.len()]
>     buffer = buffer[section.content.len()..]
> 
> So a "slot" in relplt->contents can end up elsewhere.  You can read the
> code of elf_link_sort_relocs to see it...
> 
> I can try to write a test case for this, but it's 04:00 AM now so I'd do
> it after a sleep...

Take this as an example, note that the R_LARCH_IRELATIVE relocation is
missing with -z combreloc:

$ cat b1.s
.text
.align 2

.local ifunc
.type ifunc, @gnu_indirect_function
.set ifunc, resolver

resolver:
  la.local $a0, impl
  jr $ra

impl:
  li.w $a0, 42
  jr $ra

.global test
.type test, @function
test:
  move $s0, $ra
  bl ifunc
  xori $a0, $a0, 42
  jr $s0

.data
.global ptr
.type ptr, @object
ptr:
  .dword test
$ gas/as-new b1.s -o b1.o
$ ld/ld-new -shared b1.o  # "-z combreloc" is the default
$ readelf -r a.out

Relocation section '.rela.dyn' at offset 0x1f0 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000000  000000000000 R_LARCH_NONE                         8018
000000008000  000400000002 R_LARCH_64        0000000000000264 test + 0
$ ld/ld-new -shared b1.o -z nocombreloc
$ readelf -r a.out

Relocation section '.rela.data' at offset 0x1f0 contains 1 entry:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000008000  000400000002 R_LARCH_64        0000000000000264 test + 0

Relocation section '.rela.got' at offset 0x208 contains 1 entry:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000008018  00000000000c R_LARCH_IRELATIVE                    250


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University