inconsistencies if (at least) ELF relocation handling

[Jan Beulich]

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