From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x235.google.com (mail-oi1-x235.google.com [IPv6:2607:f8b0:4864:20::235]) by sourceware.org (Postfix) with ESMTPS id 8D5883894C23 for ; Mon, 4 Jan 2021 20:00:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8D5883894C23 Received: by mail-oi1-x235.google.com with SMTP id x13so33488768oic.5 for ; Mon, 04 Jan 2021 12:00:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KMbAGELyigN2bCtm92brO7hczrxWBdhAfRaNkqy2qUA=; b=eH/f8VTEeByUoHLj3fEjCQhvU+CIdRJzaFeAetX7z2r4l9+vjys9/6GM/B4X3EPdF3 8nU+OiQqEyuJ21sxgNIvkCIJ8sDNGFKW1a+xREZwgx+bxcIsR5to3/Gwos8GJHzT91ML z6wUtS5A8wJp0KvEVP1JY6VSqKLLrw9n74CXOgBHwWM/t5b9FmEKhOOpUkD29Btg8GhM QpWncfp436fRCnvfiRwLMy3c21Wu5q+0dLvwlrLc7/Rl5UVB9qYMbV8L8iWVFiR3B31X aMBaBtfdD1rONm7FkAv9m10M180UPKOl+tKrXiC/rk14XoFady5Wc58Bb74sNDgiuLpC kodA== X-Gm-Message-State: AOAM530aNpQa+gCFFXmh7Px8mCMK3VOt7r+a3Akls3NqwpbNg8LLY+o0 IQCbyRSGm/sDQoqARZvreDP40vGMBlCNFKTNh/7Xb7Ag X-Google-Smtp-Source: ABdhPJxZ4R6501msa3n0l2bKyEfhFvUaZ0Kut45Z+ft8zMbgHHjeaXg4zMwrWCidSQZRQIvJz8ZHsGvTZfzFe7wh0Qo= X-Received: by 2002:aca:f456:: with SMTP id s83mr392348oih.58.1609790426017; Mon, 04 Jan 2021 12:00:26 -0800 (PST) MIME-Version: 1.0 References: <20201228141102.2562718-1-hjl.tools@gmail.com> <0d676184-2aee-6580-7281-dbbafbe671c4@redhat.com> <4058143e-9b84-f8f5-1361-420a5b3aa808@redhat.com> In-Reply-To: <4058143e-9b84-f8f5-1361-420a5b3aa808@redhat.com> From: "H.J. Lu" Date: Mon, 4 Jan 2021 11:59:49 -0800 Message-ID: Subject: Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019] To: "Carlos O'Donell" Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3037.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jan 2021 20:00:28 -0000 On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell wrote: > > On 1/4/21 2:34 PM, H.J. Lu wrote: > > On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell 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 > > > From 85fd4f35471038f734532ee902fd0b99a9aa16ba Mon Sep 17 00:00:00 2001 > > From: "H.J. Lu" > > 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.