diff mbox series

elf: Refine direct extern access diagnostics to protected symbol

Message ID 20220607235308.1190097-1-maskray@google.com
State Superseded
Headers
Series 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 7, 2022, 11:53 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).  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(-)

Comments

H.J. Lu June 8, 2022, 1:49 a.m. UTC | #1 
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?

>  }
>
>  #endif /* _DL_PROTECTED_H */
> --
> 2.36.1.255.ge46751e96f-goog
>
Fangrui Song June 8, 2022, 3:53 a.m. UTC | #2 
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.

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.

// 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. Lu June 8, 2022, 6:15 p.m. UTC | #3 
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?

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

> // 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.
Fangrui Song June 8, 2022, 6:59 p.m. UTC | #4 
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?

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.

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__) ?

>> // 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. Lu June 8, 2022, 7:10 p.m. UTC | #5 
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.
Fangrui Song June 8, 2022, 7:48 p.m. UTC | #6 
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.
H.J. Lu June 8, 2022, 8:05 p.m. UTC | #7 
On Wed, Jun 8, 2022 at 12:48 PM Fangrui Song <maskray@google.com> wrote:
>
> 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

Just to be clear.  Some i386 shared libraries are compiled without
-fPIC on purpose
to improve performance.  When ld sees R_386_PC32 of an undefined
symbol in a shared
library, it creates a dynamic R_386_PC32 relocation in the .text
section.  Replace
R_386_PC32 with R_386_PLT32 will break this.

> * 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.
Fangrui Song June 8, 2022, 8:21 p.m. UTC | #8 
On 2022-06-08, H.J. Lu wrote:
>On Wed, Jun 8, 2022 at 12:48 PM Fangrui Song <maskray@google.com> wrote:
>>
>> 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
>
>Just to be clear.  Some i386 shared libraries are compiled without
>-fPIC on purpose
>to improve performance.  When ld sees R_386_PC32 of an undefined
>symbol in a shared
>library, it creates a dynamic R_386_PC32 relocation in the .text
>section.  Replace
>R_386_PC32 with R_386_PLT32 will break this.

Thanks. I did not know this usage (which should be *unsupported*, but might have been abused in the past)

int bar() { return 1; }
int foo() { return bar(); }

gcc -fno-pic -m32 -fuse-ld=bfd -shared a.c -z notext

0000114b  00000502 R_386_PC32             0000113d   bar

>> * 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.
>
>
>
>-- 
>H.J.
diff mbox series

Patch

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);
 }
 
 #endif /* _DL_PROTECTED_H */