Message ID | CAAs8Hmy32PV1z0D7So6TEzFosCyJNUB_yco_6SYi=tKHUpBMQg@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Tue, Aug 11, 2015 at 2:21 PM, Sriraman Tallam <tmsriram@google.com> wrote: > Details here: > https://sourceware.org/bugzilla/show_bug.cgi?id=18801 > > Thanks to Paul Pluzhnikov for identifying the problem and suggesting the fix. I'll note that this will cause any TEXTREL binary to fail under SELinux config that prohibits "W+E" permissions. But I think there are few such binaries. It's either - make TEXTREL binary not run under SELinux, or - make them run, but crash mysteriously if they have a called IFUNC resolver in them (or are linked with '-z,now'). > PR 18801 > * elf/dl-reloc.c (_dl_relocate_object): Preserve the > original permissions when protecting the segment for writing it. > * sysdeps/x86_64/Makefile (ifunc-pie-txtrel-test): New test. > * sysdeps/x86_64/tst-pie-ifunc-txtrel.S: New file. The canonical ChangeLog entry format is: 2015-08-11 Sriraman Tallam <tmsriram@google.com> Paul Pluzhnikov <ppluzhnikov@google.com> [BZ #18801] * elf/dl-reloc.c (_dl_relocate_object): Don't remove execute permissions when making segment writable. * sysdeps/x86_64/Makefile (ifunc-pie-txtrel-test): New test. * sysdeps/x86_64/tst-pie-ifunc-txtrel.S: New file.
On Tue, Aug 11, 2015 at 2:39 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > On Tue, Aug 11, 2015 at 2:21 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> Details here: >> https://sourceware.org/bugzilla/show_bug.cgi?id=18801 >> >> Thanks to Paul Pluzhnikov for identifying the problem and suggesting the fix. > > I'll note that this will cause any TEXTREL binary to fail under > SELinux config that prohibits "W+E" permissions. But I think there are > few such binaries. > > It's either > - make TEXTREL binary not run under SELinux, or > - make them run, but crash mysteriously if they have a called IFUNC > resolver in them (or are linked with '-z,now'). How about 1. Change ld to disallow TEXTREL with IFUNC and without "-z now'". Or 2. Change ld to set DT_BIND_NOW if there is TEXTREL with IFUNC. Or 3. Update ld to set a new DT_XXXX if there TEXTREL with IFUNC and ld.so will call mprotect with PROT_EXEC only if there is DT_XXXX. My preference is #1, #2, #3.
On Tue, Aug 11, 2015 at 3:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Aug 11, 2015 at 2:39 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >> It's either >> - make TEXTREL binary not run under SELinux, or >> - make them run, but crash mysteriously if they have a called IFUNC >> resolver in them (or are linked with '-z,now'). > > How about > > 1. Change ld to disallow TEXTREL with IFUNC and without "-z now'". That would still fail under SELinux, wouldn't it? Are you proposing also changing SELinux policy to allow "W+E" if DF_BIND_NOW is set? That would be too permissive, I think -- we need that while doing the relocation, but not after transferring control to a.out. But I don't know if that's possible to encode into the policy. Thanks,
On Tue, Aug 11, 2015 at 3:19 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > On Tue, Aug 11, 2015 at 3:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Tue, Aug 11, 2015 at 2:39 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > >>> It's either >>> - make TEXTREL binary not run under SELinux, or >>> - make them run, but crash mysteriously if they have a called IFUNC >>> resolver in them (or are linked with '-z,now'). >> >> How about >> >> 1. Change ld to disallow TEXTREL with IFUNC and without "-z now'". > > That would still fail under SELinux, wouldn't it? > > Are you proposing also changing SELinux policy to allow "W+E" if > DF_BIND_NOW is set? No. I am proposing that linker issues an error if there is TEXTREL with IFUNC unless "-z now'" is used, assuming that this doesn't require changes to ld.so nor SELinux.
On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > No. I am proposing that linker issues an error if there is TEXTREL > with IFUNC unless "-z now'" is used, assuming that this doesn't > require changes to ld.so nor SELinux. Ah, ok. But that *doesn't* help current crash at all: "-z now" will force IFUNC resolver (if any) to be called, and that call will fail since we are currently removing execute protections. (This is in fact the situation we've discovered the crash in originally.)
On Tue, Aug 11, 2015 at 3:37 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > >> No. I am proposing that linker issues an error if there is TEXTREL >> with IFUNC unless "-z now'" is used, assuming that this doesn't >> require changes to ld.so nor SELinux. > > Ah, ok. But that *doesn't* help current crash at all: "-z now" will > force IFUNC resolver (if any) to be called, and that call will fail > since we are currently removing execute protections. > (This is in fact the situation we've discovered the crash in originally.) Can you try adding -Wl,-z,execstack?
On Tue, Aug 11, 2015 at 3:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Aug 11, 2015 at 3:37 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >> On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> >>> No. I am proposing that linker issues an error if there is TEXTREL >>> with IFUNC unless "-z now'" is used, assuming that this doesn't >>> require changes to ld.so nor SELinux. >> >> Ah, ok. But that *doesn't* help current crash at all: "-z now" will >> force IFUNC resolver (if any) to be called, and that call will fail >> since we are currently removing execute protections. >> (This is in fact the situation we've discovered the crash in originally.) > > Can you try adding -Wl,-z,execstack? Yes, making the stack executable will solve the problem. My test case needed ".note.GNU-stack" specifically for this. Thanks Sri > > -- > H.J.
On Tue, Aug 11, 2015 at 3:57 PM, Sriraman Tallam <tmsriram@google.com> wrote: > On Tue, Aug 11, 2015 at 3:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Tue, Aug 11, 2015 at 3:37 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >>> On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> >>>> No. I am proposing that linker issues an error if there is TEXTREL >>>> with IFUNC unless "-z now'" is used, assuming that this doesn't >>>> require changes to ld.so nor SELinux. >>> >>> Ah, ok. But that *doesn't* help current crash at all: "-z now" will >>> force IFUNC resolver (if any) to be called, and that call will fail >>> since we are currently removing execute protections. >>> (This is in fact the situation we've discovered the crash in originally.) >> >> Can you try adding -Wl,-z,execstack? > > Yes, making the stack executable will solve the problem. My test case > needed ".note.GNU-stack" specifically for this. Given SELinux issue, I don't think we should change ld.so. Instead, we can change ld to issue an error for TEXTREL with IFUNC and suggest -fPIE and -Wl,-z,execstack as workaround.
On Tue, Aug 11, 2015 at 5:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Aug 11, 2015 at 3:57 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> On Tue, Aug 11, 2015 at 3:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Tue, Aug 11, 2015 at 3:37 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >>>> On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> >>>>> No. I am proposing that linker issues an error if there is TEXTREL >>>>> with IFUNC unless "-z now'" is used, assuming that this doesn't >>>>> require changes to ld.so nor SELinux. >>>> >>>> Ah, ok. But that *doesn't* help current crash at all: "-z now" will >>>> force IFUNC resolver (if any) to be called, and that call will fail >>>> since we are currently removing execute protections. >>>> (This is in fact the situation we've discovered the crash in originally.) >>> >>> Can you try adding -Wl,-z,execstack? >> >> Yes, making the stack executable will solve the problem. My test case >> needed ".note.GNU-stack" specifically for this. > > Given SELinux issue, I don't think we should change ld.so. Instead, > we can change ld to issue an error for TEXTREL with IFUNC and > suggest -fPIE and -Wl,-z,execstack as workaround. I am not sure I understand the problem. What is wrong with the patch? Why should IFUNC+TEXTREL be disallowed? Thanks Sri > > > -- > H.J.
On Tue, Aug 11, 2015 at 5:55 PM, Sriraman Tallam <tmsriram@google.com> wrote: > On Tue, Aug 11, 2015 at 5:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Tue, Aug 11, 2015 at 3:57 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> On Tue, Aug 11, 2015 at 3:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> On Tue, Aug 11, 2015 at 3:37 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >>>>> On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>> >>>>>> No. I am proposing that linker issues an error if there is TEXTREL >>>>>> with IFUNC unless "-z now'" is used, assuming that this doesn't >>>>>> require changes to ld.so nor SELinux. >>>>> >>>>> Ah, ok. But that *doesn't* help current crash at all: "-z now" will >>>>> force IFUNC resolver (if any) to be called, and that call will fail >>>>> since we are currently removing execute protections. >>>>> (This is in fact the situation we've discovered the crash in originally.) >>>> >>>> Can you try adding -Wl,-z,execstack? >>> >>> Yes, making the stack executable will solve the problem. My test case >>> needed ".note.GNU-stack" specifically for this. >> >> Given SELinux issue, I don't think we should change ld.so. Instead, >> we can change ld to issue an error for TEXTREL with IFUNC and >> suggest -fPIE and -Wl,-z,execstack as workaround. > > I am not sure I understand the problem. What is wrong with the patch? > Why should IFUNC+TEXTREL be disallowed? Since this will cause any TEXTREL binary to fail under SELinux config that prohibits "W+E" permissions, which is OK without IFUNC.
On 11 Aug 2015 17:02, H.J. Lu wrote: > On Tue, Aug 11, 2015 at 3:57 PM, Sriraman Tallam <tmsriram@google.com> wrote: > > On Tue, Aug 11, 2015 at 3:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > >> On Tue, Aug 11, 2015 at 3:37 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > >>> On Tue, Aug 11, 2015 at 3:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > >>> > >>>> No. I am proposing that linker issues an error if there is TEXTREL > >>>> with IFUNC unless "-z now'" is used, assuming that this doesn't > >>>> require changes to ld.so nor SELinux. > >>> > >>> Ah, ok. But that *doesn't* help current crash at all: "-z now" will > >>> force IFUNC resolver (if any) to be called, and that call will fail > >>> since we are currently removing execute protections. > >>> (This is in fact the situation we've discovered the crash in originally.) > >> > >> Can you try adding -Wl,-z,execstack? > > > > Yes, making the stack executable will solve the problem. My test case > > needed ".note.GNU-stack" specifically for this. > > Given SELinux issue, I don't think we should change ld.so. Instead, > we can change ld to issue an error for TEXTREL with IFUNC and > suggest -fPIE and -Wl,-z,execstack as workaround. i don't see why we should make any change. it isn't ld's problem that the restrictive runtime prevents things. ld already issues a warning when you have textrels in shared segments, so let's leave it at that. ftr, the issue you describe is not specific to selinux as other security systems have been doing this for a long time. e.g. grsec/PaX. -mike
On Tue, Aug 11, 2015 at 10:48 PM, Mike Frysinger <vapier@gentoo.org> wrote: > i don't see why we should make any change. it isn't ld's problem that the > restrictive runtime prevents things. ld already issues a warning when you > have textrels in shared segments, so let's leave it at that. For the test case, ld does not issue any warnings: gcc -fPIE -pie tst-pie-ifunc-txtrel.S readelf -d a.out | grep TEXTREL 0x0000000000000016 (TEXTREL) 0x0 Apparently --warn-shared-textrel is not the default for GNU ld (GNU Binutils for Ubuntu) 2.24. The resulting binary crashes inside ld.so, for no apparent reason: (gdb) r Starting program: /tmp/a.out Program received signal SIGSEGV, Segmentation fault. 0x000055555555478b in selector () (gdb) bt #0 0x000055555555478b in selector () #1 0x00007ffff7de680f in elf_machine_rela (reloc=0x555555554510, reloc=0x555555554510, skip_ifunc=<optimized out>, reloc_addr_arg=0x555555554798 <main+2>, version=<optimized out>, sym=<optimized out>, map=0x7ffff7ffe1c8) at ../sysdeps/x86_64/dl-machine.h:467 #2 elf_dynamic_do_Rela (skip_ifunc=<optimized out>, lazy=<optimized out>, nrelative=<optimized out>, relsize=<optimized out>, reladdr=<optimized out>, map=0x7ffff7ffe1c8) at do-rel.h:149 #3 _dl_relocate_object (scope=<optimized out>, reloc_mode=<optimized out>, consider_profiling=<optimized out>, consider_profiling@entry=0) at dl-reloc.c:264 #4 0x00007ffff7dddafa in dl_main (phdr=<optimized out>, phdr@entry=0x555555554040, phnum=<optimized out>, phnum@entry=9, user_entry=user_entry@entry=0x7fffffffe2b8, auxv=<optimized out>) at rtld.c:2204 #5 0x00007ffff7df1565 in _dl_sysdep_start (start_argptr=start_argptr@entry=0x7fffffffe3a0, dl_main=dl_main@entry=0x7ffff7ddb910 <dl_main>) at ../elf/dl-sysdep.c:249 #6 0x00007ffff7ddecf8 in _dl_start_final (arg=0x7fffffffe3a0) at rtld.c:332 #7 _dl_start (arg=0x7fffffffe3a0) at rtld.c:558 #8 0x00007ffff7ddb2d8 in _start () at rtld.c:875 There is nothing obviously wrong with the executable (aside from the fact that it has TEXTREL). ld.so took away execute permissions from selector(), when arguably it shouldn't have. The patch fixes that, but the fix will cause additional damage (not currently present) to other TEXTREL binaries under SELinux and other security systems. 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). > ftr, the issue you describe is not specific to selinux as other security > systems have been doing this for a long time. e.g. grsec/PaX. Yes, I am aware of these. I am just using SELinux as a synonym for "SELinux and other security systems".
On 11 Aug 2015 23:15, Paul Pluzhnikov wrote: > On Tue, Aug 11, 2015 at 10:48 PM, Mike Frysinger wrote: > > i don't see why we should make any change. it isn't ld's problem that the > > restrictive runtime prevents things. ld already issues a warning when you > > have textrels in shared segments, so let's leave it at that. > > For the test case, ld does not issue any warnings: > > gcc -fPIE -pie tst-pie-ifunc-txtrel.S > readelf -d a.out | grep TEXTREL > 0x0000000000000016 (TEXTREL) 0x0 > > Apparently --warn-shared-textrel is not the default for GNU ld (GNU > Binutils for Ubuntu) 2.24. it is in Gentoo ;). i wouldn't mind that going upstream. > The resulting binary crashes inside ld.so, for no apparent reason: i'm ambivalent on the ld.so change. the quoted context i used before was just for the ld part, not ld.so. -mike
diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c index 61252d7..1c0ed7c 100644 --- a/elf/dl-reloc.c +++ b/elf/dl-reloc.c @@ -201,13 +201,6 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], newp->start = PTR_ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize)) + (caddr_t) l->l_addr; - if (__mprotect (newp->start, newp->len, PROT_READ|PROT_WRITE) < 0) - { - errstring = N_("cannot make segment writable for relocation"); - call_error: - _dl_signal_error (errno, l->l_name, NULL, errstring); - } - #if (PF_R | PF_W | PF_X) == 7 && (PROT_READ | PROT_WRITE | PROT_EXEC) == 7 newp->prot = (PF_TO_PROT >> ((ph->p_flags & (PF_R | PF_W | PF_X)) * 4)) & 0xf; @@ -220,6 +213,13 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], if (ph->p_flags & PF_X) newp->prot |= PROT_EXEC; #endif + if (__mprotect (newp->start, newp->len, newp->prot | PROT_WRITE) < 0) + { + errstring = N_("cannot make segment writable for relocation"); + call_error: + _dl_signal_error (errno, l->l_name, NULL, errstring); + } + newp->next = textrels; textrels = newp; } diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile index ef70a50..c0a9c43 100644 --- a/sysdeps/x86_64/Makefile +++ b/sysdeps/x86_64/Makefile @@ -34,6 +34,10 @@ 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) + tests += tst-audit3 tst-audit4 tst-audit5 tst-audit10 ifeq (yes,$(config-cflags-avx)) tests += tst-audit6 tst-audit7 diff --git a/sysdeps/x86_64/tst-pie-ifunc-txtrel.S b/sysdeps/x86_64/tst-pie-ifunc-txtrel.S index e69de29..77360c4 100644 --- a/sysdeps/x86_64/tst-pie-ifunc-txtrel.S +++ b/sysdeps/x86_64/tst-pie-ifunc-txtrel.S @@ -0,0 +1,32 @@ +/* Verify that a PIE binary with TEXTREL and a IFUNC symbol executes. + + Copyright (C) 2015 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 + <http://www.gnu.org/licenses/>. */ + +foo: + movl $0, %eax + ret +selector: + movabsq $foo, %rax + ret + .type selector, %gnu_indirect_function + .globl main +main: + movabsq $selector, %rax + call *%rax + ret + .section .note.GNU-stack,"",@progbits