Re: [PATCH v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

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

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

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