Fix for possible ifunc bug

[theraven]

Summary

At $WORK, we've been looking at some memory safety mitigations and we wanted to stop having to build a load of different allocator configurations and switch between them at load time using the ifunc mechanism. Unfortunately, this didn't work because other globals in the library were not resolved during ifunc resolution. I'm not 100% sure that this is intended to work, but there's a comment in the rtld code indicating that we resolve non-ifunc PLT relocations first to make it work, so I presume that this is a bug.

I've put together a quick-and-dirty fix that I'm now using. It's probably not the correct fix, but I wanted to upload it for review to see if:

• This is something that we'd like to work.
• This is approximately the correct fix.

Rtld was deferring handling of ifuncs until after it had processed all of the other relocations in a global. This is allows an ifunc resolver to depend on other global symbols in the same library.

Unfortunately, ifunc resolvers can be invoked in one library as a result of processing relocations in another. If library A depends on library B, and library B provides a symbol X that is resolved via an ifunc, then the ifunc resolver for X in B was being called after all of the relocations in A had been processed, but before the relocations in B had been processed. Any references to symbols in B by the ifunc resolver would then cause a crash.

[kib]

I think you somehow failed to provide the explanation of your case. Esp. the last paragraph of the description is confusing, please recheck the order of dependencies and order of resolution and correct it so I can understand. Better yet, provide the minimal example of the self-contained code which demonstrates the issue.

libexec/rtld-elf/rtld.c

2892

Extra blank line.

2897

Why did you lower-cased whole comment ?

[theraven]

In D17529#374132, @kib wrote:

I think you somehow failed to provide the explanation of your case. Esp. the last paragraph of the description is confusing, please recheck the order of dependencies and order of resolution and correct it so I can understand. Better yet, provide the minimal example of the self-contained code which demonstrates the issue.

Sorry for the delay in getting back to this. I hoped that D18400 would fix it, but now that I've upgraded to 12, I'm still seeing the issue. We have a library that exposes a function with an ifunc resolve. The ifunc resolver calls another function in the same library, via the PLT. When a second library links against our library and includes a non-PLT relocation for our ifunc symbol, our ifunc resolver invokes a function via a PLT stub, but the PLT stub is not yet initialised and so this dies.

The problem is in libexec/rtld-elf/amd64/reloc.c where, in reloc_non_plt, it calls rtld_resolve_ifunc without checking that defobj is relocated. Adding this before the call is almost certainly the wrong solution, but does address the problem:

if (!defobj->relocated)
    relocate_object(__DECONST(Obj_Entry *, defobj), false, obj_rtld, flags & ~SYMLOOK_IFUNC, lockstate);

I think the correct fix most likely involved doing something a little bit closer to this, and deferring ifunc resolution until after the library implementing the ifunc has had its non-ifunc relocations applied.

[kib]

In D17529#403212, @theraven wrote:

In D17529#374132, @kib wrote:

I think you somehow failed to provide the explanation of your case. Esp. the last paragraph of the description is confusing, please recheck the order of dependencies and order of resolution and correct it so I can understand. Better yet, provide the minimal example of the self-contained code which demonstrates the issue.

Sorry for the delay in getting back to this. I hoped that D18400 would fix it, but now that I've upgraded to 12, I'm still seeing the issue. We have a library that exposes a function with an ifunc resolve. The ifunc resolver calls another function in the same library, via the PLT. When a second library links against our library and includes a non-PLT relocation for our ifunc symbol, our ifunc resolver invokes a function via a PLT stub, but the PLT stub is not yet initialised and so this dies.

The problem is in libexec/rtld-elf/amd64/reloc.c where, in reloc_non_plt, it calls rtld_resolve_ifunc without checking that defobj is relocated. Adding this before the call is almost certainly the wrong solution, but does address the problem:

if (!defobj->relocated)
    relocate_object(__DECONST(Obj_Entry *, defobj), false, obj_rtld, flags & ~SYMLOOK_IFUNC, lockstate);

I think the correct fix most likely involved doing something a little bit closer to this, and deferring ifunc resolution until after the library implementing the ifunc has had its non-ifunc relocations applied.

This is exactly how the current rtld ifunc resolution works:

• first all objects gets their non-plt non-ifunc relocations resolved.
• second all objects gets their ifuncs resolved, including plt resolution if LD_BIND_NOW.
• third pass where init functions are called.

The second pass (for ifuncs) is performed in the reverse-init order, same as the third pass.

In fact I though about more significant rework were second and third passes are merged, i.e. for each object in reverse-init list, first ifuncs are resolved, then inits are called before moving to the next one. In other words, this would allow ifunc resolvers to use all exported symbols from needed libraries. But I decided not to do it for now, since ifunc resolvers should be lightweight and nothing more complicated than bit test for CPU feature presence is not really intended.

[theraven]

In D17529#403239, @kib wrote:

In D17529#403212, @theraven wrote:

In D17529#374132, @kib wrote:

I think you somehow failed to provide the explanation of your case. Esp. the last paragraph of the description is confusing, please recheck the order of dependencies and order of resolution and correct it so I can understand. Better yet, provide the minimal example of the self-contained code which demonstrates the issue.

Sorry for the delay in getting back to this. I hoped that D18400 would fix it, but now that I've upgraded to 12, I'm still seeing the issue. We have a library that exposes a function with an ifunc resolve. The ifunc resolver calls another function in the same library, via the PLT. When a second library links against our library and includes a non-PLT relocation for our ifunc symbol, our ifunc resolver invokes a function via a PLT stub, but the PLT stub is not yet initialised and so this dies.

The problem is in libexec/rtld-elf/amd64/reloc.c where, in reloc_non_plt, it calls rtld_resolve_ifunc without checking that defobj is relocated. Adding this before the call is almost certainly the wrong solution, but does address the problem:

if (!defobj->relocated)
    relocate_object(__DECONST(Obj_Entry *, defobj), false, obj_rtld, flags & ~SYMLOOK_IFUNC, lockstate);

I think the correct fix most likely involved doing something a little bit closer to this, and deferring ifunc resolution until after the library implementing the ifunc has had its non-ifunc relocations applied.

This is exactly how the current rtld ifunc resolution works:

• first all objects gets their non-plt non-ifunc relocations resolved.
• second all objects gets their ifuncs resolved, including plt resolution if LD_BIND_NOW.
• third pass where init functions are called.

The second pass (for ifuncs) is performed in the reverse-init order, same as the third pass.

I think the issue is actually very simple. There is some leftover code that should have been deleted:

Objects get their relocations that are resolved by an ifunc *in another library* resolved via a call to reloc_non_plt near the end of relocate_object (here). The libraries exporting the symbols and containing the ifunc resolver may, at this point, not have had any relocations processed. Calling the ifunc resolver then crashes.

There's then a second phase later when ifuncs are resolved correctly, only this is not reached because rtld has already crashed by then. With this patch, everything works for me:

diff
--- a/libexec/rtld-elf/rtld.c
+++ b/libexec/rtld-elf/rtld.c
@@ -2871,16 +2871,6 @@ relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
            lockstate) == -1)
                return (-1);
 
-       /*
-        * Process the non-PLT IFUNC relocations.  The relocations are
-        * processed in two phases, because IFUNC resolvers may
-        * reference other symbols, which must be readily processed
-        * before resolvers are called.
-        */
-       if (obj->non_plt_gnu_ifunc &&
-           reloc_non_plt(obj, rtldobj, flags | SYMLOOK_IFUNC, lockstate))
-               return (-1);
-
        if (!obj->mainprog && obj_enforce_relro(obj) == -1)
                return (-1);
 

[kib]

In D17529#403257, @theraven wrote:

I think the issue is actually very simple. There is some leftover code that should have been deleted:

Objects get their relocations that are resolved by an ifunc *in another library* resolved via a call to reloc_non_plt near the end of relocate_object (here). The libraries exporting the symbols and containing the ifunc resolver may, at this point, not have had any relocations processed. Calling the ifunc resolver then crashes.

There's then a second phase later when ifuncs are resolved correctly, only this is not reached because rtld has already crashed by then. With this patch, everything works for me:

diff
--- a/libexec/rtld-elf/rtld.c
+++ b/libexec/rtld-elf/rtld.c
@@ -2871,16 +2871,6 @@ relocate_object(Obj_Entry *obj, bool bind_now, Obj_Entry *rtldobj,
            lockstate) == -1)
                return (-1);
 
-       /*
-        * Process the non-PLT IFUNC relocations.  The relocations are
-        * processed in two phases, because IFUNC resolvers may
-        * reference other symbols, which must be readily processed
-        * before resolvers are called.
-        */
-       if (obj->non_plt_gnu_ifunc &&
-           reloc_non_plt(obj, rtldobj, flags | SYMLOOK_IFUNC, lockstate))
-               return (-1);
-
        if (!obj->mainprog && obj_enforce_relro(obj) == -1)
                return (-1);
 

Yes, I agree from reading of the code. I suspect that I left the chunk because I wanted to keep dlopen() handling as is, but later added the initlist_objects_ifunc() call in dlopen_object().

If you can test that the latest amd64 head boots multiuser with your change applied, please commit and MFC to 12+11.

[theraven]

In D17529#403296, @kib wrote:

Yes, I agree from reading of the code. I suspect that I left the chunk because I wanted to keep dlopen() handling as is, but later added the initlist_objects_ifunc() call in dlopen_object().

If you can test that the latest amd64 head boots multiuser with your change applied, please commit and MFC to 12+11.

I don't have a -HEAD setup at the moment, but I can confirm that, with this patch to rtld, I can:

• Build world, kernel, packages
• Install the resulting package set (with the dynamically linked pkg tool)
• Boot the resulting system to multiuser
• Run www/chromium and a few less demanding test applications.
• Start GNUstep apps (which all dlopen the display back end).

I have not seen any failures. This is on a 12-stable system with some changes to libc that make more aggressive use of ifunc than upstream.

[kib]

In D17529#403812, @theraven wrote:

In D17529#403296, @kib wrote:

Yes, I agree from reading of the code. I suspect that I left the chunk because I wanted to keep dlopen() handling as is, but later added the initlist_objects_ifunc() call in dlopen_object().

If you can test that the latest amd64 head boots multiuser with your change applied, please commit and MFC to 12+11.

I don't have a -HEAD setup at the moment, but I can confirm that, with this patch to rtld, I can:

• Build world, kernel, packages
• Install the resulting package set (with the dynamically linked pkg tool)
• Boot the resulting system to multiuser
• Run www/chromium and a few less demanding test applications.
• Start GNUstep apps (which all dlopen the display back end).

I have not seen any failures. This is on a 12-stable system with some changes to libc that make more aggressive use of ifunc than upstream.

I am not sure if some mail was lost, but I never saw your reply.

I am going to commit this shortly, with 'Submitted by: theraven'.

[theraven]

In D17529#405556, @kib wrote:

I am going to commit this shortly, with 'Submitted by: theraven'.

Thank you.