| Summary: | PIE binary with STT_GNU_IFUNC symbol and TEXTREL segfaults on x86_64 | ||
|---|---|---|---|
| Product: | binutils | Reporter: | Sriraman Tallam <tmsriram> |
| Component: | ld | Assignee: | Paul Pluzhnikov <ppluzhnikov> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | drepper.fsp, hjl.tools, iant, ppluzhnikov, tmsriram |
| Priority: | P2 | ||
| Version: | 2.26 | ||
| Target Milestone: | 2.26 | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
| Attachments: |
Preserve the original segment's execute permissions when protecting the page for writing it.
Preserve the original segment's execute permissions when protecting the page for writing it Preserve the original segment's execute permissions when protecting the page for writing it Preserve the original segment's execute permissions when protecting the page for writing it A patch to issue an error for read-only segment with dynamic IFUNC relocations |
||
(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. |
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.