[PATCH v6 1/3] elf: Do not change stack permission on dlopen/dlmopen

[Florian Weimer]

* Adhemerval Zanella:

> diff --git a/NEWS b/NEWS
> index a185f2e7ee..f91c4c2421 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -57,6 +57,12 @@ Deprecated and removed features, and other changes affecting compatibility:
>  
>  * The nios2*-*-linux-gnu configurations are no longer supported.
>  
> +* dlopen and dlmopen no longer make the stack executable if a shared
> +  library requires it, either implicitly because of a missing GNU_STACK ELF
> +  header (and default ABI permission having the executable bit set) or
> +  explicitly because of the executable bit in GNU_STACK, and the stack is
> +  not already executable.

Maybe add: “Instead, loading such objects will fail.”

As written, the entry is ambiguous whether dlopen/dlmopen fails or
continues without making the stack executable.

> diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c
> index 560b353918..cd758c089e 100644
> --- a/elf/tst-execstack.c
> +++ b/elf/tst-execstack.c
> @@ -9,6 +9,11 @@
>  #include <error.h>
>  #include <stackinfo.h>
>  
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/xthread.h>
> +#include <support/xdlfcn.h>
> +
>  static void
>  print_maps (void)
>  {
> @@ -20,11 +25,21 @@ print_maps (void)
>  #endif
>  }
>  
> -static void deeper (void (*f) (void));
> +#ifndef DEFAULT_RWX_STACK
> +# define DEFAULT_RWX_STACK 0
> +#else

Please add a comment at the start of the file explaining what the macro
does, and which variant of “default” is relevant here.  For example,
i386 has an executable stack by default in Linux for historic reasons,
but I think what DEFAULT_RWX_STACK refers to is what the toolchain
produces for ordinary programs (those not containing nested functions
etc., like this test case).

Did you repost the right patch?  I thought we had agreed to splitting
the cleanups from the actual changes.  But it's not too bad, so I think
this version is okay.

> -rtld_hidden_def (__nptl_change_stack_perm)
> diff --git a/sysdeps/unix/sysv/linux/mips/Makefile b/sysdeps/unix/sysv/linux/mips/Makefile
> index d5725c69d8..05ec9150b2 100644
> --- a/sysdeps/unix/sysv/linux/mips/Makefile
> +++ b/sysdeps/unix/sysv/linux/mips/Makefile
> @@ -61,6 +61,7 @@ ifeq ($(subdir),elf)
>  # this test is expected to fail.
>  ifneq ($(mips-has-gnustack),yes)
>  test-xfail-check-execstack = yes
> +CFLAGS-tst-execstack.c += -DDEFAULT_RWX_STACK=1
>  endif
>  endif
>  
> @@ -68,6 +69,12 @@ ifeq ($(subdir),stdlib)
>  gen-as-const-headers += ucontext_i.sym
>  endif
>  
> +ifeq ($(subdir),nptl)
> +ifeq ($(mips-force-execstack),yes)
> +CFLAGS-tst-execstack-threads.c += -DDEFAULT_RWX_STACK=1
> +endif
> +endif
> +
>  ifeq ($(mips-force-execstack),yes)
>  CFLAGS-.o += -Wa,-execstack
>  CFLAGS-.os += -Wa,-execstack

What about Hurd?  On both i686 and x86-64, ld.so has an RWX stack today,
so I think it's functionally the same as (historic) MIPS.  From the
perspective of the test case, an executable stack is unavoidable, but
the cause is different for Hurd (use of nested functions for kernel
calls, if I remember correctly).

The test doesn't care about the presence of other RWE segments, so
PowerPC -msecure-plt (or the lack thereof) should not matter to it.

The actual code changes look okay to me.

Thanks,
Florian