[PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling

[Fangrui Song]

On 2022-05-10, H.J. Lu wrote:
>On Tue, May 10, 2022 at 12:42 AM Fangrui Song <maskray@google.com> wrote:
>>
>> On 2022-05-09, H.J. Lu wrote:
>> >On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
>> ><libc-alpha@sourceware.org> wrote:
>> >>
>> >> Say both a.so and b.so define protected var and the executable copy
>> >> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
>> >> semantics: a.so accesses the copy in the executable while b.so accesses
>> >> its own. This behavior requires that (a) the compiler emits
>> >> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
>> >> RELATIVE.
>> >>
>> >> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>> >> will bind to the executable (normal behavior).
>> >>
>> >> For aarch64/arm it makes sense to restore the original behavior and don't
>> >> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
>> >> unlikely used by anyone.
>> >>
>> >> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>> >>   no GOT-generating relocation in the first place.
>> >> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>> >> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>> >>   GOT-generating relocation when accessing an default visibility
>> >>   external symbol which avoids copy relocation.
>> >>
>> >> Fangrui Song (3):
>> >>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
>> >>     non-DL_EXTERN_PROTECTED_DATA ports
>> >>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
>> >>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
>> >>
>> >>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
>> >>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
>> >>  sysdeps/aarch64/dl-sysdep.h  |  2 --
>> >>  sysdeps/arm/dl-machine.h     | 10 +++-----
>> >>  sysdeps/arm/dl-sysdep.h      |  2 --
>> >>  5 files changed, 24 insertions(+), 49 deletions(-)
>> >>
>> >> --
>> >> 2.36.0.464.gb9c8b46e94-goog
>> >>
>> >
>> >Given that protected symbols never work properly with copy relocation,
>> >can we change protected symbol handling to simply warn copy relocation
>> >against protected data symbol and non-zero symbol values of undefined
>> >symbols against protected function symbols?
>>
>> "protected symbols never work properly with copy relocation" generally
>> applies to all non-x86 architectures (arm/aarch64 has some guarantee,
>> but not reliable). My goal is indeed to remove relevant code for all
>> non-x86 architectures. They should not even bother testing
>> STV_PROTECTED.
>
>Even on x86, protected symbols don't work as intended.   They don't
>provide any performance benefits.  I think we should move away from
>copy relocation on protected symbols.  We can either issue a warning
>or an error.
>
>> On x86 there is some guarantee. The gcc>=5 -fpie
>> HAVE_LD_PIE_COPYRELOC behavior makes the problem trigger in some cases.
>> Having a warning in x86 specific files (e.g.
>> sysdeps/x86_64/dl-machine.h R_X86_64_COPY) seems appropriate.

How about adding a warning for R_X86_64_COPY like the following, then removing
all code paths of (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA!=0)?

 From 3f905ff283178396840861a965acf35bd21cf5f0 Mon Sep 17 00:00:00 2001
From: Fangrui Song <maskray@google.com>
Date: Wed, 11 May 2022 21:54:02 -0700
Subject: [PATCH] x86: Warn for copy relocations on a protected symbol

---
  sysdeps/x86_64/dl-machine.h | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index c70af7ab1e..48f08ecc0b 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -474,6 +474,12 @@ and creates an unsatisfiable circular dependency.\n",
             break;
           memcpy (reloc_addr_arg, (void *) value,
                   MIN (sym->st_size, refsym->st_size));
+         if (ELFW(ST_VISIBILITY) (sym->st_other) == STV_PROTECTED)
+           {
+             fmt = "\
+%s: Protected symbol `%s' is incompatible with copy relocations\n";
+             goto print_err;
+           }
           if (__glibc_unlikely (sym->st_size > refsym->st_size)
               || (__glibc_unlikely (sym->st_size < refsym->st_size)
                   && GLRO(dl_verbose)))