[PATCH] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[PATCH] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[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.  The behavior change is from commit
62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then copied to nios2
(ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc
(0e7d930c4c11de896fe807f67fa1eb756c9c1e05).

Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
will bind to the executable (normal behavior).

It's extremely unlikely anyone relies on the
ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so just remove it.
---
 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
@@ -456,59 +456,6 @@ do_lookup_x (const char *undef_name, unsigned int new_hash,
       if (sym != NULL)
 	{
 	found_it:
-	  /* When UNDEF_MAP is NULL, which indicates we are called from
-	     do_lookup_x on relocation against protected data, we skip
-	     the data definion in the executable from copy reloc.  */
-	  if (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
-	      && undef_map == NULL
-	      && map->l_type == lt_executable
-	      && type_class == ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA)
-	    {
-	      const ElfW(Sym) *s;
-	      unsigned int i;
-
-#if ! ELF_MACHINE_NO_RELA
-	      if (map->l_info[DT_RELA] != NULL
-		  && map->l_info[DT_RELASZ] != NULL
-		  && map->l_info[DT_RELASZ]->d_un.d_val != 0)
-		{
-		  const ElfW(Rela) *rela
-		    = (const ElfW(Rela) *) D_PTR (map, l_info[DT_RELA]);
-		  unsigned int rela_count
-		    = map->l_info[DT_RELASZ]->d_un.d_val / sizeof (*rela);
-
-		  for (i = 0; i < rela_count; i++, rela++)
-		    if (elf_machine_type_class (ELFW(R_TYPE) (rela->r_info))
-			== ELF_RTYPE_CLASS_COPY)
-		      {
-			s = &symtab[ELFW(R_SYM) (rela->r_info)];
-			if (!strcmp (strtab + s->st_name, undef_name))
-			  goto skip;
-		      }
-		}
-#endif
-#if ! ELF_MACHINE_NO_REL
-	      if (map->l_info[DT_REL] != NULL
-		  && map->l_info[DT_RELSZ] != NULL
-		  && map->l_info[DT_RELSZ]->d_un.d_val != 0)
-		{
-		  const ElfW(Rel) *rel
-		    = (const ElfW(Rel) *) D_PTR (map, l_info[DT_REL]);
-		  unsigned int rel_count
-		    = map->l_info[DT_RELSZ]->d_un.d_val / sizeof (*rel);
-
-		  for (i = 0; i < rel_count; i++, rel++)
-		    if (elf_machine_type_class (ELFW(R_TYPE) (rel->r_info))
-			== ELF_RTYPE_CLASS_COPY)
-		      {
-			s = &symtab[ELFW(R_SYM) (rel->r_info)];
-			if (!strcmp (strtab + s->st_name, undef_name))
-			  goto skip;
-		      }
-		}
-#endif
-	    }
-
 	  /* Hidden and internal symbols are local, ignore them.  */
 	  if (__glibc_unlikely (dl_symbol_visibility_binds_local_p (sym)))
 	    goto skip;
@@ -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;
-	    }
-	}
-    }
-
   /* We have to check whether this would bind UNDEF_MAP to an object
      in the global scope which was dynamically loaded.  In this case
      we have to prevent the latter from being unloaded unless the
diff --git a/sysdeps/arc/dl-sysdep.h b/sysdeps/arc/dl-sysdep.h
deleted file mode 100644
index cf4d160a73..0000000000
--- a/sysdeps/arc/dl-sysdep.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/* System-specific settings for dynamic linker code.  ARC version.
-   Copyright (C) 2020-2022 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include_next <dl-sysdep.h>
-
-#define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 6716e1f382..d3cbfd4f95 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -149,23 +149,13 @@ dl_symbol_visibility_binds_local_p (const ElfW(Sym) *sym)
    satisfied by any symbol in the executable.  Some architectures do
    not support copy relocations.  In this case we define the macro to
    zero so that the code for handling them gets automatically optimized
-   out.  ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA means address of protected
-   data defined in the shared library may be external, i.e., due to copy
-   relocation.  */
+   out.  */
 #define ELF_RTYPE_CLASS_PLT 1
 #ifndef DL_NO_COPY_RELOCS
 # define ELF_RTYPE_CLASS_COPY 2
 #else
 # define ELF_RTYPE_CLASS_COPY 0
 #endif
-/* If DL_EXTERN_PROTECTED_DATA is defined, address of protected data
-   defined in the shared library may be external, i.e., due to copy
-   relocation.   */
-#ifdef DL_EXTERN_PROTECTED_DATA
-# define ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA 4
-#else
-# define ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA 0
-#endif
 
 /* ELF uses the PF_x macros to specify the segment permissions, mmap
    uses PROT_xxx.  In most cases the three macros have the values 1, 2,
diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 1f8d734215..3311631a3e 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -203,8 +203,7 @@ _dl_start_user:\n\
      || (type) == R_386_TLS_DTPOFF32 || (type) == R_386_TLS_TPOFF32	      \
      || (type) == R_386_TLS_TPOFF || (type) == R_386_TLS_DESC)		      \
     * ELF_RTYPE_CLASS_PLT)						      \
-   | (((type) == R_386_COPY) * ELF_RTYPE_CLASS_COPY)			      \
-   | (((type) == R_386_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_386_COPY) * ELF_RTYPE_CLASS_COPY))
 
 /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries.  */
 #define ELF_MACHINE_JMP_SLOT	R_386_JMP_SLOT
diff --git a/sysdeps/nios2/dl-sysdep.h b/sysdeps/nios2/dl-sysdep.h
deleted file mode 100644
index 257b37c258..0000000000
--- a/sysdeps/nios2/dl-sysdep.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/* System-specific settings for dynamic linker code.  Nios II version.
-   Copyright (C) 2009-2022 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include_next <dl-sysdep.h>
-
-#define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/x86/dl-lookupcfg.h b/sysdeps/x86/dl-lookupcfg.h
index 18b3b49f6e..e136cc63af 100644
--- a/sysdeps/x86/dl-lookupcfg.h
+++ b/sysdeps/x86/dl-lookupcfg.h
@@ -20,10 +20,6 @@
 
 #include_next <dl-lookupcfg.h>
 
-/* Address of protected data defined in the shared library may be
-   external due to copy relocation.   */
-#define DL_EXTERN_PROTECTED_DATA
-
 struct link_map;
 
 extern void _dl_unmap (struct link_map *map) attribute_hidden;
diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index 7f607f6dff..78aecfc9fd 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -181,10 +181,7 @@ _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 may
-   against protected data whose address be external due to copy relocation.
- */
+   of the main executable's symbols, as for a COPY reloc.  */
 #define elf_machine_type_class(type)					      \
   ((((type) == R_X86_64_JUMP_SLOT					      \
      || (type) == R_X86_64_DTPMOD64					      \
@@ -192,8 +189,7 @@ _dl_start_user:\n\
      || (type) == R_X86_64_TPOFF64					      \
      || (type) == R_X86_64_TLSDESC)					      \
     * ELF_RTYPE_CLASS_PLT)						      \
-   | (((type) == R_X86_64_COPY) * ELF_RTYPE_CLASS_COPY)			      \
-   | (((type) == R_X86_64_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_X86_64_COPY) * ELF_RTYPE_CLASS_COPY))
 
 /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries.  */
 #define ELF_MACHINE_JMP_SLOT	R_X86_64_JUMP_SLOT
-- 
2.36.1.124.g0e6072fb45-goog

Re: [PATCH] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[Szabolcs Nagy]

The 05/27/2022 21:42, Fangrui Song via Libc-alpha 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.  The behavior change is from commit
> 62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then copied to nios2
> (ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc
> (0e7d930c4c11de896fe807f67fa1eb756c9c1e05).
> 
> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
> will bind to the executable (normal behavior).

i think this should be reworded so all of this is under
assumptions (a) and (b).

otherwise protected access may (partially) resolve locally in
the dso and then copy relocation in the exe cannot work.

> 
> It's extremely unlikely anyone relies on the
> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so just remove it.

note that glibc has

elf/tst-protected1a
elf/tst-protected1b

tests that XFAIL when glibc is built with --disable-default-pie
(copy relocs in exe, GOT relocs in dso).

i think the test should be updated to reflect the cases that are
actually supported. (although if we reject copy relocs for
protected symbols in ld then the test may not work at all)

i think the exe now can interpose a protected symbol in a dso
if that uses GOT relocs. (there is no interposition if the
linker binds the protected symbol locally of course). i think
the commit should mention this change too.

> 
> It's extremely unlikely anyone relies on the
> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so just remove it.

Re: [PATCH] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[Fangrui Song]

On 2022-05-30, Szabolcs Nagy wrote:
>The 05/27/2022 21:42, Fangrui Song via Libc-alpha 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.  The behavior change is from commit
>> 62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then copied to nios2
>> (ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc
>> (0e7d930c4c11de896fe807f67fa1eb756c9c1e05).
>>
>> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>> will bind to the executable (normal behavior).
>
>i think this should be reworded so all of this is under
>assumptions (a) and (b).

How should I reword this, concretely? :)

>otherwise protected access may (partially) resolve locally in
>the dso and then copy relocation in the exe cannot work.
>
>>
>> It's extremely unlikely anyone relies on the
>> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so just remove it.
>
>note that glibc has
>
>elf/tst-protected1a
>elf/tst-protected1b
>
>tests that XFAIL when glibc is built with --disable-default-pie
>(copy relocs in exe, GOT relocs in dso).
>
>i think the test should be updated to reflect the cases that are
>actually supported. 

I think the tests currently XPASS on x86 with GNU ld and XFAIL on a
bunch of architectures. I do have a plan (after this
is accepted) to remove the `if (&protected3 == protected3b_p ())` part
and remove `test-xfail-tst-protected1a = yes`)

>(although if we reject copy relocs for
>protected symbols in ld then the test may not work at all)

Right. libc_cv_protected_data in configure.ac should be changed
to test whether copy relocations on protected data works,
instead of testing just whether protected data compiles and links.

For aarch64, I plan to make GCC and GNU ld behaviors match clang and gold/ld.lld, the IMO ideal semantics.  The full roadmap is
https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#summary

>i think the exe now can interpose a protected symbol in a dso
>if that uses GOT relocs. (there is no interposition if the
>linker binds the protected symbol locally of course). i think
>the commit should mention this change too.

This case (one dso definition copy relocated by exe) does not change.
Even with the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, the definition in
the first dso can be interposed if it uses GOT relocs.

The ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code affects the (multiple dso
definitions, with the first one copy relocated by exe) case.

>>
>> It's extremely unlikely anyone relies on the
>> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so just remove it.

Re: [PATCH] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[Szabolcs Nagy]

The 05/30/2022 23:48, Fangrui Song wrote:
> On 2022-05-30, Szabolcs Nagy wrote:
> > The 05/27/2022 21:42, Fangrui Song via Libc-alpha 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.  The behavior change is from commit
> > > 62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then copied to nios2
> > > (ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc
> > > (0e7d930c4c11de896fe807f67fa1eb756c9c1e05).
> > > 
> > > Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
> > > will bind to the executable (normal behavior).
> > 
> > i think this should be reworded so all of this is under
> > assumptions (a) and (b).
> 
> How should I reword this, concretely? :)

how about:

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) ... (b) ... .  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 the dynamic linker 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_... has strange semantics when both a.so and b.so define
protected var and the executable copy relocates var: a.so accesses the
copy in ...
The behaviour change is from commit ...

Without the ELF_RTYPE_...

There is no change when copy relocations are not present or when only
one library has definition for a protected variable.  It's extremely
unlikely that anyone relies on ...

> I think the tests currently XPASS on x86 with GNU ld and XFAIL on a
> bunch of architectures. I do have a plan (after this
> is accepted) to remove the `if (&protected3 == protected3b_p ())` part
> and remove `test-xfail-tst-protected1a = yes`)

sounds good. (this can be a separate patch)

> > (although if we reject copy relocs for
> > protected symbols in ld then the test may not work at all)
> 
> Right. libc_cv_protected_data in configure.ac should be changed
> to test whether copy relocations on protected data works,
> instead of testing just whether protected data compiles and links.
> 
> For aarch64, I plan to make GCC and GNU ld behaviors match clang and gold/ld.lld, the IMO ideal semantics.  The full roadmap is
> https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#summary

this makes sense to me.

> > i think the exe now can interpose a protected symbol in a dso
> > if that uses GOT relocs. (there is no interposition if the
> > linker binds the protected symbol locally of course). i think
> > the commit should mention this change too.
> 
> This case (one dso definition copy relocated by exe) does not change.
> Even with the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, the definition in
> the first dso can be interposed if it uses GOT relocs.
> 
> The ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code affects the (multiple dso
> definitions, with the first one copy relocated by exe) case.

either this should be fixed or documented:

on aarch64 this patch vs the extern protected change reverted:

$ elf/ld.so-old --library-path . ./a.out
var in dso: 0xffffa6d50020 2
var in exe: 0x420038 1
$ elf/ld.so-new --library-path . ./a.out
var in dso: 0x420038 1
var in exe: 0x420038 1

$ cat a.c
#include <stdio.h>
void dso_call(void);
int foobar = 1;
int main()
{
  dso_call();
  printf("var in exe: %p %d\n", &foobar, foobar);
}

$ cat dso.c
#include <stdio.h>
__attribute__((visibility("protected"))) int foobar = 2;
void dso_call(void)
{
  printf("var in dso: %p %d\n", &foobar, foobar);
}

$ aarch64-none-linux-gnu-readelf -aW dso.so |grep foobar
000000000001ffd0  0000000800000401 R_AARCH64_GLOB_DAT     0000000000020020 foobar + 0
     8: 0000000000020020     4 OBJECT  GLOBAL PROTECTED   22 foobar
    74: 0000000000020020     4 OBJECT  GLOBAL PROTECTED   22 foobar

$ aarch64-none-linux-gnu-readelf -aW a.out |grep foobar
     8: 0000000000420038     4 OBJECT  GLOBAL DEFAULT   24 foobar
    90: 0000000000420038     4 OBJECT  GLOBAL DEFAULT   24 foobar