Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol

[Fangrui Song <maskray@google.com>]

On 2022-06-10, H.J. Lu wrote:
>On Fri, Jun 10, 2022 at 2:23 PM Fangrui Song <maskray@google.com> wrote:
>>
>>
>> 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.
>
>With GCC and bfd linker?

Yes.
I have just tested both (a) ld is bfd and (b) /usr/local/bin/ld is lld.
No regression.

Note: clang cannot build glibc yet.