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

[Fangrui Song]

On Sun, Jun 12, 2022 at 2:24 PM H.J. Lu <hjl.tools@gmail.com> 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 "
>
> I think the format convention is
>
>      _dl_error_printf ("warning: direct reference to "

The patch matches both Emacs C-M-\ (GNU indentation style) and clang-format.
They place the string literal on the continuation line...

> > +       "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,
>
> _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
> >
>
>
> --
> H.J.