inconsistencies if (at least) ELF relocation handling
H.J. Lu
hjl.tools@gmail.com
Mon Apr 19 17:34:02 GMT 2021
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.
More information about the Binutils
mailing list