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

[Dragan Mladjenovic <dmladjenovic at wavecomp dot com>]

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