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

[Jan Beulich <jbeulich@suse.com>]

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

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