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

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

[Fangrui Song]

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

[PATCH 1/3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for non-DL_EXTERN_PROTECTED_DATA ports

[Fangrui Song]

A port not defining DL_EXTERN_PROTECTED_DATA should not incur the
overhead. In addition, taking the address of a protected function never
works (either breaks pointer equality or rejected by ld). Just remove
the code related to functions.
---
 elf/dl-lookup.c | 46 +++++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 989b073e4f..1bb80688ad 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -863,40 +863,24 @@ _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))
+  if (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
+      && __glibc_unlikely (
+	  *ref && ELFW (ST_VISIBILITY) ((*ref)->st_other) == STV_PROTECTED
+	  && ELFW (ST_TYPE) ((*ref)->st_info) == STT_OBJECT))
     {
-      /* 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 };
+      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;
+      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, NULL)
+	    != 0)
+	  break;
 
-	  if (protected_value.s != NULL && protected_value.m != undef_map)
-	    {
-	      current_value.s = *ref;
-	      current_value.m = undef_map;
-	    }
+      if (protected_value.s != NULL && protected_value.m != undef_map)
+	{
+	  current_value.s = *ref;
+	  current_value.m = undef_map;
 	}
     }
 
-- 
2.36.0.464.gb9c8b46e94-goog

[PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"

[Fangrui Song]

This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.

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 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.
---
 sysdeps/aarch64/dl-machine.h | 13 ++++++-------
 sysdeps/aarch64/dl-sysdep.h  |  2 --
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index b40050a981..530952a736 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -182,13 +182,12 @@ _dl_start_user:								\n\
 ");
 
 #define elf_machine_type_class(type)					\
-  ((((type) == AARCH64_R(JUMP_SLOT)					\
-     || (type) == AARCH64_R(TLS_DTPMOD)					\
-     || (type) == AARCH64_R(TLS_DTPREL)					\
-     || (type) == AARCH64_R(TLS_TPREL)					\
-     || (type) == AARCH64_R(TLSDESC)) * ELF_RTYPE_CLASS_PLT)		\
-   | (((type) == AARCH64_R(COPY)) * ELF_RTYPE_CLASS_COPY)		\
-   | (((type) == AARCH64_R(GLOB_DAT)) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+  ((((type) == R_AARCH64_JUMP_SLOT ||					\
+     (type) == R_AARCH64_TLS_DTPMOD ||					\
+     (type) == R_AARCH64_TLS_DTPREL ||					\
+     (type) == R_AARCH64_TLS_TPREL ||					\
+     (type) == R_AARCH64_TLSDESC) * ELF_RTYPE_CLASS_PLT)		\
+   | (((type) == R_AARCH64_COPY) * ELF_RTYPE_CLASS_COPY))
 
 #define ELF_MACHINE_JMP_SLOT	AARCH64_R(JUMP_SLOT)
 
diff --git a/sysdeps/aarch64/dl-sysdep.h b/sysdeps/aarch64/dl-sysdep.h
index 667786671c..ac69f414f3 100644
--- a/sysdeps/aarch64/dl-sysdep.h
+++ b/sysdeps/aarch64/dl-sysdep.h
@@ -21,5 +21,3 @@
 /* _dl_argv cannot be attribute_relro, because _dl_start_user
    might write into it after _dl_start returns.  */
 #define DL_ARGV_NOT_RELRO 1
-
-#define DL_EXTERN_PROTECTED_DATA
-- 
2.36.0.464.gb9c8b46e94-goog

Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"

[Szabolcs Nagy]

The 04/30/2022 23:06, Fangrui Song wrote:
> This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.
> 
> 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 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.

this looks fine. i guess bfd ld should warn/reject copy relocs too
since it wont work well with protected visibility.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

> --- a/sysdeps/aarch64/dl-sysdep.h
> +++ b/sysdeps/aarch64/dl-sysdep.h
> @@ -21,5 +21,3 @@
>  /* _dl_argv cannot be attribute_relro, because _dl_start_user
>     might write into it after _dl_start returns.  */
>  #define DL_ARGV_NOT_RELRO 1
> -
> -#define DL_EXTERN_PROTECTED_DATA

i think this file can be removed after rebase
(DL_ARGV_NOT_RELRO got removed)

Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"

[Fangrui Song]

On 2022-05-23, Szabolcs Nagy wrote:
>The 04/30/2022 23:06, Fangrui Song wrote:
>> This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.
>>
>> 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 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.
>
>this looks fine. i guess bfd ld should warn/reject copy relocs too
>since it wont work well with protected visibility.
>
>Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

Thanks for review. I'll check GNU ld's aarch64 port if I get time.

>> --- a/sysdeps/aarch64/dl-sysdep.h
>> +++ b/sysdeps/aarch64/dl-sysdep.h
>> @@ -21,5 +21,3 @@
>>  /* _dl_argv cannot be attribute_relro, because _dl_start_user
>>     might write into it after _dl_start returns.  */
>>  #define DL_ARGV_NOT_RELRO 1
>> -
>> -#define DL_EXTERN_PROTECTED_DATA
>
>i think this file can be removed after rebase
>(DL_ARGV_NOT_RELRO got removed)

Ack. Will remove this file.

Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"

[Fangrui Song]

On 2022-05-23, Fangrui Song wrote:
>On 2022-05-23, Szabolcs Nagy wrote:
>>The 04/30/2022 23:06, Fangrui Song wrote:
>>>This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.
>>>
>>>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 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.
>>
>>this looks fine. i guess bfd ld should warn/reject copy relocs too
>>since it wont work well with protected visibility.
>>
>>Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>
>Thanks for review. I'll check GNU ld's aarch64 port if I get time.

I sent a GNU ld patch for review:
https://sourceware.org/pipermail/binutils/2022-May/120970.html
("[PATCH] aarch64: Disallow copy relocations on protected data")

[PATCH 3/3] Revert "[ARM][BZ #17711] Fix extern protected data handling"

[Fangrui Song]

This reverts commit 3bcea719ddd6ce399d7bccb492c40af77d216e42.
---
 sysdeps/arm/dl-machine.h | 10 +++-------
 sysdeps/arm/dl-sysdep.h  |  2 --
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
index a7898bf420..2a7f795e46 100644
--- a/sysdeps/arm/dl-machine.h
+++ b/sysdeps/arm/dl-machine.h
@@ -212,22 +212,18 @@ _dl_start_user:\n\
    TLS variable, so undefined references should not be allowed to
    define the value.
    ELF_RTYPE_CLASS_COPY iff TYPE should not be allowed to resolve to one
-   of the main executable's symbols, as for a COPY reloc.
-   ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA iff TYPE describes relocation against
-   protected data whose address may be external due to copy relocation.  */
+   of the main executable's symbols, as for a COPY reloc.  */
 #ifndef RTLD_BOOTSTRAP
 # define elf_machine_type_class(type) \
   ((((type) == R_ARM_JUMP_SLOT || (type) == R_ARM_TLS_DTPMOD32		\
      || (type) == R_ARM_TLS_DTPOFF32 || (type) == R_ARM_TLS_TPOFF32	\
      || (type) == R_ARM_TLS_DESC)					\
     * ELF_RTYPE_CLASS_PLT)						\
-   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY)			\
-   | (((type) == R_ARM_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY))
 #else
 #define elf_machine_type_class(type) \
   ((((type) == R_ARM_JUMP_SLOT) * ELF_RTYPE_CLASS_PLT)	\
-   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY)	\
-   | (((type) == R_ARM_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY))
 #endif
 
 /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries.  */
diff --git a/sysdeps/arm/dl-sysdep.h b/sysdeps/arm/dl-sysdep.h
index ce7a84a7de..3099ee419f 100644
--- a/sysdeps/arm/dl-sysdep.h
+++ b/sysdeps/arm/dl-sysdep.h
@@ -21,5 +21,3 @@
 /* _dl_argv cannot be attribute_relro, because _dl_start_user
    might write into it after _dl_start returns.  */
 #define DL_ARGV_NOT_RELRO 1
-
-#define DL_EXTERN_PROTECTED_DATA
-- 
2.36.0.464.gb9c8b46e94-goog

Re: [PATCH 3/3] Revert "[ARM][BZ #17711] Fix extern protected data handling"

[Szabolcs Nagy]

The 04/30/2022 23:06, Fangrui Song wrote:
> This reverts commit 3bcea719ddd6ce399d7bccb492c40af77d216e42.

i'd reference the previous commit here for details.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

Re: [PATCH 3/3] Revert "[ARM][BZ #17711] Fix extern protected data handling"

[Fangrui Song]

On 2022-05-23, Szabolcs Nagy wrote:
>The 04/30/2022 23:06, Fangrui Song wrote:
>> This reverts commit 3bcea719ddd6ce399d7bccb492c40af77d216e42.
>
>i'd reference the previous commit here for details.
>
>Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>

Thanks:)

Added "Similar to commit e555954e026df1b85b8ef6c101d05f97b1520d7e for aarch64." to the commit message and pushed.
When I sent the patch series I did not have a commit id for this arm patch :)

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

[H.J. Lu]

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?


-- 
H.J.

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

[Fangrui Song]

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.

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.

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

[H.J. Lu]

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.



-- 
H.J.

Re: [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)))

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

[H.J. Lu]

On Wed, May 11, 2022 at 9:56 PM Fangrui Song <maskray@google.com> wrote:
>
> 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)))

Should it be handled in sysdeps/generic/dl-protected.h?

BTW, I'd like to see a migration plan to remove copy relocation and
use protected
visibility in shared libraries.   Qt uses protected/default
visibilities to mark exported
and imported symbols with GCC 12:

https://github.com/qt/qtbase/commit/19b7f854a274812d9c95fc7aaf134a12530c105f

Should glibc provide some EXPORT and IMPORT macros to help this?

--
H.J.

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

[Fangrui Song]

On 2022-05-13, H.J. Lu wrote:
>On Wed, May 11, 2022 at 9:56 PM Fangrui Song <maskray@google.com> wrote:
>>
>> 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)))
>
>Should it be handled in sysdeps/generic/dl-protected.h?

Yes, look like the suitable place. I think we should report a warning
regardless of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS, at least for
all non-x86 architectures.

For x86, gating the logic under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS may be fine
but I think it may be a bit overengineering as well.

>BTW, I'd like to see a migration plan to remove copy relocation and
>use protected
>visibility in shared libraries.   

As mentioned, the elf/dl-lookup.c code is to handle the case when a
protected variable is defined in multiple shared objects and the
executable copy relocates it. This is a pathologic case and does not
have clear semantics.  So IMO we should just remove the
ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code that let the second shared
object access its own copy.

I sent the patch series keeping ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA,
because I did not know whether you want to keep it for x86 :)

Note: the single protected data copy relocated by the executable does
not need any ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code to work. It
simply depends on whether the compiler and the linker cooperate and
produce a GLOB_DAT dynamic relocation.

>Qt uses protected/default
>visibilities to mark exported
>and imported symbols with GCC 12:
>
>https://github.com/qt/qtbase/commit/19b7f854a274812d9c95fc7aaf134a12530c105f

The Qt example belongs to the majority of cases where there is one
single definition for a protected variable.  Removing the dl-lookup.c
STV_PROTECTED code won't affect it.  If Qt ever defines the protected
variable in two shared objects, we can see that it is in a very broken
state.

And the original issue was really related to GCC 5's x86-64 decision to use
HAVE_LD_PIE_COPYRELOC. Other architectures don't have the problem.
In recent years most (if not all) distributions have switched to default
PIE. On all non-x86 architecture supporting shared objects, -fPIE
codegen uses an indirection for an external data symbol.

On the GCC side, we should just fix x86-64 -fpie by removing
HAVE_LD_PIE_COPYRELOC
(https://gcc.gnu.org/pipermail/gcc-patches/2021-November/582987.html).

>Should glibc provide some EXPORT and IMPORT macros to help this?

The -fno-pic codegen incompatibility with -Bsymbolic/protected/some
--dynamic-list has been there since like forever for architectures with
copy relocations. It doesn't help for glibc to tell GCC what to do.

Programs specifying -fno-pic likely rely on the existing behavior.
Changing GCC -fno-pic to default indirect access may be fine. That will
just let them opt out as they currently do for some security hardening
options pushed by Debian/Gentoo/RedHat/etc (e.g. -fno-stack-protector)