[PATCH v2] x86: Restore PC16 relocation overflow check

[Jan Beulich]

On 01.06.2021 15:02, Michael Matz wrote:
> On Tue, 1 Jun 2021, Jan Beulich wrote:
>>>>>> The issue is how linker should handle overflow for PC16.
>>>>>
>>>>> I think as per above.  An value within [-0xffff,0xffff] 
>>>>> (mathematically, i.e. the 32 or 64 bit value of the computation 
>>>>> S+A-P) is defined as not overflowing.  The linker basically needs to 
>>>>> assume that the author knew what he was doing when using a PC16 
>>>>> offset and only needs to flag things that simply cannot be made to 
>>>>> work (like e.g. jumping 0x12345 bytes forward).
>>>>
>>>> While the ABI is free to state what it wants, such a wide range is 
>>>> not very useful when the relocation is used outside of 16-bit mode.
>>>
>>> Well, as it stands the relocation needs to support uses within and 
>>> outside 16-bit mode, and the linker can't differ between those, so 
>>> what's acceptable needs to be the union of all ranges.
>>>
>>>> The main need for the overflow detection is when it's out of range by 
>>>> just a little (e.g. because of some piece of code or data having 
>>>> grown just enough to bring it out of range).
>>>
>>> But out of _what_ range?  You are saying that [-0xffff,0xffff] is too 
>>> broad, what range do you want?  (I don't think you can get away with a 
>>> smaller range with PC16 in the x86-64 psABI, but for completeness)
>>
>> The true disp16 range - [-0x8000,+0x7fff].
> 
> Okay, well, yeah, I meanwhile think we can't do that generally :-/.  We 
> could possibly with an option, and I think we could have that option on by 
> default to be disabled by code that needs to embed 16bit code into elf32 
> or elf64 binaries.  (That option would extend the accepted range to 
> [-0xffff,0xffff])

That's one of the things I proposed to H.J. earlier on. He took the
position that even such projects should continue to work "out-of-the-
box" with the next binutils release. I disagree, but he's the
maintainer ...

>> As said before, for the purposes of 32- and 64-bit code (and data), this 
>> (or a new, proper reloc type) needs to match PC32 allowing 
>> [-0x80000000,+0x7fffffff]. And obviously this also ought to match PC8, 
>> allowing [-0x80,+0x7f].
> 
> I think for PC8 we have an easier time: there's no code model that wraps 
> around at 256, so allowing only [-0x80,0x7f] now and requiring 
> sign-extension to preserve the value ought to not give us any headaches in 
> the wild.
> 
>> Also note how PC8's current handling in bfd does _not_ fit 16-bit
>> code (wrapping at the 64k boundary not being accounted for).
> 
> I don't think we "correctly" do that with any code model or relocation, 
> right?  The linker doesn't know the context of relocations, and hence 
> can't know if it would need to account for wrap-around at 64k, at 4G or 
> at 1<<64.  Or, if we want to get into extremes consider the A20 gate and a 
> wraparound at 1M.  There's only so much we can model.

You don't mention the A20 gate here, do you? ;-)

> I mean you can embed 32bit code in elf64, and use an PC32 reloc to get 
> from 3G to 0G, if you consider the wrap-around.  When only strictly 
> accepting the signed32 range that would be impossible, and we have the 
> same problem as with PC16: should we, or should we not, flag such use, and 
> under which conditions?

Right, I did consider this PC32 issues as well. Embedding smaller
address size code into an object targeting a larger address size
architecture is of course always problematic. Hence why I think the
PC32 handling is okay as is, but the PC16 one would better match in
how it gets treated.

> In such cases we need to consider existing use in the wild and provide a 
> way to accept existing .o files.  If those cases are seldom then possibly 
> only under an option.

Right - the tradeoff between sufficiently precise checks (with false
positive errors) and sufficiently imprecise checks (with false
negatives, i.e. missing diagnostics). That's generally something only
a human can decide, and only on a project by project or even component
by component basis.

>>>> This is what my change was about, which - aiui - H.J. has meanwhile 
>>>> reverted.
>>>
>>> If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001] 
>>> by default then I think the reversal was justified, those need to be 
>>> continued to be accepted, for better or worse.
>>
>> Interesting - I took your initial reply to H.J. to mean the opposite.
>> Obviously a misunderstanding then ...
> 
> No, I changed my mind in the meantime.  I would have liked to say the 
> obvious thing in the psABI and not consider wrap-around, but now think we 
> can't go the full way for that.  I still would like us to make PC8 and 
> PC16 acceptable, but the latter with a larger than obvious acceptable 
> range.
> 
> As for a new relocaiton that does have the signed16 restriction, no matter 
> the code model, I remain doubtful: it can introduce more (compatibility) 
> problems than it solves.

Indeed, there is this risk. I continue to be in favor of a new command
line option, off by default, restoring original PC16 checking and at
the same time making PC32 in a 64-bit container match. (It could be two
separate options, but to me this feels like going too far.) As per what
you say above, I'd be happy to leave PC8 as is.

H.J. - can we get you to re-consider?

Jan