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

[Fangrui Song <maskray@google.com>]

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.