From: "H.J. Lu" <hjl.tools@gmail.com>
To: Fangrui Song <maskray@google.com>
Cc: GNU C Library <libc-alpha@sourceware.org>
Subject: Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
Date: Fri, 10 Jun 2022 13:43:41 -0700 [thread overview]
Message-ID: <CAMe9rOqHmcMgPsg7F2tMiyW22KMzOBHCvKC5OQsPJVkJgbtuyw@mail.gmail.com> (raw)
In-Reply-To: <20220608193701.3211265-1-maskray@google.com>
On Wed, Jun 8, 2022 at 12:37 PM Fangrui Song via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Refine commit 349b0441dab375099b1d7f6909c1742286a67da9:
>
> 1. Copy relocations for extern protected data do not work properly,
> regardless whether GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is used.
> It makes sense to produce a warning unconditionally. When the defining
> shared object has GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS, report
> an error to satisfy the "no copy relocations" enforcement intended by
> this GNU property.
>
> 2. Non-zero value of an undefined function symbol may break pointer
> equality, but may be benign in many cases (many programs don't take the
> address in the shared object then compare it with the address in the
> executable). Reword the diagnostic to be clearer.
>
> 3. Remove the unneeded condition !(undef_map->l_1_needed &
> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS). If the executable has
> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (can only occur in error
> cases), the diagnostic should be emitted as well.
>
> --
> Changes from v1:
> * Keep 2 as an error in the presence of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> as requested by H.J. Lu.
> ---
> sysdeps/generic/dl-protected.h | 50 ++++++++++++++++++----------------
> 1 file changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h
> index 88cb8ec917..38386b5200 100644
> --- a/sysdeps/generic/dl-protected.h
> +++ b/sysdeps/generic/dl-protected.h
> @@ -26,29 +26,33 @@ _dl_check_protected_symbol (const char *undef_name,
> const struct link_map *map,
> int type_class)
> {
> - if (undef_map != NULL
> - && undef_map->l_type == lt_executable
> - && !(undef_map->l_1_needed
> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)
> - && (map->l_1_needed
> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS))
> - {
> - if ((type_class & ELF_RTYPE_CLASS_COPY))
> - /* Disallow copy relocations in executable against protected
> - data symbols in a shared object which needs indirect external
> - access. */
> - _dl_signal_error (0, map->l_name, undef_name,
> - N_("copy relocation against non-copyable protected symbol"));
> - else if (ref->st_value != 0
> - && ref->st_shndx == SHN_UNDEF
> - && (type_class & ELF_RTYPE_CLASS_PLT))
> - /* Disallow non-zero symbol values of undefined symbols in
> - executable, which are used as the function pointer, against
> - protected function symbols in a shared object with indirect
> - external access. */
> - _dl_signal_error (0, map->l_name, undef_name,
> - N_("non-canonical reference to canonical protected function"));
> - }
> + if (undef_map == NULL || undef_map->l_type != lt_executable)
> + return;
> +
> + if (type_class & ELF_RTYPE_CLASS_COPY)
> + /* Disallow copy relocations in executable against protected
> + data symbols in a shared object which needs indirect external
> + access. */
> + _dl_error_printf ("warning: copy relocation against non-copyable "
> + "protected symbol `%s' in `%s'\n",
> + undef_name, map->l_name);
> + else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0
> + && ref->st_shndx == SHN_UNDEF)
> + /* Disallow non-zero symbol values of undefined symbols in
> + executable, which are used as the function pointer, against
> + protected function symbols in a shared object with indirect
> + external access. */
> + _dl_error_printf (
> + "warning: direct reference to "
> + "protected function `%s' in `%s' may break pointer equality\n",
> + undef_name, map->l_name);
> + else
> + return;
> +
> + if (map->l_1_needed & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)
> + _dl_signal_error (
> + 0, map->l_name, undef_name,
> + N_ ("error due to GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS"));
> }
>
> #endif /* _DL_PROTECTED_H */
> --
> 2.36.1.255.ge46751e96f-goog
>
Does this patch cause any extra glibc test failures?
--
H.J.
next prev parent reply other threads:[~2022-06-10 20:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-08 19:37 Fangrui Song
2022-06-10 20:43 ` H.J. Lu [this message]
2022-06-10 21:23 ` Fangrui Song
2022-06-10 22:03 ` H.J. Lu
2022-06-10 22:47 ` Fangrui Song
2022-06-14 19:50 ` Fangrui Song
2022-06-12 21:24 ` H.J. Lu
2022-06-12 21:35 ` Fangrui Song
2022-06-14 19:53 ` H.J. Lu
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=CAMe9rOqHmcMgPsg7F2tMiyW22KMzOBHCvKC5OQsPJVkJgbtuyw@mail.gmail.com \
--to=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
--cc=maskray@google.com \
/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).