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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+rthh_-nbyb=9TU5X=2gDMMgzngU6LqLejRMU8GNvrDZ1kVTA@mail.gmail.com>
Date:	Sun, 27 Apr 2014 12:03:50 +0200
From:	Mathias Krause <minipli@...glemail.com>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf x86: Fix perf to use non-executable stack, again

On 27 April 2014 11:26, Jiri Olsa <jolsa@...hat.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@...glemail.com>
>> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
>> Cc: Paul Mackerras <paulus@...ba.org>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
>> Cc: Jiri Olsa <jolsa@...hat.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),)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/