Re: [PATCH] mprotect segments with extra PROT_WRITE bit when DT_TEXTREL bit is set

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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