[PATCH] RISC-V: Segment fault in riscv_elf_append_rela.

[PATCH] RISC-V: Segment fault in riscv_elf_append_rela.

[Nelson Chu]

I seem to remember someone had sent a pacth for this before, but I cannot
find where it is for now.  So I just send it by myself.

======

% cat tmp.s
foo:
	lui	a0, %hi(_end)     # R_RISCV_HI20
	addi	a0, a0, %lo(_end) # R_RISCV_LO12
	.8byte foo                # R_RISCV_64
% riscv64-unknown-linux-gnu-as tmp.s -o tmp.o
% riscv64-unknown-linux-gnu-ld -shared tmp.o
.* tmp.o: relocation R_RISCV_HI20 against `_end' can not be used when making a shared object; recompile with -fPIC
zsh: segmentation fault

I accidently meet this segment fault from the above case.  Since we don't
allow the absolute access (R_RISCV_HI20) when building shared object, the
riscv_elf_check_relocs should return false directly when analyzing the lui,
so there won't have rel.dyn section.  But linker still try to emit the
dynamic relocation for R_RISCV_64 in the riscv_elf_relocate_section, which
cause the segmant fault in the riscv_elf_append_rela.  Refer to other targets,
Loongarch use BFD_ASSERT, and AARCH64 call abort(), to check if the dynamic
section exsits before emiting.  Since BFD_ASSERT still meet the segmant fault,
I think just call abort here is a better chosen.

bfd/
    * elfnn-riscv.c (riscv_elf_append_rela): Abort when the dynamic section
    doesn't exist.
---
 bfd/elfnn-riscv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 1200e6b11b5..36cdd629aa2 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -182,6 +182,9 @@ riscv_elf_append_rela (bfd *abfd, asection *s, Elf_Internal_Rela *rel)
   const struct elf_backend_data *bed;
   bfd_byte *loc;
 
+  if (!s || !s->contents)
+    abort ();
+
   bed = get_elf_backend_data (abfd);
   loc = s->contents + (s->reloc_count++ * bed->s->sizeof_rela);
   bed->s->swap_reloca_out (abfd, rel, loc);
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH] RISC-V: Segment fault in riscv_elf_append_rela.

[Alan Modra]

On Thu, Mar 09, 2023 at 04:04:46PM +0800, Nelson Chu wrote:
>     * elfnn-riscv.c (riscv_elf_append_rela): Abort when the dynamic section
>     doesn't exist.

It would be nicer to handle this more gracefully by returning an error
to the caller.  An abort isn't much better than a segmentation fault
if this can be triggered by user input.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] RISC-V: Segment fault in riscv_elf_append_rela.

[Nelson Chu]

Thanks for the suggestion :) Seems like there are two better solution for now,

1.  Add the BFD_ASSERT before calling every riscv_elf_append_rela to
make sure the written section exists, and then just return in the
riscv_elf_append_rela if the section doesn't exist, to avoid the
segment fault.  We will know which section is NULL, and where the
ASSERT code is.  Besides that, since parts of the callers already have
the BFD_ASSERT check before calling riscv_elf_append_rela, so this
seems the right way to do it.

2. Report error and return false if the section doesn't exist in the
riscv_elf_append_rela, and then the callers "goto out" to release the
unused data structures in the riscv_elf_relocate_section, or return
false if the caller is riscv_elf_finish_dynamic_symbol.  But it is
hard to report the more detailed errors in the riscv_elf_append_rela,
it is not easy to know which caller code meets the problem.

I think solution 1 should be better than solution 2?

Thanks
Nelson

On Thu, Mar 9, 2023 at 4:34 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Mar 09, 2023 at 04:04:46PM +0800, Nelson Chu wrote:
> >     * elfnn-riscv.c (riscv_elf_append_rela): Abort when the dynamic section
> >     doesn't exist.
>
> It would be nicer to handle this more gracefully by returning an error
> to the caller.  An abort isn't much better than a segmentation fault
> if this can be triggered by user input.
>
> --
> Alan Modra
> Australia Development Lab, IBM

Re: [PATCH] RISC-V: Segment fault in riscv_elf_append_rela.

[Alan Modra]

On Thu, Mar 09, 2023 at 07:41:05PM +0800, Nelson Chu wrote:
> Thanks for the suggestion :) Seems like there are two better solution for now,
> 
> 1.  Add the BFD_ASSERT before calling every riscv_elf_append_rela to
> make sure the written section exists, and then just return in the
> riscv_elf_append_rela if the section doesn't exist, to avoid the
> segment fault.  We will know which section is NULL, and where the
> ASSERT code is.  Besides that, since parts of the callers already have
> the BFD_ASSERT check before calling riscv_elf_append_rela, so this
> seems the right way to do it.
> 
> 2. Report error and return false if the section doesn't exist in the
> riscv_elf_append_rela, and then the callers "goto out" to release the
> unused data structures in the riscv_elf_relocate_section, or return
> false if the caller is riscv_elf_finish_dynamic_symbol.  But it is
> hard to report the more detailed errors in the riscv_elf_append_rela,
> it is not easy to know which caller code meets the problem.
> 
> I think solution 1 should be better than solution 2?

Sorry, I should have read your email more carefully before commenting.
I'm OK with an abort for things that can't happen.  ;-)  Now that I've
read it again, I believe you are saying that there is currently a
mismatch between check_relocs and relocate_section, and I'm assuming
that you intend to fix that problem sometime.  If that is the case,
then the abort in the original patch is OK.

If you don't intend to fix the mismatch, *then* an abort is not OK and
you should instead inform the user via an error message that his code
is unsupported.

> 
> Thanks
> Nelson
> 
> On Thu, Mar 9, 2023 at 4:34 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Thu, Mar 09, 2023 at 04:04:46PM +0800, Nelson Chu wrote:
> > >     * elfnn-riscv.c (riscv_elf_append_rela): Abort when the dynamic section
> > >     doesn't exist.
> >
> > It would be nicer to handle this more gracefully by returning an error
> > to the caller.  An abort isn't much better than a segmentation fault
> > if this can be triggered by user input.
> >
> > --
> > Alan Modra
> > Australia Development Lab, IBM

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] RISC-V: Segment fault in riscv_elf_append_rela.

[Nelson Chu]

Thanks!  Yeah I don't intend to fix the mismatch in the short-term, so
I will choose to use the solution 2, but report a dangerous relocation
for it.  But in the long-term, it would be great if the mismatch
problem can be resolved, so I will also add a comment to mention this
as a TODO.

Thanks again
Nelson

On Thu, Mar 9, 2023 at 8:59 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Mar 09, 2023 at 07:41:05PM +0800, Nelson Chu wrote:
> > Thanks for the suggestion :) Seems like there are two better solution for now,
> >
> > 1.  Add the BFD_ASSERT before calling every riscv_elf_append_rela to
> > make sure the written section exists, and then just return in the
> > riscv_elf_append_rela if the section doesn't exist, to avoid the
> > segment fault.  We will know which section is NULL, and where the
> > ASSERT code is.  Besides that, since parts of the callers already have
> > the BFD_ASSERT check before calling riscv_elf_append_rela, so this
> > seems the right way to do it.
> >
> > 2. Report error and return false if the section doesn't exist in the
> > riscv_elf_append_rela, and then the callers "goto out" to release the
> > unused data structures in the riscv_elf_relocate_section, or return
> > false if the caller is riscv_elf_finish_dynamic_symbol.  But it is
> > hard to report the more detailed errors in the riscv_elf_append_rela,
> > it is not easy to know which caller code meets the problem.
> >
> > I think solution 1 should be better than solution 2?
>
> Sorry, I should have read your email more carefully before commenting.
> I'm OK with an abort for things that can't happen.  ;-)  Now that I've
> read it again, I believe you are saying that there is currently a
> mismatch between check_relocs and relocate_section, and I'm assuming
> that you intend to fix that problem sometime.  If that is the case,
> then the abort in the original patch is OK.
>
> If you don't intend to fix the mismatch, *then* an abort is not OK and
> you should instead inform the user via an error message that his code
> is unsupported.
>
> >
> > Thanks
> > Nelson
> >
> > On Thu, Mar 9, 2023 at 4:34 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Thu, Mar 09, 2023 at 04:04:46PM +0800, Nelson Chu wrote:
> > > >     * elfnn-riscv.c (riscv_elf_append_rela): Abort when the dynamic section
> > > >     doesn't exist.
> > >
> > > It would be nicer to handle this more gracefully by returning an error
> > > to the caller.  An abort isn't much better than a segmentation fault
> > > if this can be triggered by user input.
> > >
> > > --
> > > Alan Modra
> > > Australia Development Lab, IBM
>
> --
> Alan Modra
> Australia Development Lab, IBM