Message ID | 20220607235308.1190097-1-maskray@google.com |
---|---|
State | New |
Headers | show |
Series | elf: Refine direct extern access diagnostics to protected symbol | expand |
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 >
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.
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.
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.
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.
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.
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.
On 2022-06-08, H.J. Lu wrote: >On Wed, Jun 8, 2022 at 12:48 PM Fangrui Song <maskray@google.com> wrote: >> >> On 2022-06-08, H.J. Lu wrote: >> >On Wed, Jun 8, 2022 at 11:59 AM Fangrui Song <maskray@google.com> wrote: >> >> >> >> On 2022-06-08, H.J. Lu wrote: >> >> >On Tue, Jun 7, 2022 at 8:53 PM Fangrui Song <maskray@google.com> wrote: >> >> >> >> >> >> On 2022-06-07, H.J. Lu wrote: >> >> >> >On Tue, Jun 7, 2022 at 4:53 PM Fangrui Song via Libc-alpha >> >> >> ><libc-alpha@sourceware.org> wrote: >> >> >> >> >> >> >> >> Refine commit 349b0441dab375099b1d7f6909c1742286a67da9: >> >> >> >> >> >> >> >> 1. Copy relocations for extern protected data do not work properly, >> >> >> >> regardless whether GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is used. >> >> >> >> It makes sense to produce a warning unconditionally. When the defining >> >> >> >> shared object has GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS, report >> >> >> >> an error to satisfy the "no copy relocations" enforcement intended by >> >> >> >> this GNU property. >> >> >> >> >> >> >> >> 2. Non-zero value of an undefined function symbol may break pointer >> >> >> >> equality, but may be benign in many cases (many programs don't take the >> >> >> >> address in the shared object then compare it with the address in the >> >> >> >> executable). Report a warning instead. While here, reword the >> >> >> >> diagnostic to be clearer. >> >> >> >> >> >> >> >> 3. Remove the unneeded condition !(undef_map->l_1_needed & >> >> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS). If the executable has >> >> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (can only occur in error >> >> >> >> cases), the diagnostic should be emitted as well. >> >> >> >> --- >> >> >> >> sysdeps/generic/dl-protected.h | 46 ++++++++++++++++++---------------- >> >> >> >> 1 file changed, 25 insertions(+), 21 deletions(-) >> >> >> >> >> >> >> >> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h >> >> >> >> index 88cb8ec917..ed40d9fea9 100644 >> >> >> >> --- a/sysdeps/generic/dl-protected.h >> >> >> >> +++ b/sysdeps/generic/dl-protected.h >> >> >> >> @@ -26,29 +26,33 @@ _dl_check_protected_symbol (const char *undef_name, >> >> >> >> const struct link_map *map, >> >> >> >> int type_class) >> >> >> >> { >> >> >> >> - if (undef_map != NULL >> >> >> >> - && undef_map->l_type == lt_executable >> >> >> >> - && !(undef_map->l_1_needed >> >> >> >> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS) >> >> >> >> - && (map->l_1_needed >> >> >> >> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)) >> >> >> >> + if (undef_map == NULL || undef_map->l_type != lt_executable) >> >> >> >> + return; >> >> >> >> + >> >> >> >> + if (type_class & ELF_RTYPE_CLASS_COPY) >> >> >> >> { >> >> >> >> - if ((type_class & ELF_RTYPE_CLASS_COPY)) >> >> >> >> - /* Disallow copy relocations in executable against protected >> >> >> >> - data symbols in a shared object which needs indirect external >> >> >> >> - access. */ >> >> >> >> - _dl_signal_error (0, map->l_name, undef_name, >> >> >> >> - N_("copy relocation against non-copyable protected symbol")); >> >> >> >> - else if (ref->st_value != 0 >> >> >> >> - && ref->st_shndx == SHN_UNDEF >> >> >> >> - && (type_class & ELF_RTYPE_CLASS_PLT)) >> >> >> >> - /* Disallow non-zero symbol values of undefined symbols in >> >> >> >> - executable, which are used as the function pointer, against >> >> >> >> - protected function symbols in a shared object with indirect >> >> >> >> - external access. */ >> >> >> >> - _dl_signal_error (0, map->l_name, undef_name, >> >> >> >> - N_("non-canonical reference to canonical protected function")); >> >> >> >> + /* Disallow copy relocations in executable against protected >> >> >> >> + data symbols in a shared object which needs indirect external >> >> >> >> + access. */ >> >> >> >> + _dl_error_printf ("warning: copy relocation against non-copyable " >> >> >> >> + "protected symbol `%s' in `%s'\n", >> >> >> >> + undef_name, map->l_name); >> >> >> >> + >> >> >> >> + if (map->l_1_needed & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS) >> >> >> >> + _dl_signal_error ( >> >> >> >> + 0, map->l_name, undef_name, >> >> >> >> + N_ ("error due to GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS")); >> >> >> >> } >> >> >> >> + else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0 >> >> >> >> + && ref->st_shndx == SHN_UNDEF) >> >> >> >> + /* Disallow non-zero symbol values of undefined symbols in >> >> >> >> + executable, which are used as the function pointer, against >> >> >> >> + protected function symbols in a shared object with indirect >> >> >> >> + external access. */ >> >> >> >> + _dl_error_printf ( >> >> >> >> + "warning: direct reference to " >> >> >> >> + "protected function `%s' in `%s' may break pointer equality\n", >> >> >> >> + undef_name, map->l_name); >> >> >> > >> >> >> >Should there be an error for GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS? >> >> >> >> >> >> I lean toward a warning, as bullet point 2 in my commit message >> >> >> explains. >> >> > >> >> >It is due to R_386_PC32. Can we make the error optional and enable it >> >> >for x86-64? >> >> >> >> Do you mean that the branch should call call _dl_signal_error in the >> >> presence of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS? >> > >> >Yes. >> >> Sent v2 >> https://sourceware.org/pipermail/libc-alpha/2022-June/139587.html >> >> >> else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0 >> >> && ref->st_shndx == SHN_UNDEF) >> >> >> >> >> In addition, this check requires that the executable with a non-zero >> >> >> st_value has at least one JUMP_SLOT relocation. >> >> >> >> >> >> In the following setup the executable does not have JUMP_SLOT, so >> >> >> there is no diagnostic, with or without the patch. >> >> > >> >> >We can pass symbol definition and check STT_FUNC. >> >> >> >> My point is that the check only kicks in when there is a dynamic >> >> relocation using ELF_RTYPE_CLASS_PLT (typically JUMP_SLOT). If the >> >> executable just takes the address but doesn't call the function, the >> >> branch will not be executed at all. >> > >> >Yes, linker has resolved the relocation to the PLT entry. There may be >> >no dynamic relocation. Replacing R_386_PC32 with R_386_PLT32 >> >won't work correctly since R_386_PLT32 assumes EBX setup and >> >R_386_PC32 doesn't. Linker needs to handle it properly for protected symbols. >> >> I see that you categorize the two relocations (which can be used as >> branches) this way: >> >> * R_386_PC32: no-pic PLT or address-taking. Needs to check SHF_EXECINSTR and disassembly the insn > >Just to be clear. Some i386 shared libraries are compiled without >-fPIC on purpose >to improve performance. When ld sees R_386_PC32 of an undefined >symbol in a shared >library, it creates a dynamic R_386_PC32 relocation in the .text >section. Replace >R_386_PC32 with R_386_PLT32 will break this. Thanks. I did not know this usage (which should be *unsupported*, but might have been abused in the past) int bar() { return 1; } int foo() { return bar(); } gcc -fno-pic -m32 -fuse-ld=bfd -shared a.c -z notext 0000114b 00000502 R_386_PC32 0000113d bar >> * R_386_PLT32: pic PLT >> >> and therefore think that `jmp foo` cannot switch to R_386_PC32. >> >> However, I categorize them this way: >> >> * R_386_PC32: possibly address-taking >> * R_386_PLT32: branch (either no-pic or pic PLT). -no-pie uses no-pic PLT while -pie/-shared uses pic PLT >> >> My way follows the intention of R_X86_64_PC32/R_X86_64_PLT32. >> >> >> That said, I am flexible and can add the wrong if you feel strong about >> >> it. To be clear, do you indicate that the error should require >> >> !defined(__i386__) ? >> > >> >You can add a macro to disable the error by default and x86-64 can >> >opt it in. >> >> My PATCH v2 doesn't do anything with __i386__. The R_386_PC32 concern >> is a corner case (a shared object has upgraded from STV_DEFAULT to >> STV_PROTECTED, lld is used, there is a -fno-pic executable using >> neither -fno-direct-access-external-data -mno-direct-extern-access). >> Seems good to make it separate even if we decide to do something. >> >> >> >> // a.c -fno-pic -no-pie >> >> >> #include <stdio.h> >> >> >> int foo(void); >> >> >> int main(void) { printf("%p\n", foo); >> >> >> >> >> >> // b.c -fpic -shared >> >> >> int foo(void) { return 3; } >> >> >> asm(".protected foo"); >> >> >> >> >> >> >> } >> >> >> >> >> >> >> >> #endif /* _DL_PROTECTED_H */ >> >> >> >> -- >> >> >> >> 2.36.1.255.ge46751e96f-goog >> >> >> >> >> >> >> > >> >> >> > >> >> >> >-- >> >> >> >H.J. >> >> > >> >> > >> >> > >> >> >-- >> >> >H.J. >> > >> > >> > >> >-- >> >H.J. > > > >-- >H.J.
diff --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 */