Re: Non-zero RELA section contents

[Jan Beulich <jbeulich@suse.com>]

On 16.04.2022 06:44, Alex Xu (Hello71) wrote:
> In ELF RELA relocations, the addend is stored in the relocation instead 
> of the section contents. This was the premise of commit 17c6c3b99156 
> ("x86-64/ELF: clear src_mask for all reloc types"); the value in the 
> section contents is not used.
> 
> I recently encountered a difficult-to-trace bug in 7-Zip, eventually 
> determined to be caused by JWasm-family assemblers incorrectly putting 
> the addend in the section contents, and setting the RELA addend to -4. 
> Previous versions of binutils added the existing value to the relocation 
> result, producing a working program; new versions of binutils overwrite 
> it, producing a non-working program.
> 
> Now, I want to be clear: while the relevant specifications are not clear 
> about what the linker should do in this case, they are reasonably clear 
> that the assembler should not generate this.

I think the ELF spec is quite clear. The term usually referenced as A
(addend) has a well-defined origin. The value coming from the field
being relocated represents A in REL but doesn't participate in the
calculation of the final value at all in RELA.

> With that being said, I think it would be a good idea for ld to either 
> revert to the previous behavior or issue a warning or error when 
> detecting such malformed object files. I think a warning at the least is 
> appropriate, because binaries are either silently incorrect on the old 
> version, or silently incorrect on the new version, and silently 
> incorrect relocations are extremely hard to diagnose.

Such a warning might be okay to add as long as it's off by default.
Object files are free to have being-relocated fields hold whichever
value they like when the value doesn't matter for relocation. These
values may provide hints to tools (e.g. disassemblers).

> One argument in 
> favor of reverting to the previous behavior is it is better to preserve 
> backwards compatibility in BFD linker when the new behavior is not 
> clearly superior (e.g. faster or closer to spec).

But the new behavior _is_ closer to the spec.

> One argument in favor 
> of the new behavior is that it is consistent with LLD and probably gold.

And, as said, consistent with the spec.

> --- a/bfd/reloc.c
> +++ b/bfd/reloc.c
> @@ -1509,6 +1509,11 @@ _bfd_relocate_contents (reloc_howto_type *howto,
>    relocation >>= (bfd_vma) rightshift;
>    relocation <<= (bfd_vma) bitpos;
>  
> +  if (!howto->src_mask && (x & howto->dst_mask))
> +    _bfd_error_handler
> +      (_("warning: %pB: existing value for %s relocation is %lx, expected 0"),
> +       input_bfd, howto->name, x);
> +
>    /* Add RELOCATION to the right bits of X.  */
>    x = ((x & ~howto->dst_mask)
>         | (((x & howto->src_mask) + relocation) & howto->dst_mask));
> 
> I have tested this patch on a single test file which printed the desired 
> warning. I believe %lx is probably wrong, but I don't know how to fix 
> it.

You will want to use BFD_VMA_FMT instead.

Jan