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

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, Jan 5, 2021 at 5:03 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 1/4/21 5:57 PM, H.J. Lu wrote:
> > On Mon, Jan 4, 2021 at 11:59 AM H.J. Lu <hjl.tools@gmail.com> 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?
> >>
> >> I will take a look.
> >>
> >
> > The problem is
> >
> > 0000000000003fe8  0000000300000006 R_X86_64_GLOB_DAT
> > 0000000000000000 foo + 0
> > 0000000000004018  0000000300000001 R_X86_64_64
> > 0000000000000000 foo + 0
> >
> > in ifuncmod6.so.  When linker creates ifuncmod6.so, linker doesn't
> > know that foo will
> > be an IFUNC symbol defined in executable at run-time.  When linker
> > creates ifuncmain6pie,
> > linker doesn't check dynamic relocations in ifuncmod6.so and
> > ifuncmod6.so used in link-time
> > can be different from run-time
> The executable has a DT_NEEDED on ifuncmod*.so.
>
> The static linker must load both the executable and DSO to finish linking.
>
> All the information you need is in theory present.
>
> An when you run with -Wl,-Map you can see this:
> ~~~
> Local IFUNC function `foo' in ./ifuncmain.o
> ~~~
> So the static linker sees the definition and identifies it as an IFUNC.
>
> Then:
> ~~~
> LOAD ./ifuncmain.o
> LOAD ./ifuncmod.so

Static linker doesn't use dynamic relocations in ifuncmod.so.
It only uses the dynamic symbol table.  Some shared objects
used for linking don't even have dynamic relocations.

> ~~~
> So we know ifuncmod.so is dependent and we lay it out for linkage.
>
> It is at this point that you would have to attempt a quick check of the
> dependent DSOs to generate a link warning that given the present set of
> DSOs that this will not work.
>
> Obviously it *could* work at runtime where the DSO is different, where
> an LD_PRELOAD loads a DSO with an interposing definition ahead of the
> executable IFUNC etc. etc. You are subject to the entire host of dynamic
> resolution rules.
>
> However, the static linker could have given a warnings given the existing
> set of objects used in the static link to assemble the executable.
>
> This kind of warning is right on the line between the static and dynamic
> linkers because it is something you can detect at static link time but
> can't formally prove until dynamic link time.
>
> > So there is not much linker can do.
>
> There is.
>
> It is similar to generating warnings from gnu attribute tags.
>
> You issue a warning a static link time, but it won't really fail until
> you attempt to run it e.g. mismatched floating point ABI.
>
> If you wanted it could be expressed as a gnu attribute tag:
> - Non-DSO object defines IFUNC
> - DSO uses IFUNC
> - During merge of the sections you look for the problematic scenario.
>
> --
> Cheers,
> Carlos.
>


-- 
H.J.