* [PATCH] elf: Refine direct extern access diagnostics to protected symbol @ 2022-06-07 23:53 Fangrui Song 2022-06-08 1:49 ` H.J. Lu 0 siblings, 1 reply; 9+ messages in thread From: Fangrui Song @ 2022-06-07 23:53 UTC (permalink / raw) To: libc-alpha 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); } #endif /* _DL_PROTECTED_H */ -- 2.36.1.255.ge46751e96f-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-07 23:53 [PATCH] elf: Refine direct extern access diagnostics to protected symbol Fangrui Song @ 2022-06-08 1:49 ` H.J. Lu 2022-06-08 3:53 ` Fangrui Song 0 siblings, 1 reply; 9+ messages in thread From: H.J. Lu @ 2022-06-08 1:49 UTC (permalink / raw) To: Fangrui Song; +Cc: GNU C Library 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 > -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 1:49 ` H.J. Lu @ 2022-06-08 3:53 ` Fangrui Song 2022-06-08 18:15 ` H.J. Lu 0 siblings, 1 reply; 9+ messages in thread From: Fangrui Song @ 2022-06-08 3:53 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 3:53 ` Fangrui Song @ 2022-06-08 18:15 ` H.J. Lu 2022-06-08 18:59 ` Fangrui Song 0 siblings, 1 reply; 9+ messages in thread From: H.J. Lu @ 2022-06-08 18:15 UTC (permalink / raw) To: Fangrui Song; +Cc: GNU C Library 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. -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 18:15 ` H.J. Lu @ 2022-06-08 18:59 ` Fangrui Song 2022-06-08 19:10 ` H.J. Lu 0 siblings, 1 reply; 9+ messages in thread From: Fangrui Song @ 2022-06-08 18:59 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 18:59 ` Fangrui Song @ 2022-06-08 19:10 ` H.J. Lu 2022-06-08 19:48 ` Fangrui Song 0 siblings, 1 reply; 9+ messages in thread From: H.J. Lu @ 2022-06-08 19:10 UTC (permalink / raw) To: Fangrui Song; +Cc: GNU C Library On Wed, Jun 8, 2022 at 11:59 AM Fangrui Song <maskray@google.com> wrote: > > On 2022-06-08, H.J. Lu wrote: > >On Tue, Jun 7, 2022 at 8:53 PM Fangrui Song <maskray@google.com> wrote: > >> > >> On 2022-06-07, H.J. Lu wrote: > >> >On Tue, Jun 7, 2022 at 4:53 PM Fangrui Song via Libc-alpha > >> ><libc-alpha@sourceware.org> wrote: > >> >> > >> >> Refine commit 349b0441dab375099b1d7f6909c1742286a67da9: > >> >> > >> >> 1. Copy relocations for extern protected data do not work properly, > >> >> regardless whether GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is used. > >> >> It makes sense to produce a warning unconditionally. When the defining > >> >> shared object has GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS, report > >> >> an error to satisfy the "no copy relocations" enforcement intended by > >> >> this GNU property. > >> >> > >> >> 2. Non-zero value of an undefined function symbol may break pointer > >> >> equality, but may be benign in many cases (many programs don't take the > >> >> address in the shared object then compare it with the address in the > >> >> executable). Report a warning instead. While here, reword the > >> >> diagnostic to be clearer. > >> >> > >> >> 3. Remove the unneeded condition !(undef_map->l_1_needed & > >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS). If the executable has > >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (can only occur in error > >> >> cases), the diagnostic should be emitted as well. > >> >> --- > >> >> sysdeps/generic/dl-protected.h | 46 ++++++++++++++++++---------------- > >> >> 1 file changed, 25 insertions(+), 21 deletions(-) > >> >> > >> >> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h > >> >> index 88cb8ec917..ed40d9fea9 100644 > >> >> --- a/sysdeps/generic/dl-protected.h > >> >> +++ b/sysdeps/generic/dl-protected.h > >> >> @@ -26,29 +26,33 @@ _dl_check_protected_symbol (const char *undef_name, > >> >> const struct link_map *map, > >> >> int type_class) > >> >> { > >> >> - if (undef_map != NULL > >> >> - && undef_map->l_type == lt_executable > >> >> - && !(undef_map->l_1_needed > >> >> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS) > >> >> - && (map->l_1_needed > >> >> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)) > >> >> + if (undef_map == NULL || undef_map->l_type != lt_executable) > >> >> + return; > >> >> + > >> >> + if (type_class & ELF_RTYPE_CLASS_COPY) > >> >> { > >> >> - if ((type_class & ELF_RTYPE_CLASS_COPY)) > >> >> - /* Disallow copy relocations in executable against protected > >> >> - data symbols in a shared object which needs indirect external > >> >> - access. */ > >> >> - _dl_signal_error (0, map->l_name, undef_name, > >> >> - N_("copy relocation against non-copyable protected symbol")); > >> >> - else if (ref->st_value != 0 > >> >> - && ref->st_shndx == SHN_UNDEF > >> >> - && (type_class & ELF_RTYPE_CLASS_PLT)) > >> >> - /* Disallow non-zero symbol values of undefined symbols in > >> >> - executable, which are used as the function pointer, against > >> >> - protected function symbols in a shared object with indirect > >> >> - external access. */ > >> >> - _dl_signal_error (0, map->l_name, undef_name, > >> >> - N_("non-canonical reference to canonical protected function")); > >> >> + /* Disallow copy relocations in executable against protected > >> >> + data symbols in a shared object which needs indirect external > >> >> + access. */ > >> >> + _dl_error_printf ("warning: copy relocation against non-copyable " > >> >> + "protected symbol `%s' in `%s'\n", > >> >> + undef_name, map->l_name); > >> >> + > >> >> + if (map->l_1_needed & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS) > >> >> + _dl_signal_error ( > >> >> + 0, map->l_name, undef_name, > >> >> + N_ ("error due to GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS")); > >> >> } > >> >> + else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0 > >> >> + && ref->st_shndx == SHN_UNDEF) > >> >> + /* Disallow non-zero symbol values of undefined symbols in > >> >> + executable, which are used as the function pointer, against > >> >> + protected function symbols in a shared object with indirect > >> >> + external access. */ > >> >> + _dl_error_printf ( > >> >> + "warning: direct reference to " > >> >> + "protected function `%s' in `%s' may break pointer equality\n", > >> >> + undef_name, map->l_name); > >> > > >> >Should there be an error for GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS? > >> > >> I lean toward a warning, as bullet point 2 in my commit message > >> explains. > > > >It is due to R_386_PC32. Can we make the error optional and enable it > >for x86-64? > > Do you mean that the branch should call call _dl_signal_error in the > presence of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS? Yes. > else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0 > && ref->st_shndx == SHN_UNDEF) > > >> In addition, this check requires that the executable with a non-zero > >> st_value has at least one JUMP_SLOT relocation. > >> > >> In the following setup the executable does not have JUMP_SLOT, so > >> there is no diagnostic, with or without the patch. > > > >We can pass symbol definition and check STT_FUNC. > > My point is that the check only kicks in when there is a dynamic > relocation using ELF_RTYPE_CLASS_PLT (typically JUMP_SLOT). If the > executable just takes the address but doesn't call the function, the > branch will not be executed at all. Yes, linker has resolved the relocation to the PLT entry. There may be no dynamic relocation. Replacing R_386_PC32 with R_386_PLT32 won't work correctly since R_386_PLT32 assumes EBX setup and R_386_PC32 doesn't. Linker needs to handle it properly for protected symbols. > That said, I am flexible and can add the wrong if you feel strong about > it. To be clear, do you indicate that the error should require > !defined(__i386__) ? You can add a macro to disable the error by default and x86-64 can opt it in. > >> // a.c -fno-pic -no-pie > >> #include <stdio.h> > >> int foo(void); > >> int main(void) { printf("%p\n", foo); > >> > >> // b.c -fpic -shared > >> int foo(void) { return 3; } > >> asm(".protected foo"); > >> > >> >> } > >> >> > >> >> #endif /* _DL_PROTECTED_H */ > >> >> -- > >> >> 2.36.1.255.ge46751e96f-goog > >> >> > >> > > >> > > >> >-- > >> >H.J. > > > > > > > >-- > >H.J. -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 19:10 ` H.J. Lu @ 2022-06-08 19:48 ` Fangrui Song 2022-06-08 20:05 ` H.J. Lu 0 siblings, 1 reply; 9+ messages in thread From: Fangrui Song @ 2022-06-08 19:48 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 19:48 ` Fangrui Song @ 2022-06-08 20:05 ` H.J. Lu 2022-06-08 20:21 ` Fangrui Song 0 siblings, 1 reply; 9+ messages in thread From: H.J. Lu @ 2022-06-08 20:05 UTC (permalink / raw) To: Fangrui Song; +Cc: GNU C Library 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. -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol 2022-06-08 20:05 ` H.J. Lu @ 2022-06-08 20:21 ` Fangrui Song 0 siblings, 0 replies; 9+ messages in thread From: Fangrui Song @ 2022-06-08 20:21 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-06-08 20:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-07 23:53 [PATCH] elf: Refine direct extern access diagnostics to protected symbol Fangrui Song 2022-06-08 1:49 ` H.J. Lu 2022-06-08 3:53 ` Fangrui Song 2022-06-08 18:15 ` H.J. Lu 2022-06-08 18:59 ` Fangrui Song 2022-06-08 19:10 ` H.J. Lu 2022-06-08 19:48 ` Fangrui Song 2022-06-08 20:05 ` H.J. Lu 2022-06-08 20:21 ` Fangrui Song
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).