[PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
H.J. Lu
hjl.tools@gmail.com
Thu May 27 16:05:57 GMT 2021
On Thu, May 27, 2021 at 8:53 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, May 27, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 27.05.2021 15:46, H.J. Lu wrote:
> > > On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 27.05.2021 15:08, H.J. Lu wrote:
> > >>> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>
> > >>>> On 27.05.2021 14:29, H.J. Lu wrote:
> > >>>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>
> > >>>>>> 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.
> > >>>>>
> > >>>>> We can do this:
> > >>>>>
> > >>>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> > >>>>> index c17f4da16fe..8adf0fce0a6 100644
> > >>>>> --- a/gas/config/tc-i386.c
> > >>>>> +++ b/gas/config/tc-i386.c
> > >>>>> @@ -2695,10 +2695,6 @@ static void
> > >>>>> set_code_flag (int value)
> > >>>>> {
> > >>>>> update_code_flag (value, 0);
> > >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > >>>>> - if (value == CODE_16BIT)
> > >>>>> - x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > >>>>> -#endif
> > >>>>> }
> > >>>>>
> > >>>>> static void
> > >>>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
> > >>>>> cpu_arch_flags.bitfield.cpu64 = 0;
> > >>>>> cpu_arch_flags.bitfield.cpuno64 = 1;
> > >>>>> stackop_size = LONG_MNEM_SUFFIX;
> > >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > >>>>> - if (new_code_flag == CODE_16BIT)
> > >>>>> - x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > >>>>> -#endif
> > >>>>> }
> > >>>>>
> > >>>>> static void
> > >>>>> @@ -8784,6 +8776,10 @@ output_branch (void)
> > >>>>> else
> > >>>>> subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
> > >>>>> subtype |= code16;
> > >>>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > >>>>> + if (code16)
> > >>>>> + x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > >>>>> +#endif
> > >>>>
> > >>>> This would get us closer to what I have described, but doesn't go
> > >>>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
> > >>>> misleading and would probably want naming (and describing)
> > >>>> differently.
> > >>>
> > >>> Do you have any suggestions?
> > >>
> > >> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
> > >> when the flag gets set (i.e. only when a PC16 reloc gets actually
> > >> generated).
> > >
> > > How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?
> >
> > Fine with me - I don't care much about the name as long as it's
> > not actively misleading.
> >
> > > My proposed patch should be sufficient. If not, do you have a testcase?
> >
> > The decision whether to emit a relocation happens only during
> > relaxation, aiui. IOW it is my understanding that a forward
> > branch to a local symbol would also cause the flag to get set.
>
> I may be able to avoid it. But ...
>
> > >>>>>> 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.
> > >>>>>
> > >>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> > >>>>> suppressed.
> > >>>>
> > >>>> Of course it can, via -mx86-used-note=no. Or did you change this
> > >>>> kind of silently as a "side effect" of your patch, without me
> > >>>> even noticing? That would be evil - as said earlier (and reported
> > >>>> as a bug), these notes shouldn't be there by default; even less
> > >>>> so should they be there unconditionally.
> > >>>
> > >>> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
> > >>> Otherwise, it won't work for seabios.
> > >>
> > >> Well, for both this and ...
> > >>
> > >>>> Plus even if it couldn't be suppressed, there would still be the
> > >>>> issue with object files created by other / older tools.
> > >>>
> > >>> This is a workaround/compromise. There is no perfect solution.
> > >>> It works for seabios.
> > >>
> > >> ... this: Abusing relocations is something that may very well require
> > >> projects to adjust their build environment, e.g. by adding a command
> > >> line option to one of the tools involved.
> > >
> > > When I was reading the binutils source, my impression was that x86 PC16
> > > relocations were intended for 16-bit codes in an ELF32/ELF64 container.
> > > Whatever we do, we shouldn't break existing 16-bit programs.
> >
> > Well, if the treatment of PC16 isn't ABI-conforming, then perhaps
> > the ABI should be changed to express the intended behavior for this
> > reloc type (and make explicit that it's different from PC32 and PC8),
> > and there then perhaps ought to be a new reloc type with proper PC16
> > semantics (usable for 16-bit operand size XBEGIN as well as 16-bit
> > data items with PC-relative expressions)?
>
> This sounds promising. We can update the current PC16 description
> for 16-bit objects. We can add a new PC16 relocation for XBEGIN if
> it is really desirable. I will open an issue for this.
>
The current psABI has
---
A program or object file using R_X86_64_8, R_X86_64_16, R_X86_64_PC16
or R_X86_64_PC8 relocations is not conformant to this ABI, these
relocations are only
added for documentation purposes.
----
I opened:
https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/7
I think both commits should be reverted.
--
H.J.
More information about the Binutils
mailing list