[PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[YunQiang Su]

The asm code like:
	b	8
will emit absolute relocs like:
	R_MIPS_PC16	*ABS*

If they are included into PIC shared objects or PIE executables,
the branch target will be like 0x12340000, which will make the
programs crash.
---
 bfd/elfxx-mips.c                                | 9 +++++++++
 ld/testsuite/ld-mips-elf/mips-elf.exp           | 1 +
 ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d | 5 +++++
 ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s | 2 ++
 4 files changed, 17 insertions(+)
 create mode 100644 ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d
 create mode 100644 ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s

diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index 69dd71419ff..9542250dec4 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -9258,6 +9258,15 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
 		   (h) ? h->root.root.string : "a local symbol");
 	      break;
 	    default:
+	      if (branch_reloc_p (r_type) && r_symndx == STN_UNDEF)
+		{
+		  howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, NEWABI_P (abfd));
+		  info->callbacks->einfo
+		    /* xgettext:c-format */
+		    (_("%X%H: relocation %s against `*ABS*' cannot be used"
+		       " when making a PIC/PIE object\n"),
+		     abfd, sec, rel->r_offset, howto->name);
+		}
 	      break;
 	    }
 	}
diff --git a/ld/testsuite/ld-mips-elf/mips-elf.exp b/ld/testsuite/ld-mips-elf/mips-elf.exp
index 50af78d1430..a8e1b91b3a1 100644
--- a/ld/testsuite/ld-mips-elf/mips-elf.exp
+++ b/ld/testsuite/ld-mips-elf/mips-elf.exp
@@ -1678,6 +1678,7 @@ run_dump_test_o32 "pic-reloc-6"
 run_dump_test_n64 "pic-reloc-7"
 run_dump_test_n64 "pic-reloc-7" [list [list name (microMIPS)] \
 				      [list as "-mmicromips"]]
+run_dump_test "pic-reject-abs-reloc"
 
 run_dump_test_o32 "reloc-pcrel-r6"
 
diff --git a/ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d b/ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d
new file mode 100644
index 00000000000..16d9f4e8553
--- /dev/null
+++ b/ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d
@@ -0,0 +1,5 @@
+#name: MIPS PIC rejects branch absolute
+#ld: -shared -T pic-reloc-absolute-lo.ld
+#target: [check_shared_lib_support]
+#error: \A[^\n]*: in function `foo':\n
+#error:   \(\.text\+0x0\): relocation R_MIPS_PC16 against `\*ABS\*' cannot be used when making a PIC/PIE object
diff --git a/ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s b/ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s
new file mode 100644
index 00000000000..a845fd50a77
--- /dev/null
+++ b/ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s
@@ -0,0 +1,2 @@
+foo:
+	b	8
-- 
2.39.2

Re: [PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[Hans-Peter Nilsson]

A random comment:

On Wed, 21 Feb 2024, YunQiang Su wrote:

> The asm code like:
> 	b	8
> will emit absolute relocs like:
> 	R_MIPS_PC16	*ABS*
> 
> If they are included into PIC shared objects or PIE executables,
> the branch target will be like 0x12340000, which will make the
> programs crash.
> ---
>  bfd/elfxx-mips.c                                | 9 +++++++++
>  ld/testsuite/ld-mips-elf/mips-elf.exp           | 1 +
>  ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d | 5 +++++
>  ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s | 2 ++
>  4 files changed, 17 insertions(+)
>  create mode 100644 ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.d
>  create mode 100644 ld/testsuite/ld-mips-elf/pic-reject-abs-reloc.s
> 
> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 69dd71419ff..9542250dec4 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -9258,6 +9258,15 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
>  		   (h) ? h->root.root.string : "a local symbol");
>  	      break;
>  	    default:
> +	      if (branch_reloc_p (r_type) && r_symndx == STN_UNDEF)
> +		{
> +		  howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, NEWABI_P (abfd));
> +		  info->callbacks->einfo
> +		    /* xgettext:c-format */
> +		    (_("%X%H: relocation %s against `*ABS*' cannot be used"

There's no "*ABS*" in the source and IMHO that'd look confusing 
to innocent users.  How about "...against an absolute value"?  
Or "...against an absolute value or absolute symbol"?  Perhaps 
the latter is a bit too wordy, but also more complete.

> +		       " when making a PIC/PIE object\n"),
> +		     abfd, sec, rel->r_offset, howto->name);
> +		}
>  	      break;

brgds, H-P

Re: [PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[YunQiang Su]

> > +               howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, NEWABI_P (abfd));
> > +               info->callbacks->einfo
> > +                 /* xgettext:c-format */
> > +                 (_("%X%H: relocation %s against `*ABS*' cannot be used"
>
> There's no "*ABS*" in the source and IMHO that'd look confusing
> to innocent users.  How about "...against an absolute value"?
> Or "...against an absolute value or absolute symbol"?  Perhaps
> the latter is a bit too wordy, but also more complete.
>

Good idea. I think that we should use "...against an absolute value".
"absolute symbol"  is not included in this code, as it's r_symndx is
not STN_UNDEF.
An example is gas/testsuite/gas/mips/branch-absolute.s.

Thank you,

Re: [PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[Maciej W. Rozycki]

On Thu, 22 Feb 2024, YunQiang Su wrote:

> > > +               howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, NEWABI_P (abfd));
> > > +               info->callbacks->einfo
> > > +                 /* xgettext:c-format */
> > > +                 (_("%X%H: relocation %s against `*ABS*' cannot be used"
> >
> > There's no "*ABS*" in the source and IMHO that'd look confusing
> > to innocent users.  How about "...against an absolute value"?
> > Or "...against an absolute value or absolute symbol"?  Perhaps
> > the latter is a bit too wordy, but also more complete.
> >
> 
> Good idea. I think that we should use "...against an absolute value".
> "absolute symbol"  is not included in this code, as it's r_symndx is
> not STN_UNDEF.

 Well code says otherwise:

+	      if (branch_reloc_p (r_type) && r_symndx == STN_UNDEF)

did you mean "the relocation's r_symndx is STN_UNDEF"?

 But it can be a relocation against an absolute symbol, for example if the 
symbol referred to by a relocation is supplied by another module in the 
link or a linker script and said symbol is absolute.  This case has to be 
covered in testing as well, as I infer from your code it won't be handled 
correctly.

 In fact this situation is not unique to branch relocations, because no 
PC-relative relocation against an absolute symbol or value can be resolved 
at link time for PIC/PIE links.  So you need to make your code generic for 
any relocations that have the `pc_relative' flag set in their howto unless 
we have a way to make a dynamic relocation out of such a relocation (which 
is none right now, as we still only have R_MIPS_REL32 as the sole dynamic 
relocation, except for TLS stuff).

 Extra test cases will be appreciated if you can come up with ones.

  Maciej

Re: [PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[YunQiang Su]

Maciej W. Rozycki <macro@orcam.me.uk> 于2024年2月22日周四 21:13写道：
>
> On Thu, 22 Feb 2024, YunQiang Su wrote:
>
> > > > +               howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, NEWABI_P (abfd));
> > > > +               info->callbacks->einfo
> > > > +                 /* xgettext:c-format */
> > > > +                 (_("%X%H: relocation %s against `*ABS*' cannot be used"
> > >
> > > There's no "*ABS*" in the source and IMHO that'd look confusing
> > > to innocent users.  How about "...against an absolute value"?
> > > Or "...against an absolute value or absolute symbol"?  Perhaps
> > > the latter is a bit too wordy, but also more complete.
> > >
> >
> > Good idea. I think that we should use "...against an absolute value".
> > "absolute symbol"  is not included in this code, as it's r_symndx is
> > not STN_UNDEF.
>
>  Well code says otherwise:
>
> +             if (branch_reloc_p (r_type) && r_symndx == STN_UNDEF)
>
> did you mean "the relocation's r_symndx is STN_UNDEF"?
>
>  But it can be a relocation against an absolute symbol, for example if the
> symbol referred to by a relocation is supplied by another module in the
> link or a linker script and said symbol is absolute.  This case has to be

I have no idea whether we should emit this error for this case:
   If the symbol is set by a linker script, and is near enough with
our instruction,
   the object should be ok for PIC?
If not near enough, a "relocation truncated to fit" error will emit.

> covered in testing as well, as I infer from your code it won't be handled
> correctly.
>
>  In fact this situation is not unique to branch relocations, because no
> PC-relative relocation against an absolute symbol or value can be resolved
> at link time for PIC/PIE links.  So you need to make your code generic for
> any relocations that have the `pc_relative' flag set in their howto unless
> we have a way to make a dynamic relocation out of such a relocation (which
> is none right now, as we still only have R_MIPS_REL32 as the sole dynamic
> relocation, except for TLS stuff).
>
>  Extra test cases will be appreciated if you can come up with ones.
>
>   Maciej

Re: [PATCH v6] MIPS: Reject branch absolute relocs for PIC for linking

[Maciej W. Rozycki]

On Sun, 25 Feb 2024, YunQiang Su wrote:

> > > Good idea. I think that we should use "...against an absolute value".
> > > "absolute symbol"  is not included in this code, as it's r_symndx is
> > > not STN_UNDEF.
> >
> >  Well code says otherwise:
> >
> > +             if (branch_reloc_p (r_type) && r_symndx == STN_UNDEF)
> >
> > did you mean "the relocation's r_symndx is STN_UNDEF"?
> >
> >  But it can be a relocation against an absolute symbol, for example if the
> > symbol referred to by a relocation is supplied by another module in the
> > link or a linker script and said symbol is absolute.  This case has to be
> 
> I have no idea whether we should emit this error for this case:
>    If the symbol is set by a linker script, and is near enough with
> our instruction,
>    the object should be ok for PIC?
> If not near enough, a "relocation truncated to fit" error will emit.

 If the symbol referred is absolute, then the case is no different from an 
absolute relocation.  You just can't calculate a PC-relative reference to 
an absolute value at static link time, because the distance from PC to 
that value will depend on the base address at load time.  It is different 
from a reference to a regular (non-absolute) symbol that just turns out 
too distant for a PC-relative relocation to reach (where the "relocation 
truncated to fit" case applies).

  Maciej