[PR,libc/18801] PIE binary with STT_GNU_IFUNC symbol and TEXTREL segfaults on x86_64
Commit Message
Details here:
https://sourceware.org/bugzilla/show_bug.cgi?id=18801
Thanks to Paul Pluzhnikov for identifying the problem and suggesting the fix.
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.
Patch attached. Please review.
Thanks
Sri
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.
Comments
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
@@ -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;
}
@@ -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
@@ -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