[PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
H.J. Lu
hjl.tools@gmail.com
Thu May 27 12:29:14 GMT 2021
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
sym = i.op[0].disps->X_add_symbol;
off = i.op[0].disps->X_add_number;
> 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. But you can strip it at link-time.
> 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
--
H.J.
More information about the Binutils
mailing list