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

["H.J. Lu" <hjl.tools@gmail.com>]

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.

> 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.

> 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.

> >> // 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.