Re: [PATCH] x86: Make protected symbols local for -shared

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

On Mon, Jun 27, 2022 at 11:46 AM Fangrui Song <i@maskray.me> wrote:
>
> On 2022-06-27, H.J. Lu wrote:
> >On Mon, Jun 27, 2022 at 10:53 AM Fangrui Song <i@maskray.me> wrote:
> >>
> >> On 2022-06-27, H.J. Lu wrote:
> >> >On Mon, Jun 27, 2022 at 10:09 AM Fangrui Song <i@maskray.me> wrote:
> >> >>
> >> >> On 2022-06-27, H.J. Lu wrote:
> >> >> >On Sun, Jun 26, 2022 at 12:03 PM Fangrui Song <i@maskray.me> wrote:
> >> >> >>
> >> >> >> On 2022-06-26, H.J. Lu wrote:
> >> >> >> >On Sat, Jun 25, 2022 at 10:44 AM Fangrui Song <i@maskray.me> wrote:
> >> >> >> >>
> >> >> >> >> Call _bfd_elf_symbol_refs_local_p with local_protected==true.  This has
> >> >> >> >> 2 noticeable effects for -shared:
> >> >> >> >>
> >> >> >> >> * GOT-generating relocations referencing a protected data symbol no
> >> >> >> >>   longer lead to a GLOB_DAT (similar to a hidden symbol).
> >> >> >> >> * Direct access relocations (e.g. R_X86_64_PC32) no longer has the
> >> >> >> >>   confusing diagnostic below.
> >> >> >> >>
> >> >> >> >>     __attribute__((visibility("protected"))) void *foo() {
> >> >> >> >>       return (void *)foo;
> >> >> >> >>     }
> >> >> >> >>
> >> >> >> >>     // gcc -fpic -shared -fuse-ld=bfd
> >> >> >> >>     relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> >> >> >> >>
> >> >> >> >> The new behavior matches arm, aarch64 (commit
> >> >> >> >> 83c325007c5599fa9b60b8d5f7b84842160e1d1b), and powerpc ports, and other
> >> >> >> >> linkers: gold and ld.lld.
> >> >> >> >>
> >> >> >> >> Note: if some code tries to use direct access relocations to take the
> >> >> >> >> address of foo, the pointer equality will break, but the error should be
> >> >> >> >> reported on the executable link, not on the innocent shared object link.
> >> >> >> >> glibc 2.36 will give a warning at relocation resolving time.
> >> >> >> >
> >> >> >> >It should be controlled by -z [no]indirect-extern-access.   Can you enable
> >> >> >> >-z  indirect-extern-access with -shared by default instead?
> >> >> >>
> >> >> >> If I set `link_info.indirect_extern_access = 1;` in ld/ldmain.c,
> >> >> >> bfd/elf-properties.c:654 will create a
> >> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS note.
> >> >> >> This will probably be unexpected (and check-ld will have 280+ failures).
> >> >> >
> >> >> >This is normal when the default behavior is changed.  You can pass
> >> >> >-z noindirect-extern-access to these testcases.
> >> >>
> >> >> Adding GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS will be a
> >> >> significant behavior change and may unnecessarily break user programs
> >> >> (glibc will report an error instead of a warning).
> >> >
> >> >If glibc reports an error, it is a real bug with unknown consequences
> >> >when the copy in the executable is out of sync with the protected
> >> >symbol in the shared library,
> >>
> >> Not necessary.
> >>
> >> In glibc, GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS has two effects,
> >> 1 (copy relocations) and 2 (non-zero value of an undefined function
> >> symbol) on
> >> https://sourceware.org/pipermail/libc-alpha/2022-June/139552.html
> >>
> >> 2 does not necessarily cause a problem. In many cases it doesn't as
> >> function pointer equality is not an invariant a program relies upon
> >> (at least, for many functions, the property is not used). My previous
> >> comment has mentioned two cases.
> >>
> >> 1 likely causes a problem, but technically the shared object can define
> >> a protected data symbol without accessing it..
> >
> >These are unknown consequences.   We don't know what the worst
> >cases are.
>
> They are, just like when a shared object is linked with -Bsymbolic.

They have to deal with it since it is done on purpose.

> This patch focuses on changing the x86 default to a sane value (matching
> aarch64/arm/powerpc64/riscv/etc) and enabling future removal of
> `extern_protected_data`.  If you want to switch to
> indirect-extern-access default for x86, while I think unnecessary, I will not object.

extern_protected_data can be safely removed only when
direct access to external symbols are disallowed.   We can't
have both ways.

> But I'd note that we aren't really ready for the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS default.
> One major issue: -fPIE is widely used nowadays and GCC>=5 has the PIE copy relocation "regression".

-fno-PIE still has copy relocation.

> (My https://sourceware.org/pipermail/gcc-patches/2022-June/596678.html
> will fix it for future GCC, but the patch seems to get stuck since 2021-05-11.)
>
> >> >> If the executable takes the address of a protected function defined in a
> >> >> shared object, it may or may not cause a pointer equality problem (the
> >> >> shared object may not take the address) and the problem (if exists) may or
> >> >> may not be a broken invariance to the program (it may not expect pointer
> >> >> equality).
> >> >>
> >> >> All of aarch64/arm/powerpc64/riscv (likely most except x86, but I
> >> >> haven't enumerated) consider a protected data symbol local in -shared
> >> >> links. x86 did so a while ago (before 2015?).  (For
> >> >> aarch64/arm/powerpc64/riscv, I wish that we never need
> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS. The property will just
> >> >> waste some bytes in every shared object without carrying much
> >> >> information.)
> >> >>
> >> >> The 280+ failures in check-ld due to the default
> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS need to be considered as
> >> >> well.
> >> >>
> >> >> >> >> With this change, `#define elf_backend_extern_protected_data 1` is no
> >> >> >> >> longer effective.  Just remove it.
> >> >> >> >>
> >> >> >> >> Remove the test "Run protected-func-1 without PIE" since -fno-pic
> >> >> >> >> address taken operation in the executable doesn't work with protected
> >> >> >> >> symbol in a shared object by default.  Similarly, remove
> >> >> >> >> protected-data-1a and protected-data-1b.  protected-data-1b can be made
> >> >> >> >> working by removing HAVE_LD_PIE_COPYRELOC from GCC
> >> >> >> >> (https://sourceware.org/pipermail/gcc-patches/2022-June/596678.html).
> >> >> >> >> ---
> >> >> >> >>  bfd/elf32-i386.c                      |  1 -
> >> >> >> >>  bfd/elf64-x86-64.c                    |  1 -
> >> >> >> >>  bfd/elfxx-x86.c                       |  2 +-
> >> >> >> >>  ld/testsuite/ld-i386/protected1.d     |  4 +++-
> >> >> >> >>  ld/testsuite/ld-i386/protected3.d     |  2 +-
> >> >> >> >>  ld/testsuite/ld-i386/protected6a.d    |  4 +++-
> >> >> >> >>  ld/testsuite/ld-x86-64/pr24151a-x32.d |  4 +++-
> >> >> >> >>  ld/testsuite/ld-x86-64/pr24151a.d     |  4 +++-
> >> >> >> >>  ld/testsuite/ld-x86-64/protected1.d   |  4 +++-
> >> >> >> >>  ld/testsuite/ld-x86-64/protected3.d   |  2 +-
> >> >> >> >>  ld/testsuite/ld-x86-64/protected6a.d  |  4 +++-
> >> >> >> >>  ld/testsuite/ld-x86-64/protected7a.d  |  4 +++-
> >> >> >> >>  ld/testsuite/ld-x86-64/x86-64.exp     | 27 ---------------------------
> >> >> >> >>  13 files changed, 24 insertions(+), 39 deletions(-)
> >> >> >> >>
> >> >> >> >> diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
> >> >> >> >> index e4106d9fd3b..c3c46795731 100644
> >> >> >> >> --- a/bfd/elf32-i386.c
> >> >> >> >> +++ b/bfd/elf32-i386.c
> >> >> >> >> @@ -4424,7 +4424,6 @@ elf_i386_link_setup_gnu_properties (struct bfd_link_info *info)
> >> >> >> >>  #define elf_backend_got_header_size    12
> >> >> >> >>  #define elf_backend_plt_alignment      4
> >> >> >> >>  #define elf_backend_dtrel_excludes_plt 1
> >> >> >> >> -#define elf_backend_extern_protected_data 1
> >> >> >> >>  #define elf_backend_caches_rawsize     1
> >> >> >> >>  #define elf_backend_want_dynrelro      1
> >> >> >> >>
> >> >> >> >> diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
> >> >> >> >> index 6154a70bdd7..aaa5f1496b9 100644
> >> >> >> >> --- a/bfd/elf64-x86-64.c
> >> >> >> >> +++ b/bfd/elf64-x86-64.c
> >> >> >> >> @@ -5275,7 +5275,6 @@ elf_x86_64_special_sections[]=
> >> >> >> >>  #define elf_backend_got_header_size        (GOT_ENTRY_SIZE*3)
> >> >> >> >>  #define elf_backend_rela_normal                    1
> >> >> >> >>  #define elf_backend_plt_alignment          4
> >> >> >> >> -#define elf_backend_extern_protected_data   1
> >> >> >> >>  #define elf_backend_caches_rawsize         1
> >> >> >> >>  #define elf_backend_dtrel_excludes_plt     1
> >> >> >> >>  #define elf_backend_want_dynrelro          1
> >> >> >> >> diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
> >> >> >> >> index acb2cc8528d..18f3d335458 100644
> >> >> >> >> --- a/bfd/elfxx-x86.c
> >> >> >> >> +++ b/bfd/elfxx-x86.c
> >> >> >> >> @@ -3094,7 +3094,7 @@ _bfd_x86_elf_link_symbol_references_local (struct bfd_link_info *info,
> >> >> >> >>       2. When building executable, there is no dynamic linker.  Or
> >> >> >> >>       3. or "-z nodynamic-undefined-weak" is used.
> >> >> >> >>     */
> >> >> >> >> -  if (SYMBOL_REFERENCES_LOCAL (info, h)
> >> >> >> >> +  if (_bfd_elf_symbol_refs_local_p (h, info, 1)
> >> >> >> >>        || (h->root.type == bfd_link_hash_undefweak
> >> >> >> >>           && (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
> >> >> >> >>               || (bfd_link_executable (info)
> >> >> >> >> diff --git a/ld/testsuite/ld-i386/protected1.d b/ld/testsuite/ld-i386/protected1.d
> >> >> >> >> index a3cb5cef140..531645b8fe8 100644
> >> >> >> >> --- a/ld/testsuite/ld-i386/protected1.d
> >> >> >> >> +++ b/ld/testsuite/ld-i386/protected1.d
> >> >> >> >> @@ -1,3 +1,5 @@
> >> >> >> >>  #as: --32
> >> >> >> >>  #ld: -shared -melf_i386
> >> >> >> >> -#error: .*relocation R_386_GOTOFF against protected function `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-i386/protected3.d b/ld/testsuite/ld-i386/protected3.d
> >> >> >> >> index c3a6888d900..77367c4738f 100644
> >> >> >> >> --- a/ld/testsuite/ld-i386/protected3.d
> >> >> >> >> +++ b/ld/testsuite/ld-i386/protected3.d
> >> >> >> >> @@ -8,7 +8,7 @@
> >> >> >> >>  Disassembly of section .text:
> >> >> >> >>
> >> >> >> >>  0+[a-f0-9]+ <bar>:
> >> >> >> >> -[      ]*[a-f0-9]+:    8b 81 [a-f0-9][a-f0-9] [a-f0-9][a-f0-9] ff ff           mov    -0x[a-f0-9]+\(%ecx\),%eax
> >> >> >> >> +[      ]*[a-f0-9]+:    8d 81 00 00 00 00       lea    0x0\(%ecx\),%eax
> >> >> >> >>  [      ]*[a-f0-9]+:    8b 00                   mov    \(%eax\),%eax
> >> >> >> >>  [      ]*[a-f0-9]+:    c3                      ret
> >> >> >> >>  #pass
> >> >> >> >> diff --git a/ld/testsuite/ld-i386/protected6a.d b/ld/testsuite/ld-i386/protected6a.d
> >> >> >> >> index 7dc350432f4..4d3873239f9 100644
> >> >> >> >> --- a/ld/testsuite/ld-i386/protected6a.d
> >> >> >> >> +++ b/ld/testsuite/ld-i386/protected6a.d
> >> >> >> >> @@ -1,4 +1,6 @@
> >> >> >> >>  #source: protected6.s
> >> >> >> >>  #as: --32
> >> >> >> >>  #ld: -shared -melf_i386
> >> >> >> >> -#error: .*relocation R_386_GOTOFF against protected data `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/pr24151a-x32.d b/ld/testsuite/ld-x86-64/pr24151a-x32.d
> >> >> >> >> index 130611ddf49..1f49b655f7d 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/pr24151a-x32.d
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/pr24151a-x32.d
> >> >> >> >> @@ -1,4 +1,6 @@
> >> >> >> >>  #source: pr24151a.s
> >> >> >> >>  #as: --x32
> >> >> >> >>  #ld: -shared -melf32_x86_64
> >> >> >> >> -#error: .*relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/pr24151a.d b/ld/testsuite/ld-x86-64/pr24151a.d
> >> >> >> >> index 783b85a1a6f..6c48e383e01 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/pr24151a.d
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/pr24151a.d
> >> >> >> >> @@ -1,3 +1,5 @@
> >> >> >> >>  #as: --64
> >> >> >> >>  #ld: -shared -melf_x86_64
> >> >> >> >> -#error: .*relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/protected1.d b/ld/testsuite/ld-x86-64/protected1.d
> >> >> >> >> index 783b85a1a6f..6c48e383e01 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/protected1.d
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/protected1.d
> >> >> >> >> @@ -1,3 +1,5 @@
> >> >> >> >>  #as: --64
> >> >> >> >>  #ld: -shared -melf_x86_64
> >> >> >> >> -#error: .*relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/protected3.d b/ld/testsuite/ld-x86-64/protected3.d
> >> >> >> >> index 57950e4d6b6..ba63991582f 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/protected3.d
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/protected3.d
> >> >> >> >> @@ -8,7 +8,7 @@
> >> >> >> >>  Disassembly of section .text:
> >> >> >> >>
> >> >> >> >>  0+[a-f0-9]+ <bar>:
> >> >> >> >> -[      ]*[a-f0-9]+:    48 8b 05 ([0-9a-f]{2} ){4} *    mov    0x[a-f0-9]+\(%rip\),%rax        # [a-f0-9]+ <.*>
> >> >> >> >> +[      ]*[a-f0-9]+:    48 8d 05 ([0-9a-f]{2} ){4} *    lea    0x[a-f0-9]+\(%rip\),%rax        # [a-f0-9]+ <.*>
> >> >> >> >>  [      ]*[a-f0-9]+:    8b 00                   mov    \(%rax\),%eax
> >> >> >> >>  [      ]*[a-f0-9]+:    c3                      ret
> >> >> >> >>  #pass
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/protected6a.d b/ld/testsuite/ld-x86-64/protected6a.d
> >> >> >> >> index 3a7963ffd2f..50d6430b577 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/protected6a.d
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/protected6a.d
> >> >> >> >> @@ -1,4 +1,6 @@
> >> >> >> >>  #source: protected6.s
> >> >> >> >>  #as: --64
> >> >> >> >>  #ld: -shared -melf_x86_64
> >> >> >> >> -#error: .*relocation R_X86_64_GOTOFF64 against protected data `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/protected7a.d b/ld/testsuite/ld-x86-64/protected7a.d
> >> >> >> >> index 3082084a7b8..3974246a2a8 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/protected7a.d
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/protected7a.d
> >> >> >> >> @@ -1,4 +1,6 @@
> >> >> >> >>  #source: protected7.s
> >> >> >> >>  #as: --64
> >> >> >> >>  #ld: -shared -melf_x86_64
> >> >> >> >> -#error: .*relocation R_X86_64_GOTOFF64 against protected function `foo' can not be used when making a shared object
> >> >> >> >> +#readelf: -rW
> >> >> >> >> +#...
> >> >> >> >> +There are no relocations in this file.
> >> >> >> >> diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
> >> >> >> >> index 5e5636bcebe..a096c0b9d0f 100644
> >> >> >> >> --- a/ld/testsuite/ld-x86-64/x86-64.exp
> >> >> >> >> +++ b/ld/testsuite/ld-x86-64/x86-64.exp
> >> >> >> >> @@ -1832,15 +1832,6 @@ if { [isnative] && [check_compiler_available] } {
> >> >> >> >>             "pr23997" \
> >> >> >> >>             "pass.out" \
> >> >> >> >>         ] \
> >> >> >> >> -       [list \
> >> >> >> >> -           "Run protected-func-1 without PIE" \
> >> >> >> >> -           "$NOPIE_LDFLAGS -Wl,--no-as-needed tmpdir/libprotected-func-1.so" \
> >> >> >> >> -           "-Wa,-mx86-used-note=yes" \
> >> >> >> >> -           { protected-func-1b.c } \
> >> >> >> >> -           "protected-func-1a" \
> >> >> >> >> -           "pass.out" \
> >> >> >> >> -           "$NOPIE_CFLAGS" \
> >> >> >> >> -       ] \
> >> >> >> >>         [list \
> >> >> >> >>             "Run protected-func-1 with PIE" \
> >> >> >> >>             "-Wl,--no-as-needed -pie tmpdir/libprotected-func-1.so" \
> >> >> >> >> @@ -1904,24 +1895,6 @@ if { [isnative] && [check_compiler_available] } {
> >> >> >> >>             "pass.out" \
> >> >> >> >>             "-fPIE" \
> >> >> >> >>         ] \
> >> >> >> >> -       [list \
> >> >> >> >> -           "Run protected-data-1a without PIE" \
> >> >> >> >> -           "$NOPIE_LDFLAGS -Wl,--no-as-needed tmpdir/libprotected-data-1a.so" \
> >> >> >> >> -           "-Wa,-mx86-used-note=yes" \
> >> >> >> >> -           { protected-data-1b.c } \
> >> >> >> >> -           "protected-data-1a" \
> >> >> >> >> -           "pass.out" \
> >> >> >> >> -           "$NOPIE_CFLAGS" \
> >> >> >> >> -       ] \
> >> >> >> >> -       [list \
> >> >> >> >> -           "Run protected-data-1b with PIE" \
> >> >> >> >> -           "-Wl,--no-as-needed -pie tmpdir/libprotected-data-1a.so" \
> >> >> >> >> -           "-Wa,-mx86-used-note=yes" \
> >> >> >> >> -           { protected-data-1b.c } \
> >> >> >> >> -           "protected-data-1b" \
> >> >> >> >> -           "pass.out" \
> >> >> >> >> -           "-fPIE" \
> >> >> >> >> -       ] \
> >> >> >> >>         [list \
> >> >> >> >>             "Run protected-data-2a without PIE" \
> >> >> >> >>             "$NOPIE_LDFLAGS -Wl,--no-as-needed tmpdir/libprotected-data-2a.so" \
> >> >> >> >> --
> >> >> >> >> 2.37
> >> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> >--
> >> >> >> >H.J.
> >> >> >
> >> >> >
> >> >> >
> >> >> >--
> >> >> >H.J.
> >> >
> >> >
> >> >
> >> >--
> >> >H.J.
> >
> >
> >
> >--
> >H.J.



-- 
H.J.