From: Fangrui Song <maskray@google.com>
To: "H.J. Lu" <hjl.tools@gmail.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 14:23:44 -0700 [thread overview]
Message-ID: <20220610212344.kjx7d3jdygswiol5@google.com> (raw)
In-Reply-To: <CAMe9rOqHmcMgPsg7F2tMiyW22KMzOBHCvKC5OQsPJVkJgbtuyw@mail.gmail.com>
On 2022-06-10, H.J. Lu wrote:
>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?
`make -j 50 check` has no regression on x86-64.
next prev parent reply other threads:[~2022-06-10 21:23 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
2022-06-10 21:23 ` Fangrui Song [this message]
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=20220610212344.kjx7d3jdygswiol5@google.com \
--to=maskray@google.com \
--cc=hjl.tools@gmail.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).