Re: [PATCH v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[Fangrui Song <maskray@google.com>]

On 2022-06-07, H.J. Lu wrote:
>On Tue, Jun 7, 2022 at 4:57 PM Fangrui Song <maskray@google.com> wrote:
>>
>>
>> On 2022-06-07, H.J. Lu wrote:
>> >On Tue, Jun 7, 2022 at 1:00 PM Fangrui Song <maskray@google.com> wrote:
>> >>
>> >> On 2022-06-07, H.J. Lu wrote:
>> >> >On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
>> >> >>
>> >> >> On 2022-06-07, H.J. Lu wrote:
>> >> >> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
>> >> >> ><libc-alpha@sourceware.org> wrote:
>> >> >> >>
>> >> >> >> The 06/01/2022 10:56, Fangrui Song wrote:
>> >> >> >> > If an executable has copy relocations for extern protected data, that
>> >> >> >> > can only work if the library containing the definition is built with
>> >> >> >> > assumptions (a) the compiler emits GOT-generating relocations (b) the
>> >> >> >> > linker produces R_*_GLOB_DAT instead of R_*_RELATIVE.  Otherwise the
>> >> >> >> > library uses its own definition directly and the executable accesses a
>> >> >> >> > stale copy.  Note: the GOT relocations defeat the purpose of protected
>> >> >> >> > visibility as an optimization, but allow rtld to make the executable and
>> >> >> >> > library use the same copy when copy relocations are present, but it
>> >> >> >> > turns out this never worked perfectly.
>> >> >> >> >
>> >> >> >> > ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange semantics when both
>> >> >> >> > a.so and b.so define protected var and the executable copy relocates
>> >> >> >> > var: b.so accesses its own copy even with GLOB_DAT.  The behavior change
>> >> >> >> > is from commit 62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then
>> >> >> >> > copied to nios2 (ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc
>> >> >> >> > (0e7d930c4c11de896fe807f67fa1eb756c9c1e05).
>> >> >> >> >
>> >> >> >> > Without ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA, b.so accesses the copy
>> >> >> >> > relocated data like a.so.
>> >> >> >> >
>> >> >> >> > ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has another effect in the absence
>> >> >> >> > of copy relocations: when a protected data symbol is defined in multiple
>> >> >> >> > objects, the code tries to bind the relocation locally.  Without
>> >> >> >> > ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA, STV_PROTECTED is handled in the
>> >> >> >> > same way as STV_DEFAULT: if ld produces GLOB_DAT (some ports of GNU ld),
>> >> >> >> > the relocation will bind to the first definition; otherwise (e.g.
>> >> >> >> > ld.lld) ld does the binding locally and ld.so doesn't help.
>> >> >> >> >
>> >> >> >>
>> >> >> >> i think we should not change the interposition semantics.
>> >> >> >> we should go back to the old behaviour where only copy
>> >> >> >> relocs were broken (and there was an expensive workaround
>> >> >> >> to deal with protected symbol interposition).
>> >> >> >>
>> >> >> >> i think you want to revert the elf/dl-lookup.c changes of
>> >> >> >>
>> >> >> >>   commit 62da1e3b00b51383ffa7efc89d8addda0502e107
>> >> >> >>   Author:     H.J. Lu <hjl.tools@gmail.com>
>> >> >> >>   CommitDate: 2015-03-31 05:16:57 -0700
>> >> >> >>
>> >> >> >>   Add ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA to x86
>> >> >> >>
>> >> >> >
>> >> >> >I am OK to remove support of copy relocation against protected
>> >> >> >symbols since it doesn't work properly.
>> >> >>
>> >> >> Thanks.
>> >> >>
>> >> >> >My only question is if
>> >> >> >ld.so should issue a warning or an error when seeing a copy
>> >> >> >relocation against a protected symbol.   Copy relocation against
>> >> >> >protected symbol defeats the purpose of protected symbol.
>> >> >>
>> >> >> The check already exists (_dl_check_protected_symbol) but currently
>> >> >> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
>> >> >> for x86, and adoption is low on x86).
>> >> >>
>> >> >> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >> check can be removed.
>> >> >
>> >> >Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >check cause many run-time errors?
>> >> >> (
>> >> >> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
>> >> >> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
>> >> >> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >> but the incompatibility exists.
>> >> >> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
>> >> >> )
>> >> >>
>> >> >> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
>> >> >> practice:
>> >> >>
>> >> >> * protected visibility adoption is very low due to various problems.
>> >> >> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
>> >> >> * Many users use ICF and by and large don't care about function addresses to some extent.
>> >> >>
>> >> >> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
>> >> >> (
>> >> >> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
>> >> >>    indistinguishable from an address-taken operation
>> >> >>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
>> >> >> )
>> >> >
>> >> >An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >and a warning without?
>> >>
>> >> This plan sounds good, when we create a separate patch enhancing the
>> >> diagnostics.
>>
>> Created
>> https://sourceware.org/pipermail/libc-alpha/2022-June/139552.html
>> ([PATCH] elf: Refine direct extern access diagnostics to protected symbol).
>>
>> >> x86-32 may need a exception (i.e. no warning) for ELF_RTYPE_CLASS_PLT to handle R_386_PC32.
>> >
>> >Linker sets non-zero symbol values for undefined function symbols in
>> >executable only when their addresses are taken.  R_386_PC32 shouldn't
>> >matter.
>>
>> OK, I believe GNU ld distinguishes branch/address-taken usages of
>> R_386_PC32 by poking at the instruction opcode.  That works.
>> ld.lld doesn't check the opcode, and just reports "error: cannot preempt
>> symbol:" in an example I crafted.
>
>Does lld always set non-zero symbol values for undefined function symbols?

For a direct access relocation which is neither a link-time constant nor
a dynamic relocation (e.g. R_386_32 in a writable section), lld always
sets non-zero st_value for an undefined symbol referencing STT_OBJECT.

lld doesn't know R_386_PC32 may be used with call/jmp.  It simply
conservatively treats all R_386_PC32 as possibly address-taking.
BTW: This was why I opened https://sourceware.org/bugzilla/show_bug.cgi?id=27169
(i386: Emit R_386_PLT32 instead of R_386_PC32 for `call/jmp foo`) but I
can accept that we keep it unchanged.

>> >> >> >> > It's extremely unlikely anyone relies on the
>> >> >> >> > ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so let's remove it: this
>> >> >> >> > removes a check in the symbol lookup code.
>> >> >> >> >
>> >> >> >> > --
>> >> >> >> > Changes from v1:
>> >> >> >> > * Reword commit message as suggested by Szabolcs Nagy
>> >> >> >> >
>> >> >> >> > Changes from v2:
>> >> >> >> > * Explain interposition behavior
>> >> >> >> > ---
>> >> >> >> >  elf/dl-lookup.c             | 90 -------------------------------------
>> >> >> >> >  sysdeps/arc/dl-sysdep.h     | 21 ---------
>> >> >> >> >  sysdeps/generic/ldsodefs.h  | 12 +----
>> >> >> >> >  sysdeps/i386/dl-machine.h   |  3 +-
>> >> >> >> >  sysdeps/nios2/dl-sysdep.h   | 21 ---------
>> >> >> >> >  sysdeps/x86/dl-lookupcfg.h  |  4 --
>> >> >> >> >  sysdeps/x86_64/dl-machine.h |  8 +---
>> >> >> >> >  7 files changed, 4 insertions(+), 155 deletions(-)
>> >> >> >> >  delete mode 100644 sysdeps/arc/dl-sysdep.h
>> >> >> >> >  delete mode 100644 sysdeps/nios2/dl-sysdep.h
>> >> >> >> >
>> >> >> >> > diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>> >> >> >> > index a42f6d5390..41d108e0b8 100644
>> >> >> >> > --- a/elf/dl-lookup.c
>> >> >> >> > +++ b/elf/dl-lookup.c
>> >> >> >> ...
>> >> >> >> > @@ -854,43 +801,6 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
>> >> >> >> >        return 0;
>> >> >> >> >      }
>> >> >> >> >
>> >> >> >> > -  int protected = (*ref
>> >> >> >> > -                && ELFW(ST_VISIBILITY) ((*ref)->st_other) == STV_PROTECTED);
>> >> >> >> > -  if (__glibc_unlikely (protected != 0))
>> >> >> >> > -    {
>> >> >> >> > -      /* It is very tricky.  We need to figure out what value to
>> >> >> >> > -      return for the protected symbol.  */
>> >> >> >> > -      if (type_class == ELF_RTYPE_CLASS_PLT)
>> >> >> >> > -     {
>> >> >> >> > -       if (current_value.s != NULL && current_value.m != undef_map)
>> >> >> >> > -         {
>> >> >> >> > -           current_value.s = *ref;
>> >> >> >> > -           current_value.m = undef_map;
>> >> >> >> > -         }
>> >> >> >> > -     }
>> >> >> >> > -      else
>> >> >> >> > -     {
>> >> >> >> > -       struct sym_val protected_value = { NULL, NULL };
>> >> >> >> > -
>> >> >> >> > -       for (scope = symbol_scope; *scope != NULL; i = 0, ++scope)
>> >> >> >> > -         if (do_lookup_x (undef_name, new_hash, &old_hash, *ref,
>> >> >> >> > -                          &protected_value, *scope, i, version, flags,
>> >> >> >> > -                          skip_map,
>> >> >> >> > -                          (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
>> >> >> >> > -                           && ELFW(ST_TYPE) ((*ref)->st_info) == STT_OBJECT
>> >> >> >> > -                           && type_class == ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA)
>> >> >> >> > -                          ? ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
>> >> >> >> > -                          : ELF_RTYPE_CLASS_PLT, NULL) != 0)
>> >> >> >> > -           break;
>> >> >> >> > -
>> >> >> >> > -       if (protected_value.s != NULL && protected_value.m != undef_map)
>> >> >> >> > -         {
>> >> >> >> > -           current_value.s = *ref;
>> >> >> >> > -           current_value.m = undef_map;
>> >> >> >> > -         }
>> >> >> >> > -     }
>> >> >> >> > -    }
>> >> >> >> > -
>> >> >> >>
>> >> >> >> i think we should keep this part without the
>> >> >> >> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA bit.
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >--
>> >> >> >H.J.
>> >> >
>> >> >
>> >> >
>> >> >--
>> >> >H.J.
>> >
>> >
>> >
>> >--
>> >H.J.
>
>
>
>-- 
>H.J.