mprotect segments with extra PROT_WRITE bit when DT_TEXTREL bit is set

  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.

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.

2018-08-22  Fangrui Song  <maskray@google.com>

	* elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
	PROT_WRITE bit.
  

On 2018-08-22, Fangrui Song wrote:
>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.
>
>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.
>
>2018-08-22  Fangrui Song  <maskray@google.com>
>
>	* elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
>	PROT_WRITE bit.
>
>diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
>index 053916eeae..bd7d1ae84f 100644
>--- a/elf/dl-reloc.c
>+++ b/elf/dl-reloc.c
>@@ -199,14 +199,6 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>			- ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
>	    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 +212,14 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>	      newp->prot |= PROT_EXEC;
>#endif
>	    newp->next = textrels;
>+
>+	    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);
>+	      }
>+
>	    textrels = newp;
>	  }
>    }

A simple reproduce.

// 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]    88059 segmentation fault  ./a
% clang -fuse-ld=lld -Wl,-z,notext a.c -Wl,--dynamic-linker=~/Dev/Util/glibc/build/elf/ld.so -o a; ./a
# good

gcc is similar. But upstream gcc does not support -fuse-ld=lld. It is available
on Debian and its derivatives via a patch.

This works fine on FreeBSD because its rtld does the same as the patch does.
OpenBSD rtld is probably also faulty.
  

On 2018-08-22, Fangrui Song wrote:
>On 2018-08-22, Fangrui Song wrote:
>>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.
>>
>>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.
>>
>>2018-08-22  Fangrui Song  <maskray@google.com>
>>
>>	* elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
>>	PROT_WRITE bit.
>>
>>diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
>>index 053916eeae..bd7d1ae84f 100644
>>--- a/elf/dl-reloc.c
>>+++ b/elf/dl-reloc.c
>>@@ -199,14 +199,6 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>			- ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
>>	    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 +212,14 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>	      newp->prot |= PROT_EXEC;
>>#endif
>>	    newp->next = textrels;
>>+
>>+	    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);
>>+	      }
>>+
>>	    textrels = newp;
>>	  }
>>   }
>
>A simple reproduce.
>
>// 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]    88059 segmentation fault  ./a
>% clang -fuse-ld=lld -Wl,-z,notext a.c -Wl,--dynamic-linker=~/Dev/Util/glibc/build/elf/ld.so -o a; ./a
># good
>
>gcc is similar. But upstream gcc does not support -fuse-ld=lld. It is available
>on Debian and its derivatives via a patch.
>
>This works fine on FreeBSD because its rtld does the same as the patch does.
>OpenBSD rtld is probably also faulty.

To give a bit more context, ld.bfd and gold emit DT_TEXTREL on demand
(and default to -z notext): they create DT_TEXTREL only if text relocation is
required. While lld chooses a more explicit approach:

* By default text relocation is disallowed (-z text)
* If -z notext is specified, text relocation will be allowed. DT_TEXTREL
  will be created no matter if text relocation is really used.

ld.bfd probably defends this by rejecting linking when both IFUNC and DT_TEXTREL are required.
  

On 08/23/2018 07:52 AM, Fangrui Song wrote:
> 
>      * elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
>      PROT_WRITE bit.

This needs a bug in Bugzilla (is it bug 20480?), reference in the 
ChangeLog entry, and, ideally, a regression test.

I can see that the current code has a problem, but I assume we do things 
this way because we want to create an RWX mapping, even temporarily. 
System security policy may prevent the creation of such mappings.  We 
would have have to flip between RW and RWX protection flags around IFUNC 
handlers to fix that.

Thanks,
Florian
  

On 23/08/2018 03:58, Fangrui Song wrote:
> 
> On 2018-08-22, Fangrui Song wrote:
>> On 2018-08-22, Fangrui Song wrote:
>>> 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.
>>>
>>> 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.
>>>
>>> 2018-08-22  Fangrui Song  <maskray@google.com>
>>>
>>>     * elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
>>>     PROT_WRITE bit.
>>>
>>> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
>>> index 053916eeae..bd7d1ae84f 100644
>>> --- a/elf/dl-reloc.c
>>> +++ b/elf/dl-reloc.c
>>> @@ -199,14 +199,6 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>>             - ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
>>>         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 +212,14 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>>           newp->prot |= PROT_EXEC;
>>> #endif
>>>         newp->next = textrels;
>>> +
>>> +        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);
>>> +          }
>>> +
>>>         textrels = newp;
>>>       }
>>>   }
>>
>> A simple reproduce.
>>
>> // 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]    88059 segmentation fault  ./a
>> % clang -fuse-ld=lld -Wl,-z,notext a.c -Wl,--dynamic-linker=~/Dev/Util/glibc/build/elf/ld.so -o a; ./a
>> # good
>>
>> gcc is similar. But upstream gcc does not support -fuse-ld=lld. It is available
>> on Debian and its derivatives via a patch.
>>
>> This works fine on FreeBSD because its rtld does the same as the patch does.
>> OpenBSD rtld is probably also faulty.
> 
> To give a bit more context, ld.bfd and gold emit DT_TEXTREL on demand
> (and default to -z notext): they create DT_TEXTREL only if text relocation is
> required. While lld chooses a more explicit approach:
> 
> * By default text relocation is disallowed (-z text)
> * If -z notext is specified, text relocation will be allowed. DT_TEXTREL
>  will be created no matter if text relocation is really used.
> 
> ld.bfd probably defends this by rejecting linking when both IFUNC and DT_TEXTREL are required.
> 
> 

This has been reported before as BZ#20480 [1] with a fix similar to
the one you are proposing. The fix seems correct in imho, if we still
aim to support textrel we should honour 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).

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=20480
[2] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/bz20480
  

On 29/08/2018 10:55, Florian Weimer wrote:
> On 08/23/2018 07:52 AM, Fangrui Song wrote:
>>
>>      * elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
>>      PROT_WRITE bit.
> 
> This needs a bug in Bugzilla (is it bug 20480?), reference in the ChangeLog entry, and, ideally, a regression test.

BZ#20480 is the same issue with a different origin.

> 
> I can see that the current code has a problem, but I assume we do things this way because we want to create an RWX mapping, even temporarily. System security policy may prevent the creation of such mappings.  We would have have to flip between RW and RWX protection flags around IFUNC handlers to fix that.
> 
> Thanks,
> Florian

All the projects I am aware tries to avoid text relocation and consider it a
security issue, however if user still want to mix ifunc plus textrel it would
require a executable segment (which is default for some glibc targets along
with some symbols calls).

One option is we could just bail out as some ld.bfd targets does at linking
time, but it seems lld behaviour might not be unique. I don't have a strong
preference here, if the binary fails due system security policy (a hardened
kernel which prevents it), I think it is an issue which should be fixed by 
*not* using textrel.
  

On 2018-08-29, Florian Weimer wrote:
>On 08/23/2018 07:52 AM, Fangrui Song wrote:
>>
>>     * elf/dl-reloc.c (_dl_relocate_object): __mprotect with extra
>>     PROT_WRITE bit.
>
>This needs a bug in Bugzilla (is it bug 20480?), reference in the 
>ChangeLog entry, and, ideally, a regression test.
>
>I can see that the current code has a problem, but I assume we do 
>things this way because we want to create an RWX mapping, even 
>temporarily. System security policy may prevent the creation of such 
>mappings.  We would have have to flip between RW and RWX protection 
>flags around IFUNC handlers to fix that.
>
>Thanks,
>Florian

Thank you for referring to 20480! I have replied there.