Bug 18801 - PIE binary with STT_GNU_IFUNC symbol and TEXTREL segfaults on x86_64
Summary: PIE binary with STT_GNU_IFUNC symbol and TEXTREL segfaults on x86_64
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: 2.26
Assignee: Paul Pluzhnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-10 20:11 IST by Sriraman Tallam
Modified: 2015-08-13 14:41 IST (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Preserve the original segment's execute permissions when protecting the page for writing it. (581 bytes, patch)
2015-08-10 20:11 IST, Sriraman Tallam
Details | Diff
Preserve the original segment's execute permissions when protecting the page for writing it (1.42 KB, patch)
2015-08-11 20:11 IST, Sriraman Tallam
Details | Diff
Preserve the original segment's execute permissions when protecting the page for writing it (1.40 KB, text/plain)
2015-08-11 20:43 IST, Sriraman Tallam
Details
Preserve the original segment's execute permissions when protecting the page for writing it (1.40 KB, patch)
2015-08-11 21:03 IST, Sriraman Tallam
Details | Diff
A patch to issue an error for read-only segment with dynamic IFUNC relocations (489 bytes, patch)
2015-08-12 21:10 IST, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sriraman Tallam 2015-08-10 20:11:54 IST
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.
Comment 1 H.J. Lu 2015-08-10 20:21:45 IST
(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.
Comment 2 Sriraman Tallam 2015-08-11 20:11:15 IST
Created attachment 8503 [details]
Preserve the original segment's execute permissions when protecting the page for writing it

Added test case.
Comment 3 Paul Pluzhnikov 2015-08-11 20:20:54 IST
Google ref: b/20921387
Comment 4 H.J. Lu 2015-08-11 20:28:15 IST
(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
Comment 5 Sriraman Tallam 2015-08-11 20:43:35 IST
Created attachment 8504 [details]
Preserve the original segment's execute permissions when protecting the page for writing it

Patch Updated.
Comment 6 H.J. Lu 2015-08-11 20:56:32 IST
(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
Comment 7 Sriraman Tallam 2015-08-11 21:03:42 IST
Created attachment 8505 [details]
Preserve the original segment's execute permissions when protecting the page for writing it

Patch updated.
Comment 8 H.J. Lu 2015-08-11 21:05:52 IST
(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.
Comment 9 H.J. Lu 2015-08-12 11:38:57 IST
Linker should refuse to generate binary with STT_GNU_IFUNC symbol and TEXTREL.
Comment 10 Sriraman Tallam 2015-08-12 15:42:46 IST
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.
Comment 11 H.J. Lu 2015-08-12 21:10:31 IST
Created attachment 8513 [details]
A patch to issue an error for read-only segment with dynamic IFUNC relocations
Comment 12 H.J. Lu 2015-08-12 21:14:49 IST
(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.
Comment 13 cvs-commit@gcc.gnu.org 2015-08-13 11:40:58 IST
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.
Comment 14 H.J. Lu 2015-08-13 11:44:15 IST
-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.
Comment 15 Paul Pluzhnikov 2015-08-13 14:22:25 IST
> FIXED

What about Gold? Or any other linker that supports IFUNC and TEXTREL, but doesn't warn.
Comment 16 Sriraman Tallam 2015-08-13 14:41:27 IST
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.