diff mbox series

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

Message ID 20220608193701.3211265-1-maskray@google.com
State Committed
Commit 7374c02b683b7110b853a32496a619410364d70b
Headers
Series [v2] elf: Refine direct extern access diagnostics to protected symbol |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Fangrui Song June 8, 2022, 7:37 p.m. UTC 
  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(-)

Comments

H.J. Lu June 10, 2022, 8:43 p.m. UTC | #1 
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?
Fangrui Song June 10, 2022, 9:23 p.m. UTC | #2 
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.
H.J. Lu June 10, 2022, 10:03 p.m. UTC | #3 
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?
Fangrui Song June 10, 2022, 10:47 p.m. UTC | #4 
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.
H.J. Lu June 12, 2022, 9:24 p.m. UTC | #5 
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 "

> +       "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
>
Fangrui Song June 12, 2022, 9:35 p.m. UTC | #6 
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.
Fangrui Song June 14, 2022, 7:50 p.m. UTC | #7 
On 2022-06-10, Fangrui Song wrote:
>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.

Hi H.J.

Does the patch look good?
H.J. Lu June 14, 2022, 7:53 p.m. UTC | #8 
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
>

LGTM.

Thanks.
diff mbox series

Patch

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