Re: [PATCH v2 1/3] x86-64/ELF: permit relaxed overflow checking for 32-bit PC-relative relocs

["H.J. Lu" <hjl.tools@gmail.com>]

On Wed, Mar 9, 2022 at 12:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.03.2022 15:18, H.J. Lu wrote:
> > On Fri, Mar 04, 2022 at 02:34:58PM +0100, Jan Beulich wrote:
> >> Right now it is impossible to encode certain valid 32-bit mode
> >> constructs; see the respective new test case. Note that there are
> >> further 32-bit PC-relative relocations, but I don't think they make a
> >> lot of sense to use in mixed-bitness code, so they're not having
> >> overrides put in place.
> >>
> >> Putting in place a new testcase, I'd like to note that the two existing
> >> ones (pcrel16 and pcrel16abs) appear to be pretty pointless: They don't
> >> expect any error despite supposedly checking for overflow, and in fact
> >> there can't possibly be any error for the
> >> - former since gas doesn't emit any relocation in the first place there,
> >> - latter because the way the relocation gets expressed by gas doesn't
> >>   allow the linker to notice the overflow; it should be detected by gas
> >>   if at all, but see above (an error would be reported here for x86-64
> >>   afaict, but this test doesn't get re-used there).
> >> ---
> >> TBD: I didn't put thoughts yet into also making this work when linking
> >>      ELF to PE.
> >>
> >> Note that I'm not sure at all whether this propagation of the struct
> >> elf_linker_x86_params pointer is actually acceptable. But this is the
> >> 5th or 6th try I made, with all others having been worse or not even
> >> working out. Hence I'd need pretty detailed guidance on how else the
> >> information could be made available.
> >> ---
> >> v2: Re-base and split.
> >>
> >> --- a/bfd/elf-linker-x86.h
> >> +++ b/bfd/elf-linker-x86.h
> >> @@ -28,6 +28,13 @@ enum elf_x86_prop_report
> >>    prop_report_shstk = 1 << 3    /* Report missing SHSTK property.  */
> >>  };
> >>
> >> +/* Control of PC32 (on 64-bit) overflow check strictness.  */
> >> +enum elf_x86_pcrel_relocs
> >> +{
> >> +  pcrel_relocs_default,
> >> +  pcrel_relocs_lax,
> >> +};
> >> +
> >>  /* Used to pass x86-specific linker options from ld to bfd.  */
> >>  struct elf_linker_x86_params
> >>  {
> >> @@ -64,6 +71,9 @@ struct elf_linker_x86_params
> >>    /* Report relative relocations.  */
> >>    unsigned int report_relative_reloc : 1;
> >>
> >> +  /* Strictness of PC32 (on 64-bit) overflow checks.  */
> >> +  enum elf_x86_pcrel_relocs pcrel_relocs;
> >> +
> >>    /* X86-64 ISA level needed.  */
> >>    unsigned int isa_level;
> >>
> >> --- a/bfd/elf64-x86-64.c
> >> +++ b/bfd/elf64-x86-64.c
> >> @@ -192,6 +192,15 @@ static reloc_howto_type x86_64_elf_howto
> >>      false)
> >>  };
> >>
> >> +static reloc_howto_type x86_64_howto_pc32_lax =
> >> +  HOWTO(R_X86_64_PC32, 0, 2, 32, true, 0, complain_overflow_bitfield,
> >> +    bfd_elf_generic_reloc, "R_X86_64_PC32", false, 0, 0xffffffff, true);
> >> +
> >> +static reloc_howto_type x86_64_howto_pc32_bnd_lax =
> >> +  HOWTO(R_X86_64_PC32_BND, 0, 2, 32, true, 0, complain_overflow_bitfield,
> >> +    bfd_elf_generic_reloc, "R_X86_64_PC32_BND", false, 0, 0xffffffff,
> >> +    true);
> >> +
> >>  /* Map BFD relocs to the x86_64 elf relocs.  */
> >>  struct elf_reloc_map
> >>  {
> >> @@ -248,6 +257,30 @@ static const struct elf_reloc_map x86_64
> >>  };
> >>
> >>  static reloc_howto_type *
> >> +elf_x86_64_reloc_override (const bfd *abfd, reloc_howto_type *howto)
> >> +{
> >> +  const struct elf_linker_x86_params *params = elf_x86_tdata (abfd)->params;
> >> +
> >> +  switch (howto->type)
> >> +    {
> >> +    default:
> >> +      break;
> >> +
> >> +    case R_X86_64_PC32:
> >> +      if (params == NULL || params->pcrel_relocs != pcrel_relocs_lax)
> >> +    break;
> >> +      return &x86_64_howto_pc32_lax;
> >> +
> >> +    case R_X86_64_PC32_BND:
> >> +      if (params == NULL || params->pcrel_relocs != pcrel_relocs_lax)
> >> +    break;
> >> +      return &x86_64_howto_pc32_bnd_lax;
> >> +    }
> >> +
> >> +  return howto;
> >> +}
> >> +
> >> +static reloc_howto_type *
> >>  elf_x86_64_rtype_to_howto (bfd *abfd, unsigned r_type)
> >>  {
> >>    unsigned i;
> >> @@ -275,7 +308,7 @@ elf_x86_64_rtype_to_howto (bfd *abfd, un
> >>    else
> >>      i = r_type - (unsigned int) R_X86_64_vt_offset;
> >>    BFD_ASSERT (x86_64_elf_howto_table[i].type == r_type);
> >> -  return &x86_64_elf_howto_table[i];
> >> +  return elf_x86_64_reloc_override (abfd, &x86_64_elf_howto_table[i]);
> >>  }
> >>
> >>  /* Given a BFD reloc type, return a HOWTO structure.  */
> >> @@ -313,7 +346,7 @@ elf_x86_64_reloc_name_lookup (bfd *abfd,
> >>    for (i = 0; i < ARRAY_SIZE (x86_64_elf_howto_table); i++)
> >>      if (x86_64_elf_howto_table[i].name != NULL
> >>      && strcasecmp (x86_64_elf_howto_table[i].name, r_name) == 0)
> >> -      return &x86_64_elf_howto_table[i];
> >> +      return elf_x86_64_reloc_override (abfd, &x86_64_elf_howto_table[i]);
> >>
> >>    return NULL;
> >>  }
> >> @@ -1846,6 +1879,9 @@ elf_x86_64_scan_relocs (bfd *abfd, struc
> >>
> >>    BFD_ASSERT (is_x86_elf (abfd, htab));
> >>
> >> +  /* Make command line controlled settings accessible from the object.  */
> >> +  elf_x86_tdata (abfd)->params = htab->params;
> >> +
> >>    /* Get the section contents.  */
> >>    if (elf_section_data (sec)->this_hdr.contents != NULL)
> >>      contents = elf_section_data (sec)->this_hdr.contents;
> >> --- a/bfd/elfxx-x86.h
> >> +++ b/bfd/elfxx-x86.h
> >> @@ -702,6 +702,9 @@ struct elf_x86_obj_tdata
> >>    /* R_*_RELATIVE relocation in GOT for this local symbol has been
> >>       processed.  */
> >>    char *relative_reloc_done;
> >> +
> >> +  /* Container holding command line controlled linker settings.  */
> >> +  const struct elf_linker_x86_params *params;
> >>  };
> >>
> >>  enum elf_x86_plt_type
> >> --- /dev/null
> >> +++ b/gas/testsuite/gas/i386/code32.d
> >> @@ -0,0 +1,3 @@
> >> +#name: x86-64 code32
> >> +#as: -mx86-used-note=no --generate-missing-build-notes=no
> >> +#readelf: -n
> >> --- /dev/null
> >> +++ b/gas/testsuite/gas/i386/code32.s
> >> @@ -0,0 +1,11 @@
> >> +    .code32
> >> +    .text
> >> +    .section .text.0, "ax", @progbits
> >> +    .type func0, @function
> >> +func0:
> >> +    call func1
> >> +    ret
> >> +    .section .text.1, "ax", @progbits
> >> +    .type func1, @function
> >> +func1:
> >> +    jmp func0
> >> --- a/gas/testsuite/gas/i386/i386.exp
> >> +++ b/gas/testsuite/gas/i386/i386.exp
> >> @@ -1331,6 +1331,7 @@ if [gas_64_check] then {
> >>      run_dump_test "x86-64-property-8"
> >>      run_dump_test "x86-64-property-9"
> >>      run_dump_test "x86-64-property-14"
> >> +    run_dump_test "code32"
> >>
> >>      if {[istarget "*-*-linux*"]} then {
> >>          run_dump_test "x86-64-align-branch-3"
> >> --- a/ld/emulparams/elf32_x86_64.sh
> >> +++ b/ld/emulparams/elf32_x86_64.sh
> >> @@ -2,6 +2,7 @@ source_sh ${srcdir}/emulparams/plt_unwin
> >>  source_sh ${srcdir}/emulparams/extern_protected_data.sh
> >>  source_sh ${srcdir}/emulparams/dynamic_undefined_weak.sh
> >>  source_sh ${srcdir}/emulparams/reloc_overflow.sh
> >> +source_sh ${srcdir}/emulparams/pcrel-relocs.sh
> >>  source_sh ${srcdir}/emulparams/call_nop.sh
> >>  source_sh ${srcdir}/emulparams/cet.sh
> >>  source_sh ${srcdir}/emulparams/x86-report-relative.sh
> >> --- a/ld/emulparams/elf_x86_64.sh
> >> +++ b/ld/emulparams/elf_x86_64.sh
> >> @@ -2,6 +2,7 @@ source_sh ${srcdir}/emulparams/plt_unwin
> >>  source_sh ${srcdir}/emulparams/extern_protected_data.sh
> >>  source_sh ${srcdir}/emulparams/dynamic_undefined_weak.sh
> >>  source_sh ${srcdir}/emulparams/reloc_overflow.sh
> >> +source_sh ${srcdir}/emulparams/pcrel-relocs.sh
> >>  source_sh ${srcdir}/emulparams/call_nop.sh
> >>  source_sh ${srcdir}/emulparams/cet.sh
> >>  source_sh ${srcdir}/emulparams/x86-report-relative.sh
> >> --- /dev/null
> >> +++ b/ld/emulparams/pcrel-relocs.sh
> >> @@ -0,0 +1,11 @@
> >> +PARSE_AND_LIST_OPTIONS_STRICT_PCREL_RELOCS='
> >> +  fprintf (file, _("\
> >> +  -z lax-pcrel-relocs         Lax PC-relative relocation overflow checks\n"));
> >> +'
> >> +PARSE_AND_LIST_ARGS_CASE_Z_STRICT_PCREL_RELOCS='
> >> +      else if (strcmp (optarg, "lax-pcrel-relocs") == 0)
> >> +    params.pcrel_relocs = pcrel_relocs_lax;
> >> +'
> >> +
> >> +PARSE_AND_LIST_OPTIONS="$PARSE_AND_LIST_OPTIONS $PARSE_AND_LIST_OPTIONS_STRICT_PCREL_RELOCS"
> >> +PARSE_AND_LIST_ARGS_CASE_Z="$PARSE_AND_LIST_ARGS_CASE_Z $PARSE_AND_LIST_ARGS_CASE_Z_STRICT_PCREL_RELOCS"
> >> --- a/ld/ld.texi
> >> +++ b/ld/ld.texi
> >> @@ -1372,6 +1372,12 @@ missing properties in input files.  @opt
> >>  the linker issue an error for missing properties in input files.
> >>  Supported for Linux/x86_64.
> >>
> >> +@item lax-pcrel-relocs
> >> +Relax relocation overflow checks for certain 32-bit PC-relative relocations
> >> +which, when used by 32-bit code inside a 64-bit object, may require a
> >> +larger range of values to be considered valid.
> >> +Supported for x86-64 ELF targets.
> >> +
> >
> > I think the check should be turned on automatically.  Can you use a GNU
> > property bit to tell linker that a larger range of values should be
> > checked for R_X86_64_PC32
>
> I'm not convinced that would be desirable - the relaxed checking, after
> all, also affects relocations to 64-bit mode. Hence certain overflows
> won't be detected anymore. Therefore I'd expect people to make use of
> the new option only if they really have any affected relocations in
> 32-bit code. Additionally there's no way I can see to set such a
> property indicator when encountering the relocations in question only
> in data definitions, unless you wanted to tie the setting of the
> indicator to the mere use of .code{16,32} anywhere in the source (which
> would feel way to aggressive to me). IMO this level of control can only
> be achieved via command line option (without (a) becoming much more
> intrusive or (b) introducing new relocation types).

A new relocation type sounds better.

> If this was to be "automated", the assembler would need to let the
> linker know the boundaries between 64-bit and non-64-bit code, and
> (yet more difficult) between data used by 64-bit code and such used
> by 32-bit code.
>
> > and issue an error for R_X86_64_PC32_BND?
>
> Why should this result in an error?
>
> Jan
>


-- 
H.J.