Re: [Patch v2] Prevent a second round of irel application under static-pie

[Siva Chandra <sivachandra at google dot com>]

On Thu, Mar 21, 2019 at 9:01 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Mar 22, 2019, 11:51 AM Siva Chandra <sivachandra@google.com> wrote:
>>
>> On Thu, Mar 21, 2019 at 3:40 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >
>> > On Fri, Mar 22, 2019, 6:00 AM Siva Chandra <sivachandra@google.com> wrote:
>> >>
>> >> On Thu, Mar 21, 2019 at 2:32 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >> >
>> >> > On 3/22/19, Siva Chandra <sivachandra@google.com> wrote:
>> >> > > I am sending this patch as v2 of the discussion I started on the v1
>> >> > > patch: https://sourceware.org/ml/libc-alpha/2019-03/msg00421.html
>> >> > >
>> >> > > Firstly, I apologize for the fact that I miss-diagnosed the problem,
>> >> > > but I think HJ Lu's response helped me do better with v2.
>> >> > >
>> >> > > So, for v1, I wrongly concluded that elf_irel should adjust for the
>> >> > > load address. As HJ  Lu pointed out, that path is supposed to be taken
>> >> > > only for non-pie static. The reason is, for static-pie,
>> >> > > _dl_relocate_static_pie already applies the irel relocations.
>> >> > >
>> >> > > If I use ld as my linker, I do see what HJ Lu is pointing out and also
>> >> > > that _dl_relocate_static_pie is indeed applying the irel relocations.
>> >> > > But, notice that ARCH_SETUP_IREL is called in LIBC_START_MAIN after
>> >> > > _dl_relocate_static_pie. So, it seems like irel relocations are being
>> >> > > applied for a second time. And indeed, they would be applied for a
>> >> > > second time if ld generates correct values for __rela_iplt_end and
>> >> > > __rela_iplt_start. However, look at an example of what ld generates
>> >> > > with -static-pie:
>> >> > >
>> >> > >    898: 00000000000080f0     0 NOTYPE  LOCAL  DEFAULT    7 __rela_iplt_end
>> >> > >    899: 00000000000a8f20     0 NOTYPE  LOCAL  DEFAULT   20 __fini_array_end
>> >> > >    900: 000000000007e0f0   542 FUNC    LOCAL  DEFAULT   11 __letf2
>> >> > >    901: 00000000000080f0     0 NOTYPE  LOCAL  DEFAULT    7
>> >> > > __rela_iplt_start
>> >> > >
>> >> > > Notice that __rela_iplt_end and __rela_iplt_start have the same value.
>> >> > > Since ARCH_SETUP_IREL applies irel relocations iterating from
>> >> > > __rela_iplt_start to __rela_iplt_end, for an ld generated static-pie
>> >> > > binary, ARCH_SETUP_IREL ends up being a NOP. With any other linker,
>> >> > > which generates correct __rela_iplt_end and __rela_iplt_start values,
>> >> > > ARCH_SETUP_IREL tries to apply irel relocations for a second time,
>> >> > > even though _dl_relocate_static_pie has already applied them.
>> >> > >
>> >> > > So, as opposed to my v1, this change is essentially protecting against
>> >> > > reapplying irel relocations when the linker generates the correct
>> >> > > values of __rela_iplt_end and __rela_iplt_start.
>> >> > >
>> >> > > I have seen HJ Lu's patch attempting the same but with a different
>> >> > > approach: https://sourceware.org/ml/libc-alpha/2019-03/msg00445.html.
>> >> > > However, I have a feeling it breaks for non-PIE static if glibc is
>> >> > > configured with --enable-static-pie. So, I am putting my patch out
>> >> > > there as well, just in case.
>> >> > >
>> >> > > ChangeLog:
>> >> > >
>> >> > >         * csu/libc-start.c (LIBC_START_MAIN): Call
>> >> > > ARCH_SETUP_IREL/ARCH_APPLY_IREL
>> >> > >         only if _dl_relocate_static_pie did not apply any relocations.
>> >> > >         * csu/static-reloc.c (_dl_relocate_static_pie): Return 0.
>> >> > >         * elf/dl-reloc-static-pie.c (_dl_relocate_static_pie): Return 1.
>> >> > >         * sysdeps/generic/ldsodefs.h (_dl_relocate_static_pie): Add return
>> >> > > type
>> >> > >         int to the function. Define the macro to 0.
>> >> > >
>> >> >
>> >> > Can you fix lld instead?
>> >>
>> >> It wouldn't really be "fixing" lld, but introducing a bug in it to
>> >> incorrectly generate equal __rela_iplt_end and __rela_iplt_start for
>> >> static-pie.
>> >
>> >
>> > These symbols aren't equal only when there are
>> > no other dynamic relocations.  It is an lld bug.
>>
>> Even if we were to say that the symbols aren't equal only when there
>> are no dynamic relocations, a static-pie binary does have dynamic
>> relocations of type R_X86_64_RELATIVE isn't it?
>
>
> Dynamic relocations are defined in DT_XXX.
> Since there are no DT_XXX in non-PIE static,
> I added these symbols for IFUNC relocations.
> In this case, ld sets standard.

Thanks, this helps. This is knowledge I did not have before. But,
ins't this causing a brittle coupling of two moving parts?

Also, isn't my v2 patch eliminating this brittle coupling?

Thanks,
Siva Chandra