* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-08 19:37 [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol Fangrui Song
@ 2022-06-10 20:43 ` H.J. Lu
2022-06-10 21:23 ` Fangrui Song
2022-06-12 21:24 ` H.J. Lu
2022-06-14 19:53 ` H.J. Lu
2 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2022-06-10 20:43 UTC (permalink / raw)
To: Fangrui Song; +Cc: GNU C Library
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?
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-10 20:43 ` H.J. Lu
@ 2022-06-10 21:23 ` Fangrui Song
2022-06-10 22:03 ` H.J. Lu
0 siblings, 1 reply; 9+ messages in thread
From: Fangrui Song @ 2022-06-10 21:23 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-10 21:23 ` Fangrui Song
@ 2022-06-10 22:03 ` H.J. Lu
2022-06-10 22:47 ` Fangrui Song
0 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2022-06-10 22:03 UTC (permalink / raw)
To: Fangrui Song; +Cc: GNU C Library
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?
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-10 22:03 ` H.J. Lu
@ 2022-06-10 22:47 ` Fangrui Song
2022-06-14 19:50 ` Fangrui Song
0 siblings, 1 reply; 9+ messages in thread
From: Fangrui Song @ 2022-06-10 22:47 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-10 22:47 ` Fangrui Song
@ 2022-06-14 19:50 ` Fangrui Song
0 siblings, 0 replies; 9+ messages in thread
From: Fangrui Song @ 2022-06-14 19:50 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-08 19:37 [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol Fangrui Song
2022-06-10 20:43 ` H.J. Lu
@ 2022-06-12 21:24 ` H.J. Lu
2022-06-12 21:35 ` Fangrui Song
2022-06-14 19:53 ` H.J. Lu
2 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2022-06-12 21:24 UTC (permalink / raw)
To: Fangrui Song; +Cc: GNU C Library
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
>
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-12 21:24 ` H.J. Lu
@ 2022-06-12 21:35 ` Fangrui Song
0 siblings, 0 replies; 9+ messages in thread
From: Fangrui Song @ 2022-06-12 21:35 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol
2022-06-08 19:37 [PATCH v2] elf: Refine direct extern access diagnostics to protected symbol Fangrui Song
2022-06-10 20:43 ` H.J. Lu
2022-06-12 21:24 ` H.J. Lu
@ 2022-06-14 19:53 ` H.J. Lu
2 siblings, 0 replies; 9+ messages in thread
From: H.J. Lu @ 2022-06-14 19:53 UTC (permalink / raw)
To: Fangrui Song; +Cc: GNU C Library
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.
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread