Created attachment 8500 [details] Preserve the original segment's execute permissions when protecting the page for writing it. We have a PIE binary with TEXTREL and a STT_GNU_IFUNC symbol that segfaults at start-up. How to reproduce the problem: zoo.cc ------- int zoo_1 () { return 0; } extern "C" void *selector () { return (void *)&zoo_1; } int zoo() __attribute__ ((ifunc ("selector"))); int main() { return zoo (); } $ g++ -mcmodel=large -pie foo.cc $readelf -Wta ./a.out | grep TEXTREL 0x0000000000000016 (TEXTREL) 0x0 0x000000000000001e (FLAGS) TEXTREL $ ./a.out Segmentation Fault Notes: * Use mcmodel=large and -pie to create Text relocations. $ gdb ./a.out bt (gdb) bt #0 0x000055555555476b in selector () #1 0x00007ffff7de6638 in _dl_relocate_object () from /usr/grte/v4/lib64/ld-linux-x86-64.so.2 #2 0x00007ffff7ddc84d in dl_main () from /usr/grte/v4/lib64/ld-linux-x86-64.so.2 The segfault happens due to this: if (__glibc_unlikely (l->l_info[DT_TEXTREL] != NULL)) { ..... if (__mprotect (newp->start, newp->len, PROT_READ|PROT_WRITE) < 0) { ... } The execute permission of the PT_LOAD segment is removed in the process of marking for write. The attached patch seems to fix the problem.
(In reply to Sriraman Tallam from comment #0) > Created attachment 8500 [details] > Preserve the original segment's execute permissions when protecting the page > for writing it. > > We have a PIE binary with TEXTREL and a STT_GNU_IFUNC symbol that segfaults > at start-up. > > How to reproduce the problem: > > zoo.cc > ------- > int zoo_1 () { > return 0; > } > > extern "C" > void *selector () { > return (void *)&zoo_1; > } > > int zoo() __attribute__ ((ifunc ("selector"))); > > int main() { > return zoo (); > } > > $ g++ -mcmodel=large -pie foo.cc > > $readelf -Wta ./a.out | grep TEXTREL > 0x0000000000000016 (TEXTREL) 0x0 > 0x000000000000001e (FLAGS) TEXTREL > > $ ./a.out > Segmentation Fault > > Notes: > * Use mcmodel=large and -pie to create Text relocations. Please add the testcase to your patch. Please don't use __attribute__ ((ifunc ("selector"))); since older compilers don't support it.
Created attachment 8503 [details] Preserve the original segment's execute permissions when protecting the page for writing it Added test case.
Google ref: b/20921387
(In reply to Sriraman Tallam from comment #2) > Created attachment 8503 [details] > Preserve the original segment's execute permissions when protecting the page > for writing it > > Added test case. diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile index ef70a50..da06361 100644 --- a/sysdeps/x86_64/Makefile +++ b/sysdeps/x86_64/Makefile @@ -34,6 +34,12 @@ tests-pie += $(quad-pie-test) $(objpfx)tst-quad1pie: $(objpfx)tst-quadmod1pie.o $(objpfx)tst-quad2pie: $(objpfx)tst-quadmod2pie.o +ifunc-pie-txtrel-test += tst-pie-ifunc-txtrel +tests += $(ifunc-pie-txtrel-test) +tests-pie += $(ifunc-pie-txtrel-test) + +$(objpfx)tst-pie-ifunc-txtrel: $(objpfx)tst-pie-ifunc-txtrel.o + Do we need $(objpfx)tst-pie-ifunc-txtrel: $(objpfx)tst-pie-ifunc-txtrel.o
Created attachment 8504 [details] Preserve the original segment's execute permissions when protecting the page for writing it Patch Updated.
(In reply to Sriraman Tallam from comment #5) > Created attachment 8504 [details] > Preserve the original segment's execute permissions when protecting the page > for writing it > > Patch Updated. main: + movabsq $selector, %rax + call *%rax + movl $0, %eax Remove this line since foo returns 0. + ret
Created attachment 8505 [details] Preserve the original segment's execute permissions when protecting the page for writing it Patch updated.
(In reply to Sriraman Tallam from comment #7) > Created attachment 8505 [details] > Preserve the original segment's execute permissions when protecting the page > for writing it > > Patch updated. Looks good to me. Please send it to libc-alpha@sourceware.org. Thanks.
Linker should refuse to generate binary with STT_GNU_IFUNC symbol and TEXTREL.
On Tue, Aug 11, 2015 at 11:08 PM, hjl.tools at gmail dot com <sourceware-bugzilla@sourceware.org> wrote: > https://sourceware.org/bugzilla/show_bug.cgi?id=18801 > > --- Comment #9 from H.J. Lu <hjl.tools at gmail dot com> --- > Linker should refuse to generate binary with STT_GNU_IFUNC symbol and TEXTREL. Isnt this similar to execstack in some sense? I presume SELinux disallows that too but that we do not completely ban that on non-secure OS. I dont understand this as much as you do but I like Paul's idea here: "I think it would be nice to have behavior other than what's currently happening. Either ld.so should support TEXTREL binaries with IFUNCs, or it should refuse to run them. I guess it could also try to make W+E page, and IF that fails, issue a warning and change to current behavior. That way, a TEXTREL+IFUNC binary will run correctly outside SELinux, will warn, then crash under SELinux. A TEXTREL without IFUNC will also run correctly outside SELinux, and will warn but still work under SELinux (i.e. almost same as current behavior)." > > -- > You are receiving this mail because: > You are on the CC list for the bug. > You reported the bug.
Created attachment 8513 [details] A patch to issue an error for read-only segment with dynamic IFUNC relocations
(In reply to Sriraman Tallam from comment #10) > > Isnt this similar to execstack in some sense? I presume SELinux > disallows that too but that we do not completely ban that on > non-secure OS. Since ld.so isn't changed, there is no run-time behavior change. > I dont understand this as much as you do but I like Paul's idea here: > > "I think it would be nice to have behavior other than what's currently > happening. Either ld.so should support TEXTREL binaries with IFUNCs, > or it should refuse to run them. ld.so isn't responsible for user errors. > I guess it could also try to make W+E page, and IF that fails, issue a > warning and change to current behavior. That way, a TEXTREL+IFUNC > binary will run correctly outside SELinux, will warn, then crash under > SELinux. A TEXTREL without IFUNC will also run correctly outside > SELinux, and will warn but still work under SELinux (i.e. almost same > as current behavior)." My ld patch issues an error: ./ld: read-only segment has dynamic IFUNC relocations; recompile with -fPIC or link with -z execstack Take your pick for workaround.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8efa2874ab298f3923f4127340da119435f87c39 commit 8efa2874ab298f3923f4127340da119435f87c39 Author: H.J. Lu <hjl.tools@gmail.com> Date: Thu Aug 13 04:31:38 2015 -0700 Issue an error for read-only segment with dynamic IFUNC relocations To load an ELF binary with DT_TEXTREL tag, the dynamic linker calls __mprotect on the read-only segment with PROT_READ|PROT_WRITE before applying dynamic relocation. It leads to segfault when performing IFUNC relocations since the read-only segment has no execute permission. This patch changes x86 linker to issue an error for read-only segment with dynamic IFUNC relocations. Other backends with IFUNC support may need a similar change. bfd/ PR ld/18801 * elf32-i386.c (elf_i386_size_dynamic_sections): Issue an error for read-only segment with dynamic IFUNC relocations. * elf64-x86-64.c (elf_x86_64_size_dynamic_sections): Likewise. ld/testsuite/ PR ld/18801 * ld-i386/i386.exp: Run pr18801. * ld-x86-64/x86-64.exp: Likewise. * ld-i386/pr18801.d: New file. * ld-i386/pr18801.s: Likewise. * ld-x86-64/pr18801.d: Likewise. * ld-x86-64/pr18801.s: Likewise.
-z execstack can't used as a workaround. It works by accident: https://bugzilla.kernel.org/show_bug.cgi?id=102821 You have to compile PIE with -fPIE or -fPIC.
> FIXED What about Gold? Or any other linker that supports IFUNC and TEXTREL, but doesn't warn.
On Thu, Aug 13, 2015 at 7:22 AM, ppluzhnikov at google dot com <sourceware-bugzilla@sourceware.org> wrote: > https://sourceware.org/bugzilla/show_bug.cgi?id=18801 > > --- Comment #15 from Paul Pluzhnikov <ppluzhnikov at google dot com> --- >> FIXED > > What about Gold? Or any other linker that supports IFUNC and TEXTREL, but > doesn't warn. I will work on a equivalent patch for gold. > > -- > You are receiving this mail because: > You are on the CC list for the bug. > You reported the bug.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cebd6b8ac1c5a2a847a50e3efe932ff2d0867b3e commit cebd6b8ac1c5a2a847a50e3efe932ff2d0867b3e Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Jun 9 06:56:55 2020 -0700 IFUNC: Update IFUNC resolver check with DT_TEXTREL Add ifunc_resolvers to elf_link_hash_table and use it for both x86 and ppc64. Before glibc commit b5c45e837, DT_TEXTREL is incompatible with IFUNC resolvers. Set ifunc_resolvers if there are IFUNC resolvers and issue a warning for IFUNC resolvers with DT_TEXTREL. bfd/ PR ld/18801 * elf-bfd.h (elf_link_hash_table): Add ifunc_resolvers. (_bfd_elf_allocate_ifunc_dyn_relocs): Remove the bfd_boolean * argument. Set ifunc_resolvers if there are IFUNC resolvers. * elf-ifunc.c (_bfd_elf_allocate_ifunc_dyn_relocs): Updated. Set ifunc_resolvers if there are FUNC resolvers. * elf64-ppc.c (ppc_link_hash_table): Remove local_ifunc_resolver. (build_global_entry_stubs_and_plt): Replace local_ifunc_resolver with elf.ifunc_resolvers. (write_plt_relocs_for_local_syms): Likewise. (ppc64_elf_relocate_section): Likewise. (ppc64_elf_finish_dynamic_sections): Likewise. * elfnn-aarch64.c (elfNN_aarch64_allocate_ifunc_dynrelocs): Updated. * elfxx-x86.c (elf_x86_allocate_dynrelocs): Likewise. (_bfd_x86_elf_size_dynamic_sections): Check elf.ifunc_resolvers instead of readonly_dynrelocs_against_ifunc. * elfxx-x86.h (elf_x86_link_hash_table): Remove readonly_dynrelocs_against_ifunc. ld/ PR ld/18801 * testsuite/ld-i386/i386.exp: Run ifunc-textrel-1a, ifunc-textrel-1b, ifunc-textrel-2a and ifunc-textrel-2b. * testsuite/ld-x86-64/x86-64.exp: Likewise. * testsuite/ld-i386/ifunc-textrel-1a.d: Likewise. * testsuite/ld-i386/ifunc-textrel-1b.d: Likewise. * testsuite/ld-i386/ifunc-textrel-2a.d: Likewise. * testsuite/ld-i386/ifunc-textrel-2b.d: Likewise. * testsuite/ld-x86-64/ifunc-textrel-1.s: Likewise. * testsuite/ld-x86-64/ifunc-textrel-1a.d: Likewise. * testsuite/ld-x86-64/ifunc-textrel-1b.d: Likewise. * testsuite/ld-x86-64/ifunc-textrel-2.s: Likewise. * testsuite/ld-x86-64/ifunc-textrel-2a.d: Likewise. * testsuite/ld-x86-64/ifunc-textrel-2b.d: Likewise. * testsuite/ld-i386/pr18801a.d: Expect warning for IFUNC resolvers. * testsuite/ld-i386/pr18801b.d: Likewise. * estsuite/ld-x86-64/pr18801a.d: Likewise. * estsuite/ld-x86-64/pr18801b.d: Likewise.