[PATCH v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
Fangrui Song
maskray@google.com
Tue Jun 7 18:21:35 GMT 2022
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.
(
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
)
>> > 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.
More information about the Libc-alpha
mailing list