inconsistencies if (at least) ELF relocation handling

Message ID 2a1cc06c-82e2-a489-577f-cbf6a6d407ae@suse.com
State New
Headers show
Series
  • inconsistencies if (at least) ELF relocation handling
Related show

Commit Message

Luis Machado via Binutils April 19, 2021, 9:05 a.m.
All,

while further learning how exactly bfd processes relocations, I
came to notice that even ELF RELA relocations have section contents
read and whatever was found added into the new value to be stored
(which I take to mean that there is an assumption that these fields
are emitted as all zero by whichever tool creates the object files,
which even with gas can be easily violated using the .reloc pseudo).
The comment next to struct reloc_howto_struct's partial_inplace field
suggests that this is to be avoided by setting src_mask to zero. I
can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,
and Arm64 don't (I've looked at just the architectures that I'm at
least remotely familiar with). Any thoughts about fixing this (Cc-ing
the specific targets' maintainers for this reason)?

However, even if src_mask was zero for all targets using only RELA
relocations, there's then still an asymmetry in overflow checking:
REL relocs, having their addends read out of section contents by e.g.
_bfd_relocate_contents(), have overflow from adding in this addend
properly checked. RELA relocs, having their addends added in already
in e.g. _bfd_final_link_relocate(), don't. I've started putting
together a patch (appended at the end of the mail), which is both
incomplete (neither adjusting target-specific callers of
_bfd_relocate_contents() yet, nor adjusting further callers of
read_reloc() so far) and breaking at least x86-64 (see below). I'd
still like to get input on what people think a proper approach would
be here.

As to the breakage on x86-64, I observe a number of "relocation
truncated to fit: R_X86_64_32 against `.debug_str'", due to negative
addends getting truncated to 32-bit unsigned values and this reloc
using complain_overflow_unsigned. In ELF it is my understanding that
addends - no matter what their origin - are always signed. For this
case the present checking done for complain_overflow_unsigned looks
wrong to me. But of course there may be object formats where unsigned
addends might be used for at least some relocation types, so I'm
hesitant to change the checking logic itself, and I'm instead
wondering whether yet another complain_overflow_* type may be needed.
Otoh there are only very few uses of the type outside of bfd/elf*.c.

Jan

Comments

Luis Machado via Binutils April 19, 2021, 5:34 p.m. | #1
On Mon, Apr 19, 2021 at 2:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> All,

>

> while further learning how exactly bfd processes relocations, I

> came to notice that even ELF RELA relocations have section contents

> read and whatever was found added into the new value to be stored

> (which I take to mean that there is an assumption that these fields

> are emitted as all zero by whichever tool creates the object files,

> which even with gas can be easily violated using the .reloc pseudo).

> The comment next to struct reloc_howto_struct's partial_inplace field

> suggests that this is to be avoided by setting src_mask to zero. I


I have noticed this issue on x8-64.

> can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,

> and Arm64 don't (I've looked at just the architectures that I'm at

> least remotely familiar with). Any thoughts about fixing this (Cc-ing

> the specific targets' maintainers for this reason)?

>

> However, even if src_mask was zero for all targets using only RELA

> relocations, there's then still an asymmetry in overflow checking:

> REL relocs, having their addends read out of section contents by e.g.

> _bfd_relocate_contents(), have overflow from adding in this addend

> properly checked. RELA relocs, having their addends added in already

> in e.g. _bfd_final_link_relocate(), don't. I've started putting

> together a patch (appended at the end of the mail), which is both

> incomplete (neither adjusting target-specific callers of

> _bfd_relocate_contents() yet, nor adjusting further callers of

> read_reloc() so far) and breaking at least x86-64 (see below). I'd

> still like to get input on what people think a proper approach would

> be here.

>

> As to the breakage on x86-64, I observe a number of "relocation

> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative

> addends getting truncated to 32-bit unsigned values and this reloc

> using complain_overflow_unsigned. In ELF it is my understanding that

> addends - no matter what their origin - are always signed. For this

> case the present checking done for complain_overflow_unsigned looks


The addend is signed.  But the result of R_X86_64_32 relocation is
unsigned.  complain_overflow_unsigned should handle it properly.

> wrong to me. But of course there may be object formats where unsigned

> addends might be used for at least some relocation types, so I'm

> hesitant to change the checking logic itself, and I'm instead

> wondering whether yet another complain_overflow_* type may be needed.

> Otoh there are only very few uses of the type outside of bfd/elf*.c.

>

> Jan

>

> --- a/bfd/cofflink.c

> +++ b/bfd/cofflink.c

> @@ -2822,7 +2822,7 @@ _bfd_coff_reloc_link_order (bfd *output_

>        if (buf == NULL && size != 0)

>         return FALSE;

>

> -      rstat = _bfd_relocate_contents (howto, output_bfd,

> +      rstat = _bfd_relocate_contents (howto, output_bfd, 0,

>                                       (bfd_vma) link_order->u.reloc.p->addend,

>                                       buf);

>        switch (rstat)

> --- a/bfd/ecoff.c

> +++ b/bfd/ecoff.c

> @@ -3953,7 +3953,7 @@ ecoff_reloc_link_order (bfd *output_bfd,

>        buf = (bfd_byte *) bfd_zmalloc (size);

>        if (buf == NULL && size != 0)

>         return FALSE;

> -      rstat = _bfd_relocate_contents (rel.howto, output_bfd,

> +      rstat = _bfd_relocate_contents (rel.howto, output_bfd, 0,

>                                       (bfd_vma) addend, buf);

>        switch (rstat)

>         {

> --- a/bfd/elf32-i386.c

> +++ b/bfd/elf32-i386.c

> @@ -130,7 +130,7 @@ static reloc_howto_type elf_howto_table[

>    HOWTO(R_386_TLS_GOTDESC, 0, 2, 32, FALSE, 0, complain_overflow_dont,

>         bfd_elf_generic_reloc, "R_386_TLS_GOTDESC",

>         TRUE, 0xffffffff, 0xffffffff, FALSE),

> -  HOWTO(R_386_TLS_DESC_CALL, 0, 0, 0, FALSE, 0, complain_overflow_dont,

> +  HOWTO(R_386_TLS_DESC_CALL, 0, 3, 0, FALSE, 0, complain_overflow_dont,

>         bfd_elf_generic_reloc, "R_386_TLS_DESC_CALL",

>         FALSE, 0, 0, FALSE),

>    HOWTO(R_386_TLS_DESC, 0, 2, 32, FALSE, 0, complain_overflow_dont,

> --- a/bfd/elf64-x86-64.c

> +++ b/bfd/elf64-x86-64.c

> @@ -146,7 +146,7 @@ static reloc_howto_type x86_64_elf_howto

>         complain_overflow_bitfield, bfd_elf_generic_reloc,

>         "R_X86_64_GOTPC32_TLSDESC",

>         FALSE, 0xffffffff, 0xffffffff, TRUE),

> -  HOWTO(R_X86_64_TLSDESC_CALL, 0, 0, 0, FALSE, 0,

> +  HOWTO(R_X86_64_TLSDESC_CALL, 0, 3, 0, FALSE, 0,

>         complain_overflow_dont, bfd_elf_generic_reloc,

>         "R_X86_64_TLSDESC_CALL",

>         FALSE, 0, 0, FALSE),

> --- a/bfd/elflink.c

> +++ b/bfd/elflink.c

> @@ -11839,7 +11839,7 @@ elf_reloc_link_order (bfd *output_bfd,

>        buf = (bfd_byte *) bfd_zmalloc (size);

>        if (buf == NULL && size != 0)

>         return FALSE;

> -      rstat = _bfd_relocate_contents (howto, output_bfd, addend, buf);

> +      rstat = _bfd_relocate_contents (howto, output_bfd, 0, addend, buf);

>        switch (rstat)

>         {

>         case bfd_reloc_ok:

> --- a/bfd/libbfd-in.h

> +++ b/bfd/libbfd-in.h

> @@ -696,7 +696,8 @@ extern bfd_reloc_status_type _bfd_final_

>

>  /* Relocate a particular location by a howto and a value.  */

>  extern bfd_reloc_status_type _bfd_relocate_contents

> -  (reloc_howto_type *, bfd *, bfd_vma, bfd_byte *) ATTRIBUTE_HIDDEN;

> +  (reloc_howto_type *, bfd *, bfd_vma, bfd_signed_vma, bfd_byte *)

> +  ATTRIBUTE_HIDDEN;

>

>  /* Clear a given location using a given howto.  */

>  extern bfd_reloc_status_type _bfd_clear_contents

> --- a/bfd/libbfd.h

> +++ b/bfd/libbfd.h

> @@ -701,7 +701,8 @@ extern bfd_reloc_status_type _bfd_final_

>

>  /* Relocate a particular location by a howto and a value.  */

>  extern bfd_reloc_status_type _bfd_relocate_contents

> -  (reloc_howto_type *, bfd *, bfd_vma, bfd_byte *) ATTRIBUTE_HIDDEN;

> +  (reloc_howto_type *, bfd *, bfd_vma, bfd_signed_vma, bfd_byte *)

> +  ATTRIBUTE_HIDDEN;

>

>  /* Clear a given location using a given howto.  */

>  extern bfd_reloc_status_type _bfd_clear_contents

> --- a/bfd/linker.c

> +++ b/bfd/linker.c

> @@ -2393,7 +2393,7 @@ _bfd_generic_reloc_link_order (bfd *abfd

>        buf = (bfd_byte *) bfd_zmalloc (size);

>        if (buf == NULL && size != 0)

>         return FALSE;

> -      rstat = _bfd_relocate_contents (r->howto, abfd,

> +      rstat = _bfd_relocate_contents (r->howto, abfd, 0,

>                                       (bfd_vma) link_order->u.reloc.p->addend,

>                                       buf);

>        switch (rstat)

> --- a/bfd/reloc.c

> +++ b/bfd/reloc.c

> @@ -1355,7 +1355,12 @@ _bfd_final_link_relocate (reloc_howto_ty

>                           bfd_vma value,

>                           bfd_vma addend)

>  {

> -  bfd_vma relocation;

> +  /* This function assumes that we are dealing with a basic relocation

> +     against a symbol.  We want to compute the value of the symbol to

> +     relocate to.  This is just VALUE, the value of the symbol, plus

> +     ADDEND, any addend associated with the reloc (which will get added

> +     in by _bfd_relocate_contents()).  */

> +  bfd_vma relocation = value;

>    bfd_size_type octets = (address

>                           * bfd_octets_per_byte (input_bfd, input_section));

>

> @@ -1363,12 +1368,6 @@ _bfd_final_link_relocate (reloc_howto_ty

>    if (!bfd_reloc_offset_in_range (howto, input_bfd, input_section, octets))

>      return bfd_reloc_outofrange;

>

> -  /* This function assumes that we are dealing with a basic relocation

> -     against a symbol.  We want to compute the value of the symbol to

> -     relocate to.  This is just VALUE, the value of the symbol, plus

> -     ADDEND, any addend associated with the reloc.  */

> -  relocation = value + addend;

> -

>    /* If the relocation is PC relative, we want to set RELOCATION to

>       the distance between the symbol (currently in RELOCATION) and the

>       location we are relocating.  Some targets (e.g., i386-aout)

> @@ -1387,7 +1386,7 @@ _bfd_final_link_relocate (reloc_howto_ty

>         relocation -= address;

>      }

>

> -  return _bfd_relocate_contents (howto, input_bfd, relocation,

> +  return _bfd_relocate_contents (howto, input_bfd, relocation, addend,

>                                  contents + octets);

>  }

>

> @@ -1397,6 +1396,7 @@ bfd_reloc_status_type

>  _bfd_relocate_contents (reloc_howto_type *howto,

>                         bfd *input_bfd,

>                         bfd_vma relocation,

> +                       bfd_signed_vma addend,

>                         bfd_byte *location)

>  {

>    bfd_vma x;

> @@ -1404,12 +1404,18 @@ _bfd_relocate_contents (reloc_howto_type

>    unsigned int rightshift = howto->rightshift;

>    unsigned int bitpos = howto->bitpos;

>

> +  if (howto->partial_inplace)

> +    {

> +      relocation += addend;

> +      /* Get the value we are going to relocate.  */

> +      x = read_reloc (input_bfd, location, howto);

> +    }

> +  else

> +    x = addend;

> +

>    if (howto->negate)

>      relocation = -relocation;

>

> -  /* Get the value we are going to relocate.  */

> -  x = read_reloc (input_bfd, location, howto);

> -

>    /* Check for overflow.  FIXME: We may drop bits during the addition

>       which we don't check for.  We must either check at every single

>       operation, which would be tedious, or we must do the computations

> --- a/bfd/xcofflink.c

> +++ b/bfd/xcofflink.c

> @@ -5777,7 +5777,7 @@ xcoff_reloc_link_order (bfd *output_bfd,

>        if (buf == NULL && size != 0)

>         return FALSE;

>

> -      rstat = _bfd_relocate_contents (howto, output_bfd, addend, buf);

> +      rstat = _bfd_relocate_contents (howto, output_bfd, 0, addend, buf);

>        switch (rstat)

>         {

>         case bfd_reloc_ok:




-- 
H.J.
Luis Machado via Binutils April 20, 2021, 10:47 a.m. | #2
On 19.04.2021 19:34, H.J. Lu wrote:
> On Mon, Apr 19, 2021 at 2:05 AM Jan Beulich <jbeulich@suse.com> wrote:

>> while further learning how exactly bfd processes relocations, I

>> came to notice that even ELF RELA relocations have section contents

>> read and whatever was found added into the new value to be stored

>> (which I take to mean that there is an assumption that these fields

>> are emitted as all zero by whichever tool creates the object files,

>> which even with gas can be easily violated using the .reloc pseudo).

>> The comment next to struct reloc_howto_struct's partial_inplace field

>> suggests that this is to be avoided by setting src_mask to zero. I

> 

> I have noticed this issue on x8-64.


IOW you wouldn't mind correcting this? Of course I can't tell yet what
amount of testsuite fallout there would be. But presumably not much,
as binutils themselves (with the exception of .reloc, which I'd likely
use for a new testcase here) ought to zero-fill such fields. Of course,
if as the result gas also zeroes the fields targeted by .reloc, an ld
test may be more difficult to invent. (I have to admit though that I'm
not certain yet whether I'd consider gas leaving such fields non-zero
a bug or intended behavior. As per the spec there's nothing wrong with
them being non-zero in relocatable files while a rela relocation is
still attached to them.)

>> can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,

>> and Arm64 don't (I've looked at just the architectures that I'm at

>> least remotely familiar with). Any thoughts about fixing this (Cc-ing

>> the specific targets' maintainers for this reason)?

>>

>> However, even if src_mask was zero for all targets using only RELA

>> relocations, there's then still an asymmetry in overflow checking:

>> REL relocs, having their addends read out of section contents by e.g.

>> _bfd_relocate_contents(), have overflow from adding in this addend

>> properly checked. RELA relocs, having their addends added in already

>> in e.g. _bfd_final_link_relocate(), don't. I've started putting

>> together a patch (appended at the end of the mail), which is both

>> incomplete (neither adjusting target-specific callers of

>> _bfd_relocate_contents() yet, nor adjusting further callers of

>> read_reloc() so far) and breaking at least x86-64 (see below). I'd

>> still like to get input on what people think a proper approach would

>> be here.

>>

>> As to the breakage on x86-64, I observe a number of "relocation

>> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative

>> addends getting truncated to 32-bit unsigned values and this reloc

>> using complain_overflow_unsigned. In ELF it is my understanding that

>> addends - no matter what their origin - are always signed. For this

>> case the present checking done for complain_overflow_unsigned looks

> 

> The addend is signed.  But the result of R_X86_64_32 relocation is

> unsigned.  complain_overflow_unsigned should handle it properly.


Well, the question is what "unsigned" in complain_overflow_unsigned
means: Unsigned arithmetic, or an unsigned result. And hence whether it
wants fixing or a new type introducing.

Jan
Luis Machado via Binutils April 20, 2021, noon | #3
On Tue, Apr 20, 2021 at 3:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 19.04.2021 19:34, H.J. Lu wrote:

> > On Mon, Apr 19, 2021 at 2:05 AM Jan Beulich <jbeulich@suse.com> wrote:

> >> while further learning how exactly bfd processes relocations, I

> >> came to notice that even ELF RELA relocations have section contents

> >> read and whatever was found added into the new value to be stored

> >> (which I take to mean that there is an assumption that these fields

> >> are emitted as all zero by whichever tool creates the object files,

> >> which even with gas can be easily violated using the .reloc pseudo).

> >> The comment next to struct reloc_howto_struct's partial_inplace field

> >> suggests that this is to be avoided by setting src_mask to zero. I

> >

> > I have noticed this issue on x8-64.

>

> IOW you wouldn't mind correcting this? Of course I can't tell yet what

> amount of testsuite fallout there would be. But presumably not much,

> as binutils themselves (with the exception of .reloc, which I'd likely

> use for a new testcase here) ought to zero-fill such fields. Of course,

> if as the result gas also zeroes the fields targeted by .reloc, an ld

> test may be more difficult to invent. (I have to admit though that I'm

> not certain yet whether I'd consider gas leaving such fields non-zero

> a bug or intended behavior. As per the spec there's nothing wrong with

> them being non-zero in relocatable files while a rela relocation is

> still attached to them.)


I think I tried to fix it once.  But I ran into so many issues and gave it up
in the end.

> >> can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,

> >> and Arm64 don't (I've looked at just the architectures that I'm at

> >> least remotely familiar with). Any thoughts about fixing this (Cc-ing

> >> the specific targets' maintainers for this reason)?

> >>

> >> However, even if src_mask was zero for all targets using only RELA

> >> relocations, there's then still an asymmetry in overflow checking:

> >> REL relocs, having their addends read out of section contents by e.g.

> >> _bfd_relocate_contents(), have overflow from adding in this addend

> >> properly checked. RELA relocs, having their addends added in already

> >> in e.g. _bfd_final_link_relocate(), don't. I've started putting

> >> together a patch (appended at the end of the mail), which is both

> >> incomplete (neither adjusting target-specific callers of

> >> _bfd_relocate_contents() yet, nor adjusting further callers of

> >> read_reloc() so far) and breaking at least x86-64 (see below). I'd

> >> still like to get input on what people think a proper approach would

> >> be here.

> >>

> >> As to the breakage on x86-64, I observe a number of "relocation

> >> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative

> >> addends getting truncated to 32-bit unsigned values and this reloc

> >> using complain_overflow_unsigned. In ELF it is my understanding that

> >> addends - no matter what their origin - are always signed. For this

> >> case the present checking done for complain_overflow_unsigned looks

> >

> > The addend is signed.  But the result of R_X86_64_32 relocation is

> > unsigned.  complain_overflow_unsigned should handle it properly.

>

> Well, the question is what "unsigned" in complain_overflow_unsigned

> means: Unsigned arithmetic, or an unsigned result. And hence whether it

> wants fixing or a new type introducing.

>


I think it should be unsigned result.

-- 
H.J.
Luis Machado via Binutils April 22, 2021, 11:15 a.m. | #4
On 20.04.2021 14:00, H.J. Lu wrote:
> On Tue, Apr 20, 2021 at 3:47 AM Jan Beulich <jbeulich@suse.com> wrote:

>> On 19.04.2021 19:34, H.J. Lu wrote:

>>> On Mon, Apr 19, 2021 at 2:05 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>> while further learning how exactly bfd processes relocations, I

>>>> came to notice that even ELF RELA relocations have section contents

>>>> read and whatever was found added into the new value to be stored

>>>> (which I take to mean that there is an assumption that these fields

>>>> are emitted as all zero by whichever tool creates the object files,

>>>> which even with gas can be easily violated using the .reloc pseudo).

>>>> The comment next to struct reloc_howto_struct's partial_inplace field

>>>> suggests that this is to be avoided by setting src_mask to zero. I

>>>

>>> I have noticed this issue on x8-64.

>>

>> IOW you wouldn't mind correcting this? Of course I can't tell yet what

>> amount of testsuite fallout there would be. But presumably not much,

>> as binutils themselves (with the exception of .reloc, which I'd likely

>> use for a new testcase here) ought to zero-fill such fields. Of course,

>> if as the result gas also zeroes the fields targeted by .reloc, an ld

>> test may be more difficult to invent. (I have to admit though that I'm

>> not certain yet whether I'd consider gas leaving such fields non-zero

>> a bug or intended behavior. As per the spec there's nothing wrong with

>> them being non-zero in relocatable files while a rela relocation is

>> still attached to them.)

> 

> I think I tried to fix it once.  But I ran into so many issues and gave it up

> in the end.


At least there's no testsuite fallout at all. Of course I'm yet to create
a testcase for it, and it sits on top of quite a few further patches, so
it may take a little while until I get to submit the change ...

Jan
Luis Machado via Binutils May 4, 2021, 12:27 a.m. | #5
On Mon, Apr 19, 2021 at 11:05:19AM +0200, Jan Beulich via Binutils wrote:
> The comment next to struct reloc_howto_struct's partial_inplace field

> suggests that this is to be avoided by setting src_mask to zero.


Yes, the point of RELA relocations was to move addends out of the
section contents into the relocation.  So src_mask should generally be
zero for RELA.  The ELF gABI wording under "Relocation" supports this
too, talking about addends being "either from the field to be
relocated or from the addend field contained in the relocation
record".  It doesn't say "and/or".  However, when a target applies
multiple relocations to the same r_offset where the addend
accumulates, a target might accumulate such addends in the section
contents.  (Which isn't exactly correct, the accumulation ought to be
done at full word width not the width of the field.)  Such relocs
might need src_mask equal to dst_mask.

> can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,

> and Arm64 don't (I've looked at just the architectures that I'm at

> least remotely familiar with). Any thoughts about fixing this (Cc-ing

> the specific targets' maintainers for this reason)?


Correcting src_mask should be left to target maintainers.  The target
might have broken object files.

> However, even if src_mask was zero for all targets using only RELA

> relocations, there's then still an asymmetry in overflow checking:

> REL relocs, having their addends read out of section contents by e.g.

> _bfd_relocate_contents(), have overflow from adding in this addend

> properly checked. RELA relocs, having their addends added in already

> in e.g. _bfd_final_link_relocate(), don't. I've started putting

> together a patch (appended at the end of the mail), which is both

> incomplete (neither adjusting target-specific callers of

> _bfd_relocate_contents() yet, nor adjusting further callers of

> read_reloc() so far) and breaking at least x86-64 (see below). I'd

> still like to get input on what people think a proper approach would

> be here.


Yes, you're very likely going to expose a lot of target bugs if you
correct RELA overflow checking.  I tried something similar a long time
ago and decided the pain wasn't worth it.  Don't let me stop you
poking at it though, you've successfully taken on some tricky jobs
before.

> As to the breakage on x86-64, I observe a number of "relocation

> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative

> addends getting truncated to 32-bit unsigned values and this reloc

> using complain_overflow_unsigned. In ELF it is my understanding that

> addends - no matter what their origin - are always signed.


The ELF gABI does tend to give that idea by specifying r_addend as
signed, but I believe there are relocations that only allow positive
addends.  Processor ABIs are free to specify relocations like that..

> For this

> case the present checking done for complain_overflow_unsigned looks

> wrong to me. But of course there may be object formats where unsigned

> addends might be used for at least some relocation types, so I'm

> hesitant to change the checking logic itself, and I'm instead

> wondering whether yet another complain_overflow_* type may be needed.

> Otoh there are only very few uses of the type outside of bfd/elf*.c.


-- 
Alan Modra
Australia Development Lab, IBM
Luis Machado via Binutils May 4, 2021, 6:49 a.m. | #6
On 04.05.2021 02:27, Alan Modra wrote:
> On Mon, Apr 19, 2021 at 11:05:19AM +0200, Jan Beulich via Binutils wrote:

>> The comment next to struct reloc_howto_struct's partial_inplace field

>> suggests that this is to be avoided by setting src_mask to zero.

> 

> Yes, the point of RELA relocations was to move addends out of the

> section contents into the relocation.  So src_mask should generally be

> zero for RELA.  The ELF gABI wording under "Relocation" supports this

> too, talking about addends being "either from the field to be

> relocated or from the addend field contained in the relocation

> record".  It doesn't say "and/or".  However, when a target applies

> multiple relocations to the same r_offset where the addend

> accumulates, a target might accumulate such addends in the section

> contents.  (Which isn't exactly correct, the accumulation ought to be

> done at full word width not the width of the field.)  Such relocs

> might need src_mask equal to dst_mask.


But such multiple relocations to the same r_offset are spec compliant
only with REL, not with RELA, as per my understanding. (Whether
accumulation is okay to happen at less than full width imo depends on
whether intermediate values will be okay to be rejected when
overflowing, even if the final value would fit.)

>> can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,

>> and Arm64 don't (I've looked at just the architectures that I'm at

>> least remotely familiar with). Any thoughts about fixing this (Cc-ing

>> the specific targets' maintainers for this reason)?

> 

> Correcting src_mask should be left to target maintainers.  The target

> might have broken object files.

> 

>> However, even if src_mask was zero for all targets using only RELA

>> relocations, there's then still an asymmetry in overflow checking:

>> REL relocs, having their addends read out of section contents by e.g.

>> _bfd_relocate_contents(), have overflow from adding in this addend

>> properly checked. RELA relocs, having their addends added in already

>> in e.g. _bfd_final_link_relocate(), don't. I've started putting

>> together a patch (appended at the end of the mail), which is both

>> incomplete (neither adjusting target-specific callers of

>> _bfd_relocate_contents() yet, nor adjusting further callers of

>> read_reloc() so far) and breaking at least x86-64 (see below). I'd

>> still like to get input on what people think a proper approach would

>> be here.

> 

> Yes, you're very likely going to expose a lot of target bugs if you

> correct RELA overflow checking.  I tried something similar a long time

> ago and decided the pain wasn't worth it.


Perhaps then have a "compliant" mode which targets can opt-in to
(long term this might then become an opt-out)?

>> As to the breakage on x86-64, I observe a number of "relocation

>> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative

>> addends getting truncated to 32-bit unsigned values and this reloc

>> using complain_overflow_unsigned. In ELF it is my understanding that

>> addends - no matter what their origin - are always signed.

> 

> The ELF gABI does tend to give that idea by specifying r_addend as

> signed, but I believe there are relocations that only allow positive

> addends.  Processor ABIs are free to specify relocations like that..


Interesting. I would assume such targets then ought to handle such
"non-standard" (I'm inclined to even say "non-conforming") relocations
locally, rather than requiring generic code to cover for this (and
break other targets)? Of course otoh I'm unconvinced R_X86_64_32 using
complain_overflow_unsigned is correct either. Imo it ought to be
complain_overflow_bitfield.

Jan
Michael Matz May 4, 2021, 3:19 p.m. | #7
Hello,

On Tue, 4 May 2021, Jan Beulich via Binutils wrote:

> > Yes, the point of RELA relocations was to move addends out of the

> > section contents into the relocation.  So src_mask should generally be

> > zero for RELA.  The ELF gABI wording under "Relocation" supports this

> > too, talking about addends being "either from the field to be

> > relocated or from the addend field contained in the relocation

> > record".  It doesn't say "and/or".  However, when a target applies

> > multiple relocations to the same r_offset where the addend

> > accumulates, a target might accumulate such addends in the section

> > contents.  (Which isn't exactly correct, the accumulation ought to be

> > done at full word width not the width of the field.)  Such relocs

> > might need src_mask equal to dst_mask.

> 

> But such multiple relocations to the same r_offset are spec compliant

> only with REL, not with RELA, as per my understanding.


Even if the gABI would say so, that would be surprising; there's no reason 
for such restriction.  But the gABI doesn't say so, it calls such 
relocations "composed" and specifies how the calculations proceeds:

-------------------
If multiple consecutive relocation records are applied to the same 
relocation location (r_offset), they are composed instead of being applied 
independently, as described above. By consecutive, we mean that the 
relocation records are contiguous within a single relocation section. By 
composed, we mean that the standard application described above is 
modified as follows:

In all but the last relocation operation of a composed sequence, the 
result of the relocation expression is retained, rather than having part 
extracted and placed in the relocated field. The result is retained at 
full pointer precision of the applicable ABI processor supplement.
In all but the first relocation operation of a composed sequence, the 
addend used is the retained result of the previous relocation operation, 
rather than that implied by the relocation type.
-------------------

So, accumulation in full-word width, addend only matters for the first of 
a sequence of consecutive composed relocs.

Obviously the reality for specific psABIs might be different from that 
ideal, but it seems a sensible model.

> (Whether accumulation is okay to happen at less than full width imo 

> depends on whether intermediate values will be okay to be rejected when 

> overflowing, even if the final value would fit.)


See above.

> Perhaps then have a "compliant" mode which targets can opt-in to (long 

> term this might then become an opt-out)?

> 

> >> As to the breakage on x86-64, I observe a number of "relocation 

> >> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative 

> >> addends getting truncated to 32-bit unsigned values and this reloc 

> >> using complain_overflow_unsigned. In ELF it is my understanding that 

> >> addends - no matter what their origin - are always signed.

> > 

> > The ELF gABI does tend to give that idea by specifying r_addend as 

> > signed, but I believe there are relocations that only allow positive 

> > addends.  Processor ABIs are free to specify relocations like that..

> 

> Interesting. I would assume such targets then ought to handle such 

> "non-standard" (I'm inclined to even say "non-conforming") relocations 

> locally, rather than requiring generic code to cover for this (and break 

> other targets)?


As calculations are word-width and r_addend is always word-width,
why would its signedness matter?  The same bit pattern results, which then 
needs to be interpreted according to the meaning of the relocation.  The 
value V is basically an word, and if it's a signed 32bit reloc, then
  V == (word)(int32_t)(uint32_t)V
needs to hold, and if it's a unsigned 32bit reloc then
  V == (word)(uint32_t)V
needs to hold.

For REL ABIs the signedness of addend does matter of course for how to 
extract A from the relocated fields, but eventually you arrive at a 
word-width A, at which point signedness again doesn't matter.

> Of course otoh I'm unconvinced R_X86_64_32 using 

> complain_overflow_unsigned is correct either. Imo it ought to be 

> complain_overflow_bitfield.



Ciao,
Michael.
Luis Machado via Binutils May 4, 2021, 11:32 p.m. | #8
On Tue, May 04, 2021 at 08:49:54AM +0200, Jan Beulich wrote:
> But such multiple relocations to the same r_offset are spec compliant

> only with REL, not with RELA, as per my understanding.


My reading of the spec says RELA is supported too.

> >> As to the breakage on x86-64, I observe a number of "relocation

> >> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative

> >> addends getting truncated to 32-bit unsigned values and this reloc

> >> using complain_overflow_unsigned. In ELF it is my understanding that

> >> addends - no matter what their origin - are always signed.

> > 

> > The ELF gABI does tend to give that idea by specifying r_addend as

> > signed, but I believe there are relocations that only allow positive

> > addends.  Processor ABIs are free to specify relocations like that..

> 

> Interesting. I would assume such targets then ought to handle such

> "non-standard" (I'm inclined to even say "non-conforming") relocations

> locally, rather than requiring generic code to cover for this (and

> break other targets)?


I guess the question only matters for REL targets where the addend
is read from section contents.  For complain_overflow_bitfield and
complain_overflow_signed _bfd_relocate_contents currently sign extends
the field read from section contents.  For complain_overflow_unsigned
_bfd_relocate_contents treats the field as unsigned.  That makes some
sense since complain_overflow_unsigned is used with relocations for
instructions that have an unsigned offset field.  So the field really
is unsigned in a final linked object.  However, that doesn't allow
negative addends in relocatable object files.  What breaks if you
change the semantics?  There aren't that many targets that are REL and
use complain_overflow_unsigned.

> Of course otoh I'm unconvinced R_X86_64_32 using

> complain_overflow_unsigned is correct either. Imo it ought to be

> complain_overflow_bitfield.

> 

> Jan


-- 
Alan Modra
Australia Development Lab, IBM
Luis Machado via Binutils May 5, 2021, 7:33 a.m. | #9
On 04.05.2021 17:19, Michael Matz wrote:
> On Tue, 4 May 2021, Jan Beulich via Binutils wrote:

>>> Yes, the point of RELA relocations was to move addends out of the

>>> section contents into the relocation.  So src_mask should generally be

>>> zero for RELA.  The ELF gABI wording under "Relocation" supports this

>>> too, talking about addends being "either from the field to be

>>> relocated or from the addend field contained in the relocation

>>> record".  It doesn't say "and/or".  However, when a target applies

>>> multiple relocations to the same r_offset where the addend

>>> accumulates, a target might accumulate such addends in the section

>>> contents.  (Which isn't exactly correct, the accumulation ought to be

>>> done at full word width not the width of the field.)  Such relocs

>>> might need src_mask equal to dst_mask.

>>

>> But such multiple relocations to the same r_offset are spec compliant

>> only with REL, not with RELA, as per my understanding.

> 

> Even if the gABI would say so, that would be surprising; there's no reason 

> for such restriction.  But the gABI doesn't say so, it calls such 

> relocations "composed" and specifies how the calculations proceeds:

> 

> -------------------

> If multiple consecutive relocation records are applied to the same 

> relocation location (r_offset), they are composed instead of being applied 

> independently, as described above. By consecutive, we mean that the 

> relocation records are contiguous within a single relocation section. By 

> composed, we mean that the standard application described above is 

> modified as follows:

> 

> In all but the last relocation operation of a composed sequence, the 

> result of the relocation expression is retained, rather than having part 

> extracted and placed in the relocated field. The result is retained at 

> full pointer precision of the applicable ABI processor supplement.

> In all but the first relocation operation of a composed sequence, the 

> addend used is the retained result of the previous relocation operation, 

> rather than that implied by the relocation type.

> -------------------

> 

> So, accumulation in full-word width, addend only matters for the first of 

> a sequence of consecutive composed relocs.


Oh, looks like I had forgotten about this addition to gABI. Now that
you quote it, I recall having come across it. But somehow I had this
still recorded as non-standard extension.

>>>> As to the breakage on x86-64, I observe a number of "relocation 

>>>> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative 

>>>> addends getting truncated to 32-bit unsigned values and this reloc 

>>>> using complain_overflow_unsigned. In ELF it is my understanding that 

>>>> addends - no matter what their origin - are always signed.

>>>

>>> The ELF gABI does tend to give that idea by specifying r_addend as 

>>> signed, but I believe there are relocations that only allow positive 

>>> addends.  Processor ABIs are free to specify relocations like that..

>>

>> Interesting. I would assume such targets then ought to handle such 

>> "non-standard" (I'm inclined to even say "non-conforming") relocations 

>> locally, rather than requiring generic code to cover for this (and break 

>> other targets)?

> 

> As calculations are word-width and r_addend is always word-width,

> why would its signedness matter?  The same bit pattern results, which then 

> needs to be interpreted according to the meaning of the relocation.  The 

> value V is basically an word, and if it's a signed 32bit reloc, then

>   V == (word)(int32_t)(uint32_t)V

> needs to hold, and if it's a unsigned 32bit reloc then

>   V == (word)(uint32_t)V

> needs to hold.


And what do you suggest R_X86_64_32 is? Signed? Unsigned? In

    mov     $val, %eax

you can't infer the signedness of val. Granted R_X86_64_32 may not
be an ideal example, as there's also R_X86_64_32S. Yet for the
example above the assembler can't know which of the two to pick if
R_X86_64_32 was strictly unsigned. Hence I would think R_X86_64_32S
needs to be treated as strictly signed, but R_X86_64_32 should
allow for the full range of signed and unsigned 32-bit values (i.e.
complain_overflow_bitfield), contrary to what the psABI currently
says.

Interestingly these two relocations are the only ones where the
psABI talks about sign-/zero-extension, i.e. the "signedness" of a
relocation type.

Jan
Michael Matz May 5, 2021, 1:36 p.m. | #10
Hello,

On Wed, 5 May 2021, Jan Beulich wrote:

> > As calculations are word-width and r_addend is always word-width,

> > why would its signedness matter?  The same bit pattern results, which then 

> > needs to be interpreted according to the meaning of the relocation.  The 

> > value V is basically an word, and if it's a signed 32bit reloc, then

> >   V == (word)(int32_t)(uint32_t)V

> > needs to hold, and if it's a unsigned 32bit reloc then

> >   V == (word)(uint32_t)V

> > needs to hold.

> 

> And what do you suggest R_X86_64_32 is? Signed? Unsigned? In


Unsigned, which is why X86_64_32S exists.  That was the intention at 
least.  Obviously as binutils didn't carefully check for overflows in 
either interpretation I'm sure there is now inconsistency in the wild.  
And that inconsistency might even go so far as to point out that the 
initial intention in the ABI was unsustainable, and that it needs to be 
signed _and_ unsigned, like ...

>     mov     $val, %eax

> 

> you can't infer the signedness of val. Granted R_X86_64_32 may not

> be an ideal example, as there's also R_X86_64_32S. Yet for the

> example above the assembler can't know which of the two to pick if

> R_X86_64_32 was strictly unsigned.


... here.  But actually I think the assembler can pick one, it needs to be 
consistent with the instruction it chooses to implement above mnemonic.  
The above mov does a zero extension to the full 64bit word width, which is 
consistent with choosing X86_64_32 when it's strictly unsigned.

An instruction like

      sub     $val, %rax

would use 32S.  What an instruction "sub $val, %eax" would choose: hmm ;-)
I think here we really would need to accept signed and unsigned 32bit 
values.

> Hence I would think R_X86_64_32S needs to be treated as strictly signed, 


I agree.

> but R_X86_64_32 should allow for the full range of signed and unsigned 

> 32-bit values (i.e. complain_overflow_bitfield), contrary to what the 

> psABI currently says.


I'm not sure here.  I would wish for it to be treated as strictly 
unsigned, but it's quite possible that this won't work (and I'm quite sure 
it _currently_ would cause regressions).  Certainly accepting values in
[-2^31, 2^32) seems like the conservative or only possible approach.

> Interestingly these two relocations are the only ones where the psABI 

> talks about sign-/zero-extension, i.e. the "signedness" of a relocation 

> type.


I came a year too late to know the full history how that came to be (the 
32S reloc was added in September 2000).  I'm guessing that reviewing the 
initial x86-64 instruction set it was noticed that "sometimes 32bit 
numbers are sign- and sometimes zero-extended before submitted to the 
arithmetic unit" and "therefore we need two relocation types for 
appropriate checking".  Why that unsigned/signed checking wasn't deemed 
necessary for other relocs, or was deemed necessary here, I don't know.  
Probably bugs occurred during bringup, and one of them was "oh, if the 
linker would have warned here, we wouldn't have run into this strange 
segfault".  As the short relocs aren't really used in code no bugs 
occurred there, so no stricter checking was done.


Ciao,
Michael.

Patch

--- a/bfd/cofflink.c
+++ b/bfd/cofflink.c
@@ -2822,7 +2822,7 @@  _bfd_coff_reloc_link_order (bfd *output_
       if (buf == NULL && size != 0)
 	return FALSE;
 
-      rstat = _bfd_relocate_contents (howto, output_bfd,
+      rstat = _bfd_relocate_contents (howto, output_bfd, 0,
 				      (bfd_vma) link_order->u.reloc.p->addend,
 				      buf);
       switch (rstat)
--- a/bfd/ecoff.c
+++ b/bfd/ecoff.c
@@ -3953,7 +3953,7 @@  ecoff_reloc_link_order (bfd *output_bfd,
       buf = (bfd_byte *) bfd_zmalloc (size);
       if (buf == NULL && size != 0)
 	return FALSE;
-      rstat = _bfd_relocate_contents (rel.howto, output_bfd,
+      rstat = _bfd_relocate_contents (rel.howto, output_bfd, 0,
 				      (bfd_vma) addend, buf);
       switch (rstat)
 	{
--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -130,7 +130,7 @@  static reloc_howto_type elf_howto_table[
   HOWTO(R_386_TLS_GOTDESC, 0, 2, 32, FALSE, 0, complain_overflow_dont,
 	bfd_elf_generic_reloc, "R_386_TLS_GOTDESC",
 	TRUE, 0xffffffff, 0xffffffff, FALSE),
-  HOWTO(R_386_TLS_DESC_CALL, 0, 0, 0, FALSE, 0, complain_overflow_dont,
+  HOWTO(R_386_TLS_DESC_CALL, 0, 3, 0, FALSE, 0, complain_overflow_dont,
 	bfd_elf_generic_reloc, "R_386_TLS_DESC_CALL",
 	FALSE, 0, 0, FALSE),
   HOWTO(R_386_TLS_DESC, 0, 2, 32, FALSE, 0, complain_overflow_dont,
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -146,7 +146,7 @@  static reloc_howto_type x86_64_elf_howto
 	complain_overflow_bitfield, bfd_elf_generic_reloc,
 	"R_X86_64_GOTPC32_TLSDESC",
 	FALSE, 0xffffffff, 0xffffffff, TRUE),
-  HOWTO(R_X86_64_TLSDESC_CALL, 0, 0, 0, FALSE, 0,
+  HOWTO(R_X86_64_TLSDESC_CALL, 0, 3, 0, FALSE, 0,
 	complain_overflow_dont, bfd_elf_generic_reloc,
 	"R_X86_64_TLSDESC_CALL",
 	FALSE, 0, 0, FALSE),
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -11839,7 +11839,7 @@  elf_reloc_link_order (bfd *output_bfd,
       buf = (bfd_byte *) bfd_zmalloc (size);
       if (buf == NULL && size != 0)
 	return FALSE;
-      rstat = _bfd_relocate_contents (howto, output_bfd, addend, buf);
+      rstat = _bfd_relocate_contents (howto, output_bfd, 0, addend, buf);
       switch (rstat)
 	{
 	case bfd_reloc_ok:
--- a/bfd/libbfd-in.h
+++ b/bfd/libbfd-in.h
@@ -696,7 +696,8 @@  extern bfd_reloc_status_type _bfd_final_
 
 /* Relocate a particular location by a howto and a value.  */
 extern bfd_reloc_status_type _bfd_relocate_contents
-  (reloc_howto_type *, bfd *, bfd_vma, bfd_byte *) ATTRIBUTE_HIDDEN;
+  (reloc_howto_type *, bfd *, bfd_vma, bfd_signed_vma, bfd_byte *)
+  ATTRIBUTE_HIDDEN;
 
 /* Clear a given location using a given howto.  */
 extern bfd_reloc_status_type _bfd_clear_contents
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -701,7 +701,8 @@  extern bfd_reloc_status_type _bfd_final_
 
 /* Relocate a particular location by a howto and a value.  */
 extern bfd_reloc_status_type _bfd_relocate_contents
-  (reloc_howto_type *, bfd *, bfd_vma, bfd_byte *) ATTRIBUTE_HIDDEN;
+  (reloc_howto_type *, bfd *, bfd_vma, bfd_signed_vma, bfd_byte *)
+  ATTRIBUTE_HIDDEN;
 
 /* Clear a given location using a given howto.  */
 extern bfd_reloc_status_type _bfd_clear_contents
--- a/bfd/linker.c
+++ b/bfd/linker.c
@@ -2393,7 +2393,7 @@  _bfd_generic_reloc_link_order (bfd *abfd
       buf = (bfd_byte *) bfd_zmalloc (size);
       if (buf == NULL && size != 0)
 	return FALSE;
-      rstat = _bfd_relocate_contents (r->howto, abfd,
+      rstat = _bfd_relocate_contents (r->howto, abfd, 0,
 				      (bfd_vma) link_order->u.reloc.p->addend,
 				      buf);
       switch (rstat)
--- a/bfd/reloc.c
+++ b/bfd/reloc.c
@@ -1355,7 +1355,12 @@  _bfd_final_link_relocate (reloc_howto_ty
 			  bfd_vma value,
 			  bfd_vma addend)
 {
-  bfd_vma relocation;
+  /* This function assumes that we are dealing with a basic relocation
+     against a symbol.  We want to compute the value of the symbol to
+     relocate to.  This is just VALUE, the value of the symbol, plus
+     ADDEND, any addend associated with the reloc (which will get added
+     in by _bfd_relocate_contents()).  */
+  bfd_vma relocation = value;
   bfd_size_type octets = (address
 			  * bfd_octets_per_byte (input_bfd, input_section));
 
@@ -1363,12 +1368,6 @@  _bfd_final_link_relocate (reloc_howto_ty
   if (!bfd_reloc_offset_in_range (howto, input_bfd, input_section, octets))
     return bfd_reloc_outofrange;
 
-  /* This function assumes that we are dealing with a basic relocation
-     against a symbol.  We want to compute the value of the symbol to
-     relocate to.  This is just VALUE, the value of the symbol, plus
-     ADDEND, any addend associated with the reloc.  */
-  relocation = value + addend;
-
   /* If the relocation is PC relative, we want to set RELOCATION to
      the distance between the symbol (currently in RELOCATION) and the
      location we are relocating.  Some targets (e.g., i386-aout)
@@ -1387,7 +1386,7 @@  _bfd_final_link_relocate (reloc_howto_ty
 	relocation -= address;
     }
 
-  return _bfd_relocate_contents (howto, input_bfd, relocation,
+  return _bfd_relocate_contents (howto, input_bfd, relocation, addend,
 				 contents + octets);
 }
 
@@ -1397,6 +1396,7 @@  bfd_reloc_status_type
 _bfd_relocate_contents (reloc_howto_type *howto,
 			bfd *input_bfd,
 			bfd_vma relocation,
+			bfd_signed_vma addend,
 			bfd_byte *location)
 {
   bfd_vma x;
@@ -1404,12 +1404,18 @@  _bfd_relocate_contents (reloc_howto_type
   unsigned int rightshift = howto->rightshift;
   unsigned int bitpos = howto->bitpos;
 
+  if (howto->partial_inplace)
+    {
+      relocation += addend;
+      /* Get the value we are going to relocate.  */
+      x = read_reloc (input_bfd, location, howto);
+    }
+  else
+    x = addend;
+
   if (howto->negate)
     relocation = -relocation;
 
-  /* Get the value we are going to relocate.  */
-  x = read_reloc (input_bfd, location, howto);
-
   /* Check for overflow.  FIXME: We may drop bits during the addition
      which we don't check for.  We must either check at every single
      operation, which would be tedious, or we must do the computations
--- a/bfd/xcofflink.c
+++ b/bfd/xcofflink.c
@@ -5777,7 +5777,7 @@  xcoff_reloc_link_order (bfd *output_bfd,
       if (buf == NULL && size != 0)
 	return FALSE;
 
-      rstat = _bfd_relocate_contents (howto, output_bfd, addend, buf);
+      rstat = _bfd_relocate_contents (howto, output_bfd, 0, addend, buf);
       switch (rstat)
 	{
 	case bfd_reloc_ok: