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

Thu May 27 13:46:20 GMT 2021

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?
My proposed patch should be sufficient.  If not, do you have a testcase?

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

-- 
H.J.