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] elf: Refine direct extern access diagnostics to protected symbol
Date: Wed, 8 Jun 2022 12:48:08 -0700 [thread overview]
Message-ID: <20220608194808.kucpfiir4prwuym5@google.com> (raw)
In-Reply-To: <CAMe9rOpDZfL2nuc_RTj+itBNUU+EsH1CUTpbumq5dJH5jV+XNA@mail.gmail.com>
On 2022-06-08, H.J. Lu wrote:
>On Wed, Jun 8, 2022 at 11:59 AM Fangrui Song <maskray@google.com> wrote:
>>
>> On 2022-06-08, H.J. Lu wrote:
>> >On Tue, Jun 7, 2022 at 8:53 PM Fangrui Song <maskray@google.com> wrote:
>> >>
>> >> On 2022-06-07, H.J. Lu wrote:
>> >> >On Tue, Jun 7, 2022 at 4:53 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). Report a warning instead. While here, 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.
>> >> >> ---
>> >> >> sysdeps/generic/dl-protected.h | 46 ++++++++++++++++++----------------
>> >> >> 1 file changed, 25 insertions(+), 21 deletions(-)
>> >> >>
>> >> >> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h
>> >> >> index 88cb8ec917..ed40d9fea9 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 (undef_map == NULL || undef_map->l_type != lt_executable)
>> >> >> + return;
>> >> >> +
>> >> >> + if (type_class & ELF_RTYPE_CLASS_COPY)
>> >> >> {
>> >> >> - 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"));
>> >> >> + /* 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);
>> >> >> +
>> >> >> + 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"));
>> >> >> }
>> >> >> + 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);
>> >> >
>> >> >Should there be an error for GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS?
>> >>
>> >> I lean toward a warning, as bullet point 2 in my commit message
>> >> explains.
>> >
>> >It is due to R_386_PC32. Can we make the error optional and enable it
>> >for x86-64?
>>
>> Do you mean that the branch should call call _dl_signal_error in the
>> presence of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS?
>
>Yes.
Sent v2
https://sourceware.org/pipermail/libc-alpha/2022-June/139587.html
>> else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0
>> && ref->st_shndx == SHN_UNDEF)
>>
>> >> In addition, this check requires that the executable with a non-zero
>> >> st_value has at least one JUMP_SLOT relocation.
>> >>
>> >> In the following setup the executable does not have JUMP_SLOT, so
>> >> there is no diagnostic, with or without the patch.
>> >
>> >We can pass symbol definition and check STT_FUNC.
>>
>> My point is that the check only kicks in when there is a dynamic
>> relocation using ELF_RTYPE_CLASS_PLT (typically JUMP_SLOT). If the
>> executable just takes the address but doesn't call the function, the
>> branch will not be executed at all.
>
>Yes, linker has resolved the relocation to the PLT entry. There may be
>no dynamic relocation. Replacing R_386_PC32 with R_386_PLT32
>won't work correctly since R_386_PLT32 assumes EBX setup and
>R_386_PC32 doesn't. Linker needs to handle it properly for protected symbols.
I see that you categorize the two relocations (which can be used as
branches) this way:
* R_386_PC32: no-pic PLT or address-taking. Needs to check SHF_EXECINSTR and disassembly the insn
* R_386_PLT32: pic PLT
and therefore think that `jmp foo` cannot switch to R_386_PC32.
However, I categorize them this way:
* R_386_PC32: possibly address-taking
* R_386_PLT32: branch (either no-pic or pic PLT). -no-pie uses no-pic PLT while -pie/-shared uses pic PLT
My way follows the intention of R_X86_64_PC32/R_X86_64_PLT32.
>> That said, I am flexible and can add the wrong if you feel strong about
>> it. To be clear, do you indicate that the error should require
>> !defined(__i386__) ?
>
>You can add a macro to disable the error by default and x86-64 can
>opt it in.
My PATCH v2 doesn't do anything with __i386__. The R_386_PC32 concern
is a corner case (a shared object has upgraded from STV_DEFAULT to
STV_PROTECTED, lld is used, there is a -fno-pic executable using
neither -fno-direct-access-external-data -mno-direct-extern-access).
Seems good to make it separate even if we decide to do something.
>> >> // a.c -fno-pic -no-pie
>> >> #include <stdio.h>
>> >> int foo(void);
>> >> int main(void) { printf("%p\n", foo);
>> >>
>> >> // b.c -fpic -shared
>> >> int foo(void) { return 3; }
>> >> asm(".protected foo");
>> >>
>> >> >> }
>> >> >>
>> >> >> #endif /* _DL_PROTECTED_H */
>> >> >> --
>> >> >> 2.36.1.255.ge46751e96f-goog
>> >> >>
>> >> >
>> >> >
>> >> >--
>> >> >H.J.
>> >
>> >
>> >
>> >--
>> >H.J.
>
>
>
>--
>H.J.
next prev parent reply other threads:[~2022-06-08 19:48 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-07 23:53 Fangrui Song
2022-06-08 1:49 ` H.J. Lu
2022-06-08 3:53 ` Fangrui Song
2022-06-08 18:15 ` H.J. Lu
2022-06-08 18:59 ` Fangrui Song
2022-06-08 19:10 ` H.J. Lu
2022-06-08 19:48 ` Fangrui Song [this message]
2022-06-08 20:05 ` H.J. Lu
2022-06-08 20:21 ` Fangrui Song
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=20220608194808.kucpfiir4prwuym5@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).