[PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions

Jan Beulich jbeulich@suse.com
Thu May 27 07:10:40 GMT 2021


On 26.05.2021 16:16, H.J. Lu wrote:
> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 25.05.2021 18:11, H.J. Lu wrote:
>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>
>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>
>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>
>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>>>>
>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>>>>
>>>>>>>>>> as in
>>>>>>>>>>
>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>>>>
>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>>>>
>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>>>>> introduction wants to be separated from the specific use in the
>>>>>>>>> linker, not the lease because ...
>>>>>>>>>
>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>>>>
>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>>>>> goal of the earlier change.
>>>>>>>>
>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>
>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>
>>>>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>>>>> restores the old behavior only when input has 16-bit codes.
>>>>>>>
>>>>>>> When paying specific attention to resulting code size, people may
>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>>>>> code. This not having worked properly is what initially triggered
>>>>>>> me touching this area. And of course assembler programmers could
>>>>>>> even cause data to have PC16 relocations attached to it.
>>>>>>
>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>>>>> changed.
>>>>>
>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
>>>>> contain small pieces of 16-bit code.
>>>>
>>>> And we never ran into any issues before.  My patch just restores
>>>> the old behavior for them.
>>>
>>> And my point is that the old behavior was wrong (and will be again if
>>> we go with your change).
>>
>> The odd behavior is correct for 16-bit codes.  My patch restores the
>> old behavior only if input has 16-bit codes.
>>
> 
> This is the patch I am going to check in.

Sadly you've sent all but v1 only as attachments, making commenting
difficult. There are issues:
1) The workaround gets triggered from the handling of the actual
.code* directives, so would come into effect even if no insn at all
was emitted. Imo this should be tied to the specific case of a PC16
relocation getting emitted for 16-bit code. This would also take
care of 4) below.
2) Object files created by other tools (including older gas, which
would include an incremental build after having updated binutils),
or created with that note's emission suppressed (which, as said in
bug 27753, shouldn't be emitted by default anyway) will still cause
problems.
3) Imo the workaround would better be limited to things living in
the low 64k of address space. This would in particular avoid
negatively affecting OS kernels and alike, which typically get
linked for a (large) non-zero base address. Albeit I admit there
would be (theoretical) cases remaining where an overflow might
still get wrongly diagnosed.
4) Whether .code16gcc should trigger the workaround is at least
questionable. In this mode, all stack related operations are 32-bit
ones, and hence only JMP and Jcc (but not CALL) would require the
marker flag to get set.

Jan


More information about the Binutils mailing list