Message ID | 20180823054723.wbiatxqzp775xfho@google.com |
---|---|
State | New |
Headers | show |
Series | mprotect segments with extra PROT_WRITE bit when DT_TEXTREL bit is set | expand |
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.
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; } }