Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]

[Carlos O'Donell <carlos@redhat.com>]

On 1/4/21 3:44 PM, H.J. Lu wrote:
> On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 1/4/21 2:34 PM, H.J. Lu wrote:
>>> On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>>>
>>>> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
>>>>> Calling an IFUNC function defined in unrelocated executable may also
>>>>> lead to segfault.  Issue an error message when calling IFUNC function
>>>>> defined in the unrelocated executable from a shared library.
>>>>
>>>> The logic here makes sense, but we need a stronger error message.
>>>>
>>>> Please review my understanding and suggested error message.
>>>>
>>>> Looking forward to v2.
>>>>
>>>>> ---
>>>>>  sysdeps/i386/dl-machine.h   | 15 ++++++++++-----
>>>>>  sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
>>>>>  2 files changed, 20 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
>>>>> index fea9e579ec..dedda484ba 100644
>>>>> --- a/sysdeps/i386/dl-machine.h
>>>>> +++ b/sysdeps/i386/dl-machine.h
>>>>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
>>>>>       {
>>>>>  # ifndef RTLD_BOOTSTRAP
>>>>
>>>> OK. Logic is in the correct place in dl-machine.h for i386.
>>>>
>>>>>         if (sym_map != map
>>>>> -           && sym_map->l_type != lt_executable
>>>>>             && !sym_map->l_relocated)
>>>>>           {
>>>>>             const char *strtab
>>>>>               = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>>>> -           _dl_error_printf ("\
>>>>> +           if (sym_map->l_type == lt_executable)
>>>>> +             _dl_error_printf ("\
>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>>>> +                               RTLD_PROGNAME, strtab + refsym->st_name,
>>>>> +                               map->l_name);
>>>>> +           else
>>>>> +             _dl_error_printf ("\
>>>>>  %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>>>> -                             RTLD_PROGNAME, map->l_name,
>>>>> -                             sym_map->l_name,
>>>>> -                             strtab + refsym->st_name);
>>>>> +                               RTLD_PROGNAME, map->l_name,
>>>>> +                               sym_map->l_name,
>>>>> +                               strtab + refsym->st_name);
>>>>>           }
>>>>>  # endif
>>>>>         value = ((Elf32_Addr (*) (void)) value) ();
>>>>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
>>>>> index bb93c7c6ab..fc847f4bc2 100644
>>>>> --- a/sysdeps/x86_64/dl-machine.h
>>>>> +++ b/sysdeps/x86_64/dl-machine.h
>>>>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>>>>>       {
>>>>>  # ifndef RTLD_BOOTSTRAP
>>>>
>>>> OK. Logic is in the correct place in dl-machine.h for x86_64.
>>>>
>>>>>         if (sym_map != map
>>>>> -           && sym_map->l_type != lt_executable
>>>>>             && !sym_map->l_relocated)
>>>>>           {
>>>>>             const char *strtab
>>>>>               = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>>>> -           _dl_error_printf ("\
>>>>> +           if (sym_map->l_type == lt_executable)
>>>>> +             _dl_error_printf ("\
>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>>>
>>>> The message should explain the error
>>>> e.g. "Such and such *must not* reference such and such."
>>>>
>>>> Or the message should explain how to fix the error (as the other does)
>>>> e.g. "Such and such must be relinked with such and such."
>>>>
>>>> We have made this a hard error. An executable with immediate binding
>>>> may not define an IFUNC resolver and implementation that is used from
>>>> a shared library since it creates an ordering issue with the dependent
>>>> libraries that use the resolution of the symbol i.e. you must initialize
>>>> the executable but to do that you must initialize the libraries, but to
>>>> do that you must initialize the executable etc. etc.
>>>>
>>>> In which case the error message could be:
>>>>
>>>> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
>>>>  and creates an unsatisfiable circular dependency."
>>>
>>> Fixed.
>>>
>>>> Note: Use '' quotes not `' since the GNU Coding standards have changed.
>>>> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
>>>>
>>>>> +                               RTLD_PROGNAME, strtab + refsym->st_name,
>>>>> +                               map->l_name);
>>>>> +           else
>>>>> +             _dl_error_printf ("\
>>>>>  %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>>>> -                             RTLD_PROGNAME, map->l_name,
>>>>> -                             sym_map->l_name,
>>>>> -                             strtab + refsym->st_name);
>>>>> +                               RTLD_PROGNAME, map->l_name,
>>>>> +                               sym_map->l_name,
>>>>> +                               strtab + refsym->st_name);
>>>>>           }
>>>>>  # endif
>>>>>         value = ((ElfW(Addr) (*) (void)) value) ();
>>>>>
>>>>
>>>>
>>>
>>> Here is the updated patch.  Changes from V1:
>>>
>>> 1. Update the error message based on feedback from Carlos.
>>> 2. Make the error fatal instead of segfault later.
>>>
>>> OK for master?
>>
>> Could binutils have given the user a better warnings?
>>
>> OK for master.
>>
> 
> Now I got
> 
> [hjl@gnu-cfl-2 build-x86_64-linux]$ ./elf/ifuncmain6pie --direct
> ./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in
> '/export/build/gnu/tools-build/glibc/build-x86_64-linux/elf/ifuncmod6.so'
> is defined in the executable and creates an unsatisfiable circular
> dependency.
> [hjl@gnu-cfl-2 build-x86_64-linux]$
> 
> The message is correct.  Should we update the testcase to avoid it?

Yes, but it is still possible to support this with lazy binding?

Should ifuncmain6pie be explicitly compiled with -Wl,-z,lazy to
bypass selection from the toolchain?

-- 
Cheers,
Carlos.