[1/3,ELF] Allow the machine to override stack permissions via USE_DL_EXEC_STACK_OVERRIDE.

Message ID 1561672142-5907-2-git-send-email-dmladjenovic@wavecomp.com
State New
Headers show
Series
  • Mips support for PT_GNU_STACK
Related show

Commit Message

Dragan Mladjenovic June 27, 2019, 9:50 p.m.
This patch allows the machine-dependent code to override non-executable stack
permissions by defining USE_DL_EXEC_STACK_OVERRIDE and implementing _dl_exec_stack_override.
It is called early during the static startup after the os version check and during the load
of every shared library or main executable if ld.so is invoked explicitly.

	* elf/dl-load.c (_dl_map_object_from_fd): Call '_dl_exec_stack_override'.
	* elf/dl-support.c (_dl_non_dynamic_init): Likewise.
	* sysdeps/generic/ldsodefs.h (_dl_exec_stack_override): New prototype.
---
 elf/dl-load.c              | 10 ++++++++++
 elf/dl-support.c           |  8 +++++++-
 sysdeps/generic/ldsodefs.h |  4 ++++
 3 files changed, 21 insertions(+), 1 deletion(-)

-- 
1.9.1

Comments

Joseph Myers July 8, 2019, 11:33 a.m. | #1
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
Florian Weimer July 8, 2019, 11:50 a.m. | #2
* 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
Dragan Mladjenovic July 9, 2019, 9:35 p.m. | #3
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

Patch

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