[PATCH] perf x86: Fix perf to use non-executable stack, again

[PATCH] perf x86: Fix perf to use non-executable stack, again

[Mathias Krause]

arch/x86/tests/regs_load.S is missing the linker note about the stack
requirements, therefore making the linker fall back to an executable
stack. As this object gets linked against the final perf binary, it'll
needlessly end up with an executable stack.

Fix this by adding the appropriate linker note.

Fixes: 3c8b06f981 ("perf tests x86: Introduce perf_regs_load function")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>

---
 tools/perf/arch/x86/tests/regs_load.S |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/regs_load.S b/tools/perf/arch/x86/tests/regs_load.S
index 99167bf644..60875d5c55 100644
--- a/tools/perf/arch/x86/tests/regs_load.S
+++ b/tools/perf/arch/x86/tests/regs_load.S
@@ -1,4 +1,3 @@
-
 #include <linux/linkage.h>
 
 #define AX	 0
@@ -90,3 +89,10 @@ ENTRY(perf_regs_load)
 	ret
 ENDPROC(perf_regs_load)
 #endif
+
+/*
+ * We need to provide note.GNU-stack section, saying that we want
+ * NOT executable stack. Otherwise the final linking will assume that
+ * the ELF stack should not be restricted at all and set it RWX.
+ */
+.section .note.GNU-stack,"",@progbits
-- 
1.7.10.4

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Jiri Olsa]

On Sat, Apr 26, 2014 at 09:02:45PM +0200, Mathias Krause wrote:
> arch/x86/tests/regs_load.S is missing the linker note about the stack
> requirements, therefore making the linker fall back to an executable
> stack. As this object gets linked against the final perf binary, it'll
> needlessly end up with an executable stack.
> 
> Fix this by adding the appropriate linker note.
> 
> Fixes: 3c8b06f981 ("perf tests x86: Introduce perf_regs_load function")
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> 
> ---
>  tools/perf/arch/x86/tests/regs_load.S |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/arch/x86/tests/regs_load.S b/tools/perf/arch/x86/tests/regs_load.S
> index 99167bf644..60875d5c55 100644
> --- a/tools/perf/arch/x86/tests/regs_load.S
> +++ b/tools/perf/arch/x86/tests/regs_load.S
> @@ -1,4 +1,3 @@
> -
>  #include <linux/linkage.h>
>  
>  #define AX	 0
> @@ -90,3 +89,10 @@ ENTRY(perf_regs_load)
>  	ret
>  ENDPROC(perf_regs_load)
>  #endif
> +
> +/*
> + * We need to provide note.GNU-stack section, saying that we want
> + * NOT executable stack. Otherwise the final linking will assume that
> + * the ELF stack should not be restricted at all and set it RWX.
> + */
> +.section .note.GNU-stack,"",@progbits
> -- 
> 1.7.10.4
> 

hum, how about fixing this once and for all.. ;-)
please check attached patch, thanks

jirka


---
diff --git a/tools/perf/bench/mem-memcpy-x86-64-asm.S b/tools/perf/bench/mem-memcpy-x86-64-asm.S
index fcd9cf0..a20780b 100644
--- a/tools/perf/bench/mem-memcpy-x86-64-asm.S
+++ b/tools/perf/bench/mem-memcpy-x86-64-asm.S
@@ -4,9 +4,3 @@
 #define Lmemcpy_c globl memcpy_c; memcpy_c
 #define Lmemcpy_c_e globl memcpy_c_e; memcpy_c_e
 #include "../../../arch/x86/lib/memcpy_64.S"
-/*
- * We need to provide note.GNU-stack section, saying that we want
- * NOT executable stack. Otherwise the final linking will assume that
- * the ELF stack should not be restricted at all and set it RWX.
- */
-.section .note.GNU-stack,"",@progbits
diff --git a/tools/perf/bench/mem-memset-x86-64-asm.S b/tools/perf/bench/mem-memset-x86-64-asm.S
index 9e5af89..cb92170 100644
--- a/tools/perf/bench/mem-memset-x86-64-asm.S
+++ b/tools/perf/bench/mem-memset-x86-64-asm.S
@@ -4,10 +4,3 @@
 #define Lmemset_c globl memset_c; memset_c
 #define Lmemset_c_e globl memset_c_e; memset_c_e
 #include "../../../arch/x86/lib/memset_64.S"
-
-/*
- * We need to provide note.GNU-stack section, saying that we want
- * NOT executable stack. Otherwise the final linking will assume that
- * the ELF stack should not be restricted at all and set it RWX.
- */
-.section .note.GNU-stack,"",@progbits
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index d50869e..dd56b6b 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -117,6 +117,9 @@ CFLAGS += -Wall
 CFLAGS += -Wextra
 CFLAGS += -std=gnu99
 
+# force non-executable stack
+LDFLAGS += -Wl,-z,noexecstack
+
 EXTLIBS = -lelf -lpthread -lrt -lm -ldl
 
 ifneq ($(OUTPUT),)

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Mathias Krause]

On 27 April 2014 11:26, Jiri Olsa <jolsa@redhat.com> wrote:
> On Sat, Apr 26, 2014 at 09:02:45PM +0200, Mathias Krause wrote:
>> arch/x86/tests/regs_load.S is missing the linker note about the stack
>> requirements, therefore making the linker fall back to an executable
>> stack. As this object gets linked against the final perf binary, it'll
>> needlessly end up with an executable stack.
>>
>> Fix this by adding the appropriate linker note.
>>
>> Fixes: 3c8b06f981 ("perf tests x86: Introduce perf_regs_load function")
>> Signed-off-by: Mathias Krause <minipli@googlemail.com>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>>
>> ---
>>  tools/perf/arch/x86/tests/regs_load.S |    8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/arch/x86/tests/regs_load.S b/tools/perf/arch/x86/tests/regs_load.S
>> index 99167bf644..60875d5c55 100644
>> --- a/tools/perf/arch/x86/tests/regs_load.S
>> +++ b/tools/perf/arch/x86/tests/regs_load.S
>> @@ -1,4 +1,3 @@
>> -
>>  #include <linux/linkage.h>
>>
>>  #define AX    0
>> @@ -90,3 +89,10 @@ ENTRY(perf_regs_load)
>>       ret
>>  ENDPROC(perf_regs_load)
>>  #endif
>> +
>> +/*
>> + * We need to provide note.GNU-stack section, saying that we want
>> + * NOT executable stack. Otherwise the final linking will assume that
>> + * the ELF stack should not be restricted at all and set it RWX.
>> + */
>> +.section .note.GNU-stack,"",@progbits
>> --
>> 1.7.10.4
>>
>
> hum, how about fixing this once and for all.. ;-)
> please check attached patch, thanks

Yeah, I thought about this option too but declined it for two reasons:
1/ The kernel sources should provide good quality examples, even for
usage outside of the kernel. Imagine somebody taking the memcpy
implementation for his own project but not copying the LDFLAGS. That
would make his code have an executable stack while with the .GNU-stack
marker in the assembler file it won't.
2/ What if somebody tries to add/link code to perf that makes use of
nested functions? That'll make perf fail as the trampoline code
generated by gcc won't be executable due to the enforced
non-executable stack by -Wl,-z,noexecstack.

So, consistently adding the (brain damaged!) .GNU-stack annotations to
assembler files handles both cases well. It serves as a good example
to fulfill the requirements for the PT_GNU_STACK program header and
will also work out of the box in case someone later on tries to add
code to perf that makes use of nested functions. The latter might even
be in code outside of perf, e.g. in some library. Adding
-Wl,-z,noexecstack unconditionally would needlessly break that case.

Thanks,
Mathias

>
> jirka
>
>
> ---
> diff --git a/tools/perf/bench/mem-memcpy-x86-64-asm.S b/tools/perf/bench/mem-memcpy-x86-64-asm.S
> index fcd9cf0..a20780b 100644
> --- a/tools/perf/bench/mem-memcpy-x86-64-asm.S
> +++ b/tools/perf/bench/mem-memcpy-x86-64-asm.S
> @@ -4,9 +4,3 @@
>  #define Lmemcpy_c globl memcpy_c; memcpy_c
>  #define Lmemcpy_c_e globl memcpy_c_e; memcpy_c_e
>  #include "../../../arch/x86/lib/memcpy_64.S"
> -/*
> - * We need to provide note.GNU-stack section, saying that we want
> - * NOT executable stack. Otherwise the final linking will assume that
> - * the ELF stack should not be restricted at all and set it RWX.
> - */
> -.section .note.GNU-stack,"",@progbits
> diff --git a/tools/perf/bench/mem-memset-x86-64-asm.S b/tools/perf/bench/mem-memset-x86-64-asm.S
> index 9e5af89..cb92170 100644
> --- a/tools/perf/bench/mem-memset-x86-64-asm.S
> +++ b/tools/perf/bench/mem-memset-x86-64-asm.S
> @@ -4,10 +4,3 @@
>  #define Lmemset_c globl memset_c; memset_c
>  #define Lmemset_c_e globl memset_c_e; memset_c_e
>  #include "../../../arch/x86/lib/memset_64.S"
> -
> -/*
> - * We need to provide note.GNU-stack section, saying that we want
> - * NOT executable stack. Otherwise the final linking will assume that
> - * the ELF stack should not be restricted at all and set it RWX.
> - */
> -.section .note.GNU-stack,"",@progbits
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index d50869e..dd56b6b 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -117,6 +117,9 @@ CFLAGS += -Wall
>  CFLAGS += -Wextra
>  CFLAGS += -std=gnu99
>
> +# force non-executable stack
> +LDFLAGS += -Wl,-z,noexecstack
> +
>  EXTLIBS = -lelf -lpthread -lrt -lm -ldl
>
>  ifneq ($(OUTPUT),)

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Jiri Olsa]

On Sun, Apr 27, 2014 at 12:03:50PM +0200, Mathias Krause wrote:

SNIP

> >> 1.7.10.4
> >>
> >
> > hum, how about fixing this once and for all.. ;-)
> > please check attached patch, thanks
> 
> Yeah, I thought about this option too but declined it for two reasons:
> 1/ The kernel sources should provide good quality examples, even for
> usage outside of the kernel. Imagine somebody taking the memcpy
> implementation for his own project but not copying the LDFLAGS. That
> would make his code have an executable stack while with the .GNU-stack
> marker in the assembler file it won't.

that 'somebody' should check/know better ;-) but ok, fair enough

> 2/ What if somebody tries to add/link code to perf that makes use of
> nested functions? That'll make perf fail as the trampoline code
> generated by gcc won't be executable due to the enforced
> non-executable stack by -Wl,-z,noexecstack.

I guess in that case he would change the Makefile as well?

anyway I have no objection for leaving that code in assembly
objects, but I suggest we use the global option as well to
prevent any future surprise..

or insert test case for perf's executable stack to 'perf test'

thanks,
jirka

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Sun, Apr 27, 2014 at 12:03:50PM +0200, Mathias Krause wrote:
> 
> SNIP
> 
> > >> 1.7.10.4
> > >>
> > >
> > > hum, how about fixing this once and for all.. ;-)
> > > please check attached patch, thanks
> > 
> > Yeah, I thought about this option too but declined it for two reasons:
> > 1/ The kernel sources should provide good quality examples, even for
> > usage outside of the kernel. Imagine somebody taking the memcpy
> > implementation for his own project but not copying the LDFLAGS. That
> > would make his code have an executable stack while with the .GNU-stack
> > marker in the assembler file it won't.
> 
> that 'somebody' should check/know better ;-) but ok, fair enough
> 
> > 2/ What if somebody tries to add/link code to perf that makes use of
> > nested functions? That'll make perf fail as the trampoline code
> > generated by gcc won't be executable due to the enforced
> > non-executable stack by -Wl,-z,noexecstack.
> 
> I guess in that case he would change the Makefile as well?
> 
> anyway I have no objection for leaving that code in assembly
> objects, but I suggest we use the global option as well to
> prevent any future surprise..

Correct. Regression in this area is 'silent' and can go unnoticed for 
long, because it has no functional side effects. So being defensive 
and forcing the secure option we want is important.

That way if an executable stack is needed by a patch then perf will 
break visibly, and the patch that relies on an executable stack can be 
fixed ...

So for the combo solution:

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Mathias Krause]

On 27 April 2014 12:39, Jiri Olsa <jolsa@redhat.com> wrote:
> On Sun, Apr 27, 2014 at 12:03:50PM +0200, Mathias Krause wrote:
> [...]
>> 2/ What if somebody tries to add/link code to perf that makes use of
>> nested functions? That'll make perf fail as the trampoline code
>> generated by gcc won't be executable due to the enforced
>> non-executable stack by -Wl,-z,noexecstack.
>
> I guess in that case he would change the Makefile as well?

Not necessarily. What if a later version of a library already used by
perf needs an executable stack because it now makes use of nested
functions? Unlikely, though in that case no change to perf would be
made, but perf would then require an executable stack, too.

Anyway, as Ingo votes for the global linker option as well, I'll send
a v2 of the patch containing your suggested linker flag.

> anyway I have no objection for leaving that code in assembly
> objects, but I suggest we use the global option as well to
> prevent any future surprise..

Okay.

> or insert test case for perf's executable stack to 'perf test'

That won't work for systems preventing processes getting an executable
stack in the first place. That was the reason I stumbled about the
problem in the first place.


Thanks,
Mathias

>
> thanks,
> jirka

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Jiri Olsa]

On Sun, Apr 27, 2014 at 06:07:30PM +0200, Mathias Krause wrote:
> On 27 April 2014 12:39, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Sun, Apr 27, 2014 at 12:03:50PM +0200, Mathias Krause wrote:
> > [...]
> >> 2/ What if somebody tries to add/link code to perf that makes use of
> >> nested functions? That'll make perf fail as the trampoline code
> >> generated by gcc won't be executable due to the enforced
> >> non-executable stack by -Wl,-z,noexecstack.
> >
> > I guess in that case he would change the Makefile as well?
> 
> Not necessarily. What if a later version of a library already used by
> perf needs an executable stack because it now makes use of nested
> functions? Unlikely, though in that case no change to perf would be
> made, but perf would then require an executable stack, too.

I tried you can run binary with noexecstack having dynamic
library dependency wit execstack

> 
> Anyway, as Ingo votes for the global linker option as well, I'll send
> a v2 of the patch containing your suggested linker flag.

cool

> 
> > anyway I have no objection for leaving that code in assembly
> > objects, but I suggest we use the global option as well to
> > prevent any future surprise..
> 
> Okay.
> 
> > or insert test case for perf's executable stack to 'perf test'
> 
> That won't work for systems preventing processes getting an executable
> stack in the first place. That was the reason I stumbled about the

could be disabled on such systems

jirka

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Mathias Krause]

On 27 April 2014 18:16, Jiri Olsa <jolsa@redhat.com> wrote:
> On Sun, Apr 27, 2014 at 06:07:30PM +0200, Mathias Krause wrote:
>> On 27 April 2014 12:39, Jiri Olsa <jolsa@redhat.com> wrote:
>> > On Sun, Apr 27, 2014 at 12:03:50PM +0200, Mathias Krause wrote:
>> > [...]
>> >> 2/ What if somebody tries to add/link code to perf that makes use of
>> >> nested functions? That'll make perf fail as the trampoline code
>> >> generated by gcc won't be executable due to the enforced
>> >> non-executable stack by -Wl,-z,noexecstack.
>> >
>> > I guess in that case he would change the Makefile as well?
>>
>> Not necessarily. What if a later version of a library already used by
>> perf needs an executable stack because it now makes use of nested
>> functions? Unlikely, though in that case no change to perf would be
>> made, but perf would then require an executable stack, too.
>
> I tried you can run binary with noexecstack having dynamic
> library dependency wit execstack

Well, it might work on your system but it won't work on mine. See this
bug, why: https://sourceware.org/bugzilla/show_bug.cgi?id=12492

> [...]
>>
>> > or insert test case for perf's executable stack to 'perf test'
>>
>> That won't work for systems preventing processes getting an executable
>> stack in the first place. That was the reason I stumbled about the
>
> could be disabled on such systems

Of course, it could be disabled, i.e. I could allow perf to get an
executable stack. Though, I don't like my stacks to be executable ;)

Thanks,
Mathias

>
> jirka

Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

[Ingo Molnar]

* Mathias Krause <minipli@googlemail.com> wrote:

> On 27 April 2014 18:16, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Sun, Apr 27, 2014 at 06:07:30PM +0200, Mathias Krause wrote:
> >> On 27 April 2014 12:39, Jiri Olsa <jolsa@redhat.com> wrote:
> >> > On Sun, Apr 27, 2014 at 12:03:50PM +0200, Mathias Krause wrote:
> >> > [...]
> >> >> 2/ What if somebody tries to add/link code to perf that makes use of
> >> >> nested functions? That'll make perf fail as the trampoline code
> >> >> generated by gcc won't be executable due to the enforced
> >> >> non-executable stack by -Wl,-z,noexecstack.
> >> >
> >> > I guess in that case he would change the Makefile as well?
> >>
> >> Not necessarily. What if a later version of a library already used by
> >> perf needs an executable stack because it now makes use of nested
> >> functions? Unlikely, though in that case no change to perf would be
> >> made, but perf would then require an executable stack, too.
> >
> > I tried you can run binary with noexecstack having dynamic
> > library dependency wit execstack
> 
> Well, it might work on your system but it won't work on mine. See this
> bug, why: https://sourceware.org/bugzilla/show_bug.cgi?id=12492
> 
> > [...]
> >>
> >> > or insert test case for perf's executable stack to 'perf test'
> >>
> >> That won't work for systems preventing processes getting an executable
> >> stack in the first place. That was the reason I stumbled about the
> >
> > could be disabled on such systems
> 
> Of course, it could be disabled, i.e. I could allow perf to get an 
> executable stack. Though, I don't like my stacks to be executable ;)

Absolutely. Using an executable stack is really just a legacy thing, 
it should be avoided (and I'd say it must be avoided) in any modern 
application.

Thanks,

	Ingo