From: "H.J. Lu" <hjl.tools@gmail.com>
To: "Carlos O'Donell" <carlos@redhat.com>
Cc: GNU C Library <libc-alpha@sourceware.org>
Subject: Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
Date: Mon, 4 Jan 2021 11:59:49 -0800 [thread overview]
Message-ID: <CAMe9rOq+_H5=RewFmFYT+xV856zC6rVVZA2iYaa8y0SxSknsWA@mail.gmail.com> (raw)
In-Reply-To: <4058143e-9b84-f8f5-1361-420a5b3aa808@redhat.com>
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.
> OK for master.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > From 85fd4f35471038f734532ee902fd0b99a9aa16ba Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Mon, 28 Dec 2020 05:28:49 -0800
> > Subject: [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ
> > #20019]
> >
> > Calling an IFUNC function defined in unrelocated executable also leads to
> > segfault. Issue a fatal error message when calling IFUNC function defined
> > in the unrelocated executable from a shared library.
> > ---
> > sysdeps/i386/dl-machine.h | 16 +++++++++++-----
> > sysdeps/x86_64/dl-machine.h | 16 +++++++++++-----
> > 2 files changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> > index 50960605e6..23e9cc3bfb 100644
> > --- a/sysdeps/i386/dl-machine.h
> > +++ b/sysdeps/i386/dl-machine.h
> > @@ -337,16 +337,22 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> > {
> > # ifndef RTLD_BOOTSTRAP
> > 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_fatal_printf ("\
> > +%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
> > +and creates an unsatisfiable circular dependency.\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 f582be5320..103eee6c3f 100644
> > --- a/sysdeps/x86_64/dl-machine.h
> > +++ b/sysdeps/x86_64/dl-machine.h
> > @@ -314,16 +314,22 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> > {
> > # ifndef RTLD_BOOTSTRAP
> > 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_fatal_printf ("\
> > +%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
> > +and creates an unsatisfiable circular dependency.\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 = ((ElfW(Addr) (*) (void)) value) ();
> > --
> > 2.29.2
> >
>
> --
> Cheers,
> Carlos.
>
--
H.J.
next prev parent reply other threads:[~2021-01-04 20:00 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-28 14:11 H.J. Lu
2021-01-04 18:47 ` Carlos O'Donell
2021-01-04 19:34 ` V2 " H.J. Lu
2021-01-04 19:50 ` Carlos O'Donell
2021-01-04 19:59 ` H.J. Lu [this message]
2021-01-04 22:57 ` H.J. Lu
2021-01-05 13:03 ` Carlos O'Donell
2021-01-05 15:14 ` H.J. Lu
2021-01-04 20:44 ` H.J. Lu
2021-01-04 21:20 ` Carlos O'Donell
2021-01-04 22:38 ` [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency " H.J. Lu
2021-01-13 19:43 ` Adhemerval Zanella
2021-01-13 19:48 ` H.J. Lu
2021-01-14 13:10 ` Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMe9rOq+_H5=RewFmFYT+xV856zC6rVVZA2iYaa8y0SxSknsWA@mail.gmail.com' \
--to=hjl.tools@gmail.com \
--cc=carlos@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).