Non-zero RELA section contents

Non-zero RELA section contents

[Alex Xu (Hello71)]

Hi,

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. The assembler is absolutely 
wrong.

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. 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). One argument in favor 
of the new behavior is that it is consistent with LLD and probably gold.

The following patch adds a warning:

diff --git a/bfd/reloc.c b/bfd/reloc.c
index 5098e0ab09f..aecdb21ec59 100644
--- 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. I suspect this patch may cause a large number of warnings in certain 
cases; however, I think adding a warning here is valid, because the 
result is either broken with old binutils or broken with new binutils 
(or both), and furthermore, proper assemblers should never generate this 
case in the first place.

I think the primary counterargument is that the point of "either broken 
with old binutils or broken with new binutils" primarily applies to 
x86-64/ELF, and may not apply to other targets. I am insufficiently 
familiar with other targets to say whether this is the case. If so, it 
may be better to add this warning only for x86-64/ELF.

Thanks,
Alex.

Re: Non-zero RELA section contents

[H.J. Lu]

On Fri, Apr 15, 2022 at 9:44 PM Alex Xu (Hello71) via Binutils
<binutils@sourceware.org> wrote:
>
> Hi,
>
> 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. The assembler is absolutely
> wrong.
>
> 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. 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). One argument in favor
> of the new behavior is that it is consistent with LLD and probably gold.
>
> The following patch adds a warning:
>
> diff --git a/bfd/reloc.c b/bfd/reloc.c
> index 5098e0ab09f..aecdb21ec59 100644
> --- 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. I suspect this patch may cause a large number of warnings in certain
> cases; however, I think adding a warning here is valid, because the
> result is either broken with old binutils or broken with new binutils
> (or both), and furthermore, proper assemblers should never generate this
> case in the first place.
>
> I think the primary counterargument is that the point of "either broken
> with old binutils or broken with new binutils" primarily applies to
> x86-64/ELF, and may not apply to other targets. I am insufficiently
> familiar with other targets to say whether this is the case. If so, it
> may be better to add this warning only for x86-64/ELF.

It isn't wrong to have non-zero value at relocation offset for RELA
targets.  One can encode optional information at relocation offset
for linker as an extension.  But it is wrong to store addend value at
relocation offset for RELA targets.

-- 
H.J.

Re: Non-zero RELA section contents

[Jan Beulich]

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

Re: Non-zero RELA section contents

[Fangrui Song]

On 2022-04-19, Jan Beulich via Binutils wrote:
>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.

Agree.

>> 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).

Seems that the "JWasm-family assemblers" in question computed
the value with something like ... + explicit addend + implicit addend.
I think such a bug is rare, so I'd think we don't want more code in ld
to detect such rare bugs.

>> 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.

Yes. We should go with the spec and not try working around the extremely
few buggy assemblers.

>> --- 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
>