Created attachment 9447 [details] Change the code that makes sections temporarily writable for relocation preserve execute permission Recent versions of the GNU C compiler provide a function attribute "ifunc" for access to an ELF facility that allows user provided code to be run at link time to determine the address of a symbol. However, during relocation, glibc temporarily maps some section executable by that code as PROT_READ|PROT_WRITE without setting PROT_EXEC. A real world consequence of this problem is that, on Linux i686, Qemu versions 2.6.0-rc0 and later crash during relocation (before main) if run from sudo on i686 Linux, which I think is because setuid programs clear the READ_IMPLIES_EXEC inheritable process flag, at least guessing from my reading of of linux-4.7/fs/exec.c, wherebprm_fill_uid does "bprm->per_clear |= PER_CLEAR_ON_SETID". PER_CLEAR_ON_SETID is bit mask defined in linux-4.7/include/uapi/linux/personality.h that includes READ_IMPLIES_EXEC. Single stepping through the Qemu crash seems to indicate that the permission violation is not in the first instruction of the ifunc code (which Qemu uses to select between functions with and without some special CPU instructions), but rather a helper function that pushes %ebx on the stack. Also, my attempt at making the most trivial test case failed. So, I think I need the ifunc to something slightly more complex to make a better case. I see from reading the code that the git glibc also appears to have this bug, but I have not tried it. Anyhow, I have attached a patch that works and that I like. It happens that the necessary permissions were computed immediately after the bad mprotect() call anyhow, so my patch just uses that with PROT_READ | PROT_WRITE masked in. Whether PROT_READ is necessary, I am not sure, but it is the least difference from the previous version of the code.
We need a full reproducer. This could well be a compiler or binutils bug. Can you provide more information, please? “eu-readelf -l -S” could be helpful, combined with the call stack and disassembly of the affected functions. The patch looks wrong. Some systems disallow PROT_WRITE|PROT_EXEC as a matter of policy.
Created attachment 9450 [details] Output of "eu-readelf -l -S ./x86_64-softmmu/qemu-system-x86_64" for qemu-2.6.0 Thanks for your thoughtful reply. Here is the output of eu-readelf that you requested. I will try to make time to develop a simpler test case.
Created attachment 9451 [details] gdb stack trace of qemu-2.6.0 getting a SIGSEGV due, I think, to ifunc not being mapped executable when run Also, here is the stack trace you requested, which I happened to have recorded, but I did not record the disassembly. To get the disassembly, I need to revert my glibc change temporarily to run this again, which I hope to do tomorrow. I hope this is helpful in the meantime.
Created attachment 9454 [details] Stack trace of qemu-2.6.0 on i686-pc-linux-gnu with disassembly of the function in the nonexecuable area that is about to be called, which will thereby generate a SIGSEGV Here is the stack trace with disassembly that you requested. By the way, this time, the first instruction of the function in the non-executable area generated a SIGSEGV, instead of the second instruction, as I previously recalled (the "pushl $ebx" instruction at the start of __x86.get_pc_thunk.bx). Although it is possible that that discrepancy was caused by some minor kernel bug that sporadically allows a few instructions to execute, such as perhaps a bug in mprotect on i386 related to instruction caching when PROT_EXEC goes from being set to cleared, I suspect I probably was just wrong in my recollection of that detail and may have accidentally been reading a trace of the non-failing (non-setuid) case.
Created attachment 9455 [details] Version 2 of proposed patch. This one should fall back to the old behavior if mprotect refuses PROT_WRITE|PROT_EXEC.
Regarding my version 2 patch, I want to mention that I can envision the following refinements, although I recommend integrating it in the meantime. a. To reduce the number of mprotect system calls made, remove the mprotect calls that I changed and instead require callers of _dl_relocate_object already to have all of the sections of the file being relocated mapped rwx (or, if that fails, rw, for systems that prohibit wx). b. Also to reduce the number of system calls, make part to reprotects memory skip the mprotect calls when the permissions are already what we want (rwx or rw, depending on whether rwx was allowed). c. Finally, and this would be a big change, to make ifuncs work on configurations that prohibit rwx memory, have a slower fallback that sets skip_ifuncs upon detecting that rwx memory is prohibited, and then have another pass of relocations after memory proections have been set to their final value that takes the result returned by the ifunc call, temporarily sets just the page(s) holding the ifunc target to writable, writes the ifunc result, and then restores their protections. Conceivably, this could be optimized by remembering multiple ifunc results before writing them back to the same page. This would be a big, slow, complex rarely used corner case, and I certainly would welcome alternative suggestions, but this is the only way that I have thought of so far for systems that do not allow read+write+execute memory mappings to support ifuncs (which I believe can be in any section, including the section of the target relocation). Also, I am convinced the problem is in glibc as I described. If you still think it could be in binutils, gcc, etc., I need you to describe a scenario, given the current information, for how that could be, if you want me to look into it. Meanwhile, I will try to generate a smaller test case to reproduce the problem. Thanks again for your response. I completely agree that we should support systems that prohibit rwx memory mappings, at the very least to the extent that we do so now (that is, non-ifunc code works always, and ifunc works for a kernel facility like READ_IMPLIES_EXEC).
I may have to eat my words "I am convinced the problem is in glibc as I described", at the very least the "as I described" part. The bug does not occur if qemu-2.6.0 is configured it with "--disable-pie"
Created attachment 9471 [details] A simple program to reproduce the problem. It tuns out the DT_TEXTREL is also necessary to trigger the problem (DT_TEXTREL + ifunc + setuid) Hi, Florian. I have finally made a reproducer C program. The realization that I was missing is that this problem only occurs in programs that have DT_TEXTREL set, which usually undesirable and correctable, but I think this problem still should be addressed, as I get the impression that use of ifunc is increasing. In particular, I saw a posting that implied that, at least for GNU ForTran, standard trigonometric now use ifunc (I don't know if that is true for "-lm" in general). Regarding Qemu, where I originally observed the problem, that was caused by it assuming that a couple of libraries that I had installed as static were available as shared libraries. For now, I have restored the shared libraries, and may later try to fix the Qemu build process, but there is nothing intrinsic to Qemu that needs this fix. It just happens to use ifunc is commonly invoked through a setuid program like sudo for certain use cases. I recommend applying the version 2 patch that I posted. It apparently solves this problem on systems that allow read+write+execute memory mapping, and gives the current behavior on systems that forbid it. Fixing the case for systems that prohibit read-write-execute memory mappings would be bigger project. In the meantime, if you do end up applying my patch or some similar partial fix, I suppose that the problem that remains (problem still happens if rwx is phohibited) should be documented somewhere. In case this will speed integration, I hereby release my copyright interest in anything that I have posted to this bugzilla bug report to the public domain. Please let me know if you have any questions or if I can be of any further assistance. Thank you for your attention to and analysis of this bug report.
Note that the sample program that I attached is a tar.gz file, because I included a Makefile to pass the specific CFLAGS and LDFLAGS needed to reproduce the problem and to provide a "make test" target to illustrate the problem.
I should also correct my statement about when the problem occurs. To produce the proboem, it is also necessary to compile the example with "-fPIE" and link it with "-pie". I am not sure why that is. So, the failing case is: ifunc + DT_TEXTREL + PIE + Linux setuid descendant. I suppose that makes the failure rarer, although I imagine that adoption is also increasing for the the position independent execution security feature. So, I would still recommend integrating the partial fix I submitted unless and until someone comes up with something better.
Updating my comment about not being certain why PIE is necessary to reproduce the problem, I think I understand now. The test case's code to induce DT_TEXTREL is in the main program. Without PIE, those text relocations would be done by the linker, so there would be no relocations in the .text segement for the dynamic linker to handle (no DT_TEXTREL). So, I think this problem also would occur when the other requirements are met if there were a .text section relocation in a ("-fPIC") shared library.
I agree that looks like a dynamic linker bug on i386. It seems that in other cases, the dynamic linker does not special markup for sections which contain TEXTREL relocations.
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, azanella/bz20480 has been created at 516ae423a269c0d40469ca9f6ecf3e3ea13f356b (commit) - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=516ae423a269c0d40469ca9f6ecf3e3ea13f356b commit 516ae423a269c0d40469ca9f6ecf3e3ea13f356b Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Mon Aug 27 16:16:43 2018 -0300 Fix ifunc support with DT_TEXTREL segments (BZ#20480) Currently, DT_TEXTREL is incompatible with IFUNC. When DT_TEXTREL or DF_TEXTREL is seen, the dynamic linker calls __mprotect on the segments with PROT_READ|PROT_WRITE before applying dynamic relocations. It leads to segfault when performing IFUNC resolution (which requires PROT_EXEC as well for the IFUNC resolver). This patch makes it call __mprotect with extra PROT_WRITE bit, which will keep the PROT_EXEC bit if exists, and thus fixes the segfault. FreeBSD rtld libexec/rtld-elf/rtld.c (reloc_textrel_prot) does the same. Adam J. Richte <adam_richter2004@yahoo.com> Adhemerval Zanella <adhemerval.zanella@linaro.org> Fangrui Song <maskray@google.com> [BZ #20480] * config.h.in (CAN_TEXTREL_IFUNC): New define. * configure.ac: Add check if linker supports textrel relocation with ifunc. * elf/dl-reloc.c (_dl_relocate_object): Use all required flags on DT_TEXTREL segments, not only PROT_READ and PROT_WRITE. * elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel. (CFLAGS-tst-ifunc-textrel.c): New rule. * elf/tst-ifunc-textrel.c: New file. -----------------------------------------------------------------------
DT_TEXTREL + R_X86_64_IRELATIVE relocations in r-x segment are sufficient. PIE or setuid is not a prerequisite. The SIGSEGV is easier to reproduce with lld. * ld.bfd and gold default to -z notext but emit DT_TEXTREL on demand * lld defaults to -z text, and emit DT_TEXTREL unconditionally if -z notext is passed % cat a.c void f_imp(void) {} void (*resolve_f(void))(void) { return f_imp; } void f(void) __attribute__((ifunc("resolve_f"))); int main(void) { f(); } % clang -fuse-ld=lld -Wl,-z,notext a.c -o a; ./a [1] 255838 segmentation fault ./a You may use GCC on Debian or some of its derivatives. Their gcc has a local patch to support -fuse-ld=lld % gcc -fuse-ld=lld -Wl,-z,notext a.c -o a; ./a [1] 259563 segmentation fault ./a Adam's patch looks good to me. I sent out https://sourceware.org/ml/libc-alpha/2018-08/msg00490.html unaware of this bug. There might be some security policy to prohibit rwx segments on some systems but systems that support this well should not see a silent SIGSEGV. I have checked that the program runs fine on FreeBSD because its rtld (libexec/rtld-elf/rtld.c:reloc_textrel_prot) does mprotect similar as these patches.
(In reply to Fangrui Song from comment #14) > DT_TEXTREL + R_X86_64_IRELATIVE relocations in r-x segment are sufficient. > PIE or setuid is not a prerequisite. > > The SIGSEGV is easier to reproduce with lld. > > * ld.bfd and gold default to -z notext but emit DT_TEXTREL on demand > * lld defaults to -z text, and emit DT_TEXTREL unconditionally if -z notext > is passed > > % cat a.c > void f_imp(void) {} > void (*resolve_f(void))(void) { return f_imp; } > void f(void) __attribute__((ifunc("resolve_f"))); > int main(void) { f(); } > > % clang -fuse-ld=lld -Wl,-z,notext a.c -o a; ./a > [1] 255838 segmentation fault ./a > > You may use GCC on Debian or some of its derivatives. Their gcc has a local > patch to support -fuse-ld=lld > > % gcc -fuse-ld=lld -Wl,-z,notext a.c -o a; ./a > [1] 259563 segmentation fault ./a > > > Adam's patch looks good to me. I sent out > https://sourceware.org/ml/libc-alpha/2018-08/msg00490.html unaware of this > bug. There might be some security policy to prohibit rwx segments on some > systems but systems that support this well should not see a silent SIGSEGV. > > I have checked that the program runs fine on FreeBSD because its rtld > (libexec/rtld-elf/rtld.c:reloc_textrel_prot) does mprotect similar as these > patches. I will replicate my comment [1] in libc-alpha since you moved the discussion back to this thread. The fix seems correct in imho, if we still aim to support textrel we should honor the expected flags in the segment for relocation (which implies PROT_EXEC in this case). I would say the it is QoI lack of lld to emit DT_TEXTREL for -z notext regardless text relocation existence. However it is still possible to create text relocation with ld.bfd and trigger the very issue. BZ#20480 has an example with explicit set an object segment to .text for force it. So I think we should add a testcase to stress it on architecture that supports ifunc. With adds another issue: ld.bfd behaviour for ifunc plus text relocation is inconsistent over releases and architecture. At least for powerpc, binutils 82e66161e (2.29+) reject linking binaries with 'text relocations and GNU indirect functions will result in a segfault at runtime'. On others architectures I have tested (arm, aarch64, sparc, s390, x86) ld.bfd does not fail to link, however powerpc sets the idea linker is free to reject such combination. It would require a configure test to check if linker support such features to add a test. I have add a testcase for this fix, along with the configure check, on a personal branch [2]. I have also cleanup a little the current code (the PF_TO_PROT micro-optimization really does not add much), and move the mprotect and check before setting the pointers. If you are ok with the change I can send upstream for review. PS: as a side node, afaik *BSD does not support ifunc. They just add the PROT_WRITE on defined segments (so they are not really susceptible to this issue since the segment does not really require PROT_EXEC). [1] https://sourceware.org/ml/libc-alpha/2018-08/msg00591.html [2] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/bz20480
Thanks for doing all these! https://sourceware.org/git/?p=glibc.git;a=commit;h=516ae423a269c0d40469ca9f6ecf3e3ea13f356b looks good.
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, azanella/bz20480 has been created at 48ebdd282daec74a530ed95ad67d609d8594a9de (commit) - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=48ebdd282daec74a530ed95ad67d609d8594a9de commit 48ebdd282daec74a530ed95ad67d609d8594a9de Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Mon Aug 27 16:16:43 2018 -0300 Fix ifunc support with DT_TEXTREL segments (BZ#20480) Currently, DT_TEXTREL is incompatible with IFUNC. When DT_TEXTREL or DF_TEXTREL is seen, the dynamic linker calls __mprotect on the segments with PROT_READ|PROT_WRITE before applying dynamic relocations. It leads to segfault when performing IFUNC resolution (which requires PROT_EXEC as well for the IFUNC resolver). This patch makes it call __mprotect with extra PROT_WRITE bit, which will keep the PROT_EXEC bit if exists, and thus fixes the segfault. FreeBSD rtld libexec/rtld-elf/rtld.c (reloc_textrel_prot) does the same. Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, sparc64-linux-gnu, sparcv9-linux-gnu, and armv8-linux-gnueabihf. Adam J. Richte <adam_richter2004@yahoo.com> Adhemerval Zanella <adhemerval.zanella@linaro.org> Fangrui Song <maskray@google.com> [BZ #20480] * config.h.in (CAN_TEXTREL_IFUNC): New define. * configure.ac: Add check if linker supports textrel relocation with ifunc. * elf/dl-reloc.c (_dl_relocate_object): Use all required flags on DT_TEXTREL segments, not only PROT_READ and PROT_WRITE. * elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel. (CFLAGS-tst-ifunc-textrel.c): New rule. * elf/tst-ifunc-textrel.c: New file. -----------------------------------------------------------------------
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, azanella/bz20480 has been created at 1823a9665c2d842db70c732eb3fe8a13c98f5319 (commit) - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=1823a9665c2d842db70c732eb3fe8a13c98f5319 commit 1823a9665c2d842db70c732eb3fe8a13c98f5319 Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Mon Aug 27 16:16:43 2018 -0300 Fix ifunc support with DT_TEXTREL segments (BZ#20480) Currently, DT_TEXTREL is incompatible with IFUNC. When DT_TEXTREL or DF_TEXTREL is seen, the dynamic linker calls __mprotect on the segments with PROT_READ|PROT_WRITE before applying dynamic relocations. It leads to segfault when performing IFUNC resolution (which requires PROT_EXEC as well for the IFUNC resolver). This patch makes it call __mprotect with extra PROT_WRITE bit, which will keep the PROT_EXEC bit if exists, and thus fixes the segfault. FreeBSD rtld libexec/rtld-elf/rtld.c (reloc_textrel_prot) does the same. Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, sparc64-linux-gnu, sparcv9-linux-gnu, and armv8-linux-gnueabihf. Adam J. Richte <adam_richter2004@yahoo.com> Adhemerval Zanella <adhemerval.zanella@linaro.org> Fangrui Song <maskray@google.com> [BZ #20480] * config.h.in (CAN_TEXTREL_IFUNC): New define. * configure.ac: Add check if linker supports textrel relocation with ifunc. * elf/dl-reloc.c (_dl_relocate_object): Use all required flags on DT_TEXTREL segments, not only PROT_READ and PROT_WRITE. * elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel. (CFLAGS-tst-ifunc-textrel.c): New rule. * elf/tst-ifunc-textrel.c: New file. -----------------------------------------------------------------------
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, master has been updated via b5c45e83753b27dc538dff2d55d4410c385cf3a4 (commit) from d62f9ec0cce26a275ec68f4564814041a33395b1 (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=b5c45e83753b27dc538dff2d55d4410c385cf3a4 commit b5c45e83753b27dc538dff2d55d4410c385cf3a4 Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Mon Aug 27 16:16:43 2018 -0300 Fix ifunc support with DT_TEXTREL segments (BZ#20480) Currently, DT_TEXTREL is incompatible with IFUNC. When DT_TEXTREL or DF_TEXTREL is seen, the dynamic linker calls __mprotect on the segments with PROT_READ|PROT_WRITE before applying dynamic relocations. It leads to segfault when performing IFUNC resolution (which requires PROT_EXEC as well for the IFUNC resolver). This patch makes it call __mprotect with extra PROT_WRITE bit, which will keep the PROT_EXEC bit if exists, and thus fixes the segfault. FreeBSD rtld libexec/rtld-elf/rtld.c (reloc_textrel_prot) does the same. Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, sparc64-linux-gnu, sparcv9-linux-gnu, and armv8-linux-gnueabihf. Adam J. Richte <adam_richter2004@yahoo.com> Adhemerval Zanella <adhemerval.zanella@linaro.org> Fangrui Song <maskray@google.com> [BZ #20480] * config.h.in (CAN_TEXTREL_IFUNC): New define. * configure.ac: Add check if linker supports textrel relocation with ifunc. * elf/dl-reloc.c (_dl_relocate_object): Use all required flags on DT_TEXTREL segments, not only PROT_READ and PROT_WRITE. * elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel. (CFLAGS-tst-ifunc-textrel.c): New rule. * elf/tst-ifunc-textrel.c: New file. ----------------------------------------------------------------------- Summary of changes: ChangeLog | 15 ++++++ config.make.in | 1 + configure | 47 ++++++++++++++++++++ configure.ac | 35 +++++++++++++++ elf/Makefile | 4 ++ elf/dl-reloc.c | 20 +++----- .../tst-cet-legacy-3.c => elf/tst-ifunc-textrel.c | 28 ++++++++---- 7 files changed, 128 insertions(+), 22 deletions(-) copy sysdeps/x86/tst-cet-legacy-3.c => elf/tst-ifunc-textrel.c (66%)
Fixed on 2.29 (b5c45e83753b27dc538dff2d55d4410c385cf3a4).