| Message ID | 1561672142-5907-2-git-send-email-dmladjenovic@wavecomp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Mips support for PT_GNU_STACK | expand |
On Thu, 27 Jun 2019, Dragan Mladjenovic wrote: > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 5abeb86..9155b74 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -1242,6 +1242,16 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > /* Adjust the PT_PHDR value by the runtime load address. */ > l->l_phdr = (ElfW(Phdr) *) ((ElfW(Addr)) l->l_phdr + l->l_addr); > > +#ifdef USE_DL_EXEC_STACK_OVERRIDE > + /* Program requests a non-executable stack, but architecture does > + not support it. */ > + if (__glibc_unlikely (_dl_exec_stack_override (&stack_flags) != 0)) > + { > + errstring = N_("cannot override stack memory protections"); > + goto call_lose_errno; > + } > +#endif This sort of #ifdef is not proper glibc style. You should have a default trivial (inline?) definition of _dl_exec_stack_override and then have MIPS override the file with that function definition (without duplicating any architecture-independent code in the process). If you have a default inline function definition, that means all this code gets checked for syntax when building for any architecture, not just for MIPS.
* Dragan Mladjenovic: > +#ifdef USE_DL_EXEC_STACK_OVERRIDE > + /* Program requests a non-executable stack, but architecture does > + not support it. */ > + if (__glibc_unlikely (_dl_exec_stack_override (&stack_flags) != 0)) > + { > + errstring = N_("cannot override stack memory protections"); > + goto call_lose_errno; > + } > +#endif Is the comment really correct? I think the proposed MIPS implementation is not architecture-specific, but specific to the kernel version. Thanks, Florian
Florian Weimer * > Is the comment really correct? I think the proposed MIPS implementation > is not architecture-specific, but specific to the kernel version. My mistake. I guess the correct term would be machine in glibc nomenclature. Joseph Mayers* >> > +#ifdef USE_DL_EXEC_STACK_OVERRIDE >> > + /* Program requests a non-executable stack, but architecture does >> > + not support it. */ >> > + if (__glibc_unlikely (_dl_exec_stack_override (&stack_flags) != 0)) >> > + { >> > + errstring = N_("cannot override stack memory protections"); >> > + goto call_lose_errno; >> > + } >> > +#endif > This sort of #ifdef is not proper glibc style. You should have a default > trivial (inline?) definition of _dl_exec_stack_override and then have MIPS > override the file with that function definition (without duplicating any > architecture-independent code in the process). If you have a default > inline function definition, that means all this code gets checked for > syntax when building for any architecture, not just for MIPS. > Is patch below more suitable? We would have a common _dl_exec_stack_override that would (preferably) not be overridden by the machine support, but instead one can define DL_EXEC_STACK_OVERRIDE or maybe DL_EXEC_STACK_OVERRIDE_P to control the conditions under which the stack "override" happens. I realized that I don't actually need to duplicate the __stack_prot unprotect/protect code from elf/dl-load.c, so I moved the dynamic linking case into dl-main. This way the version check is done at most once. Static linking case is still done as part of _dl_non_dynamic_init. If we ever gain support for IFUNC on MIPS we would probably need to move this somewhere before running IFUNC revolvers. What are your thoughts on this? diff --git a/elf/dl-exec-stack-override.h b/elf/dl-exec-stack-override.h new file mode 100644 index 0000000..10401a8 --- /dev/null +++ b/elf/dl-exec-stack-override.h @@ -0,0 +1,36 @@ +/* Make stack executable if the machine requires it. Generic version. ... + +#include <ldsodefs.h> + +extern int __stack_prot attribute_relro attribute_hidden; + +static __always_inline void +_dl_exec_stack_override (void) +{ + if (__glibc_unlikely ((GL(dl_stack_flags) & PF_X) == 0 + && DL_EXEC_STACK_OVERRIDE)) + { + __stack_prot |= PROT_READ|PROT_WRITE|PROT_EXEC; + + void *stack_end = __libc_stack_end; + int err = _dl_make_stack_executable (&stack_end); + if (__glibc_unlikely (err)) + _dl_fatal_printf ("cannot enable executable stack as machine requires\n"); + } +} diff --git a/elf/dl-support.c b/elf/dl-support.c index 0a8b636..923aa4c 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -29,6 +29,7 @@ #include <dl-machine.h> #include <libc-lock.h> #include <dl-cache.h> +#include <dl-exec-stack-override.h> #include <dl-librecon.h> #include <dl-procinfo.h> #include <unsecvars.h> @@ -375,6 +376,8 @@ _dl_non_dynamic_init (void) _dl_stack_flags = _dl_phdr[i].p_flags; break; } + + _dl_exec_stack_override (); } #ifdef DL_SYSINFO_IMPLEMENTATION diff --git a/elf/rtld.c b/elf/rtld.c index c9490ff..f3e00f9 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -36,6 +36,7 @@ #include <dl-librecon.h> #include <unsecvars.h> #include <dl-cache.h> +#include <dl-exec-stack-override.h> #include <dl-osinfo.h> #include <dl-procinfo.h> #include <dl-prop.h> @@ -1542,6 +1543,8 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]); DL_SYSDEP_OSCHECK (_dl_fatal_printf); #endif + _dl_exec_stack_override (); + /* Initialize the data structures for the search paths for shared objects. */ _dl_init_paths (library_path); diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index b1fc5c3..70e96c0 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -119,6 +119,10 @@ dl_symbol_visibility_binds_local_p (const ElfW(Sym) *sym) # define DL_STATIC_INIT(map) #endif +#ifndef DL_EXEC_STACK_OVERRIDE +# define DL_EXEC_STACK_OVERRIDE false +#endif + /* Reloc type classes as returned by elf_machine_type_class(). ELF_RTYPE_CLASS_PLT means this reloc should not be satisfied by some PLT symbol, ELF_RTYPE_CLASS_COPY means this reloc should not be
diff --git a/elf/dl-load.c b/elf/dl-load.c index 5abeb86..9155b74 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1242,6 +1242,16 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, /* Adjust the PT_PHDR value by the runtime load address. */ l->l_phdr = (ElfW(Phdr) *) ((ElfW(Addr)) l->l_phdr + l->l_addr); +#ifdef USE_DL_EXEC_STACK_OVERRIDE + /* Program requests a non-executable stack, but architecture does + not support it. */ + if (__glibc_unlikely (_dl_exec_stack_override (&stack_flags) != 0)) + { + errstring = N_("cannot override stack memory protections"); + goto call_lose_errno; + } +#endif + if (__glibc_unlikely ((stack_flags &~ GL(dl_stack_flags)) & PF_X)) { /* The stack is presently not executable, but this module diff --git a/elf/dl-support.c b/elf/dl-support.c index 0a8b636..dd99d58 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -179,7 +179,6 @@ ElfW(Word) _dl_stack_flags = DEFAULT_STACK_PERMS; It returns an errno code or zero on success. */ int (*_dl_make_stack_executable_hook) (void **) = _dl_make_stack_executable; - /* Function in libpthread to wait for termination of lookups. */ void (*_dl_wait_lookup_done) (void); @@ -375,6 +374,13 @@ _dl_non_dynamic_init (void) _dl_stack_flags = _dl_phdr[i].p_flags; break; } + +#ifdef USE_DL_EXEC_STACK_OVERRIDE + if (__glibc_unlikely (_dl_exec_stack_override (&_dl_stack_flags) != 0)) + { + _dl_fatal_printf ("cannot override stack memory protections\n"); + } +#endif } #ifdef DL_SYSINFO_IMPLEMENTATION diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index b1fc5c3..4e1f0f1 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -642,6 +642,10 @@ extern size_t _dl_phnum; extern int _dl_make_stack_executable (void **stack_endp); rtld_hidden_proto (_dl_make_stack_executable) +#ifdef USE_DL_EXEC_STACK_OVERRIDE +extern int _dl_exec_stack_override (void *); +rtld_hidden_proto (_dl_exec_stack_override) +#endif /* Variable pointing to the end of the stack (or close to it). This value must be constant over the runtime of the application. Some programs might use the variable which results in copy relocations on some