Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
Hi! On Mon, Sep 17, 2018 at 12:15:05PM +, Christophe Leroy wrote: > Now, GCC offers the possibility to manually set the > stack-protector mode (global or tls) regardless of libc support. Yup :-) > This time, the patch selects HAVE_STACKPROTECTOR only if > -mstack-protector-guard=global is supported by GCC. "global" is weaker than "tls" (it is easier to read the cookie in an exploit). It is better to use tls if you can. Segher
Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
Hi Christophe, Thank you for the patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on v4.19-rc4 next-20180913] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-initial-stack-protector-fstack-protector-support/20180917-202227 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-ppc6xx_defconfig (attached as .config) compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=powerpc All errors (new ones prefixed by >>): arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf': >> bootx_init.c:(.init.text+0x2bc): undefined reference to >> `__stack_chk_fail_local' arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_add_display_props.isra.1': bootx_init.c:(.init.text+0x750): undefined reference to `__stack_chk_fail_local' arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_scan_dt_build_struct': bootx_init.c:(.init.text+0xa84): undefined reference to `__stack_chk_fail_local' arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_init': bootx_init.c:(.init.text+0xf48): undefined reference to `__stack_chk_fail_local' powerpc-linux-gnu-ld: .tmp_vmlinux1: hidden symbol `__stack_chk_fail_local' isn't defined powerpc-linux-gnu-ld: final link failed: Bad value --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
Partialy copied from commit c743f38013aef ("ARM: initial stack protector
(-fstack-protector) support")
This is the very basic stuff without the changing canary upon
task switch yet. Just the Kconfig option and a constant canary
value initialized at boot time.
This patch was tentatively added in the past (commit 6533b7c16ee5
("powerpc: Initial stack protector (-fstack-protector) support"))
but had to be reverted (commit f2574030b0e3 ("powerpc: Revert the
initial stack protector support") because GCC implementing it
differently whether it had been built with libc support or not.
Now, GCC offers the possibility to manually set the
stack-protector mode (global or tls) regardless of libc support.
This time, the patch selects HAVE_STACKPROTECTOR only if
-mstack-protector-guard=global is supported by GCC.
$ echo CORRUPT_STACK > /sys/kernel/debug/provoke-crash/DIRECT
[ 134.943666] Kernel panic - not syncing: stack-protector: Kernel stack is
corrupted in: lkdtm_CORRUPT_STACK+0x64/0x64
[ 134.943666]
[ 134.955414] CPU: 0 PID: 283 Comm: sh Not tainted
4.18.0-s3k-dev-12143-ga3272be41209 #835
[ 134.963380] Call Trace:
[ 134.965860] [c6615d60] [c001f76c] panic+0x118/0x260 (unreliable)
[ 134.971775] [c6615dc0] [c001f654] panic+0x0/0x260
[ 134.976435] [c6615dd0] [c032c368] lkdtm_CORRUPT_STACK_STRONG+0x0/0x64
[ 134.982769] [c6615e00] [] 0x
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/Makefile | 4 +++
arch/powerpc/include/asm/stackprotector.h | 41 +++
arch/powerpc/kernel/Makefile | 4 +++
arch/powerpc/kernel/process.c | 6 +
5 files changed, 56 insertions(+)
create mode 100644 arch/powerpc/include/asm/stackprotector.h
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index db0b6eebbfa5..3f5776ed99d3 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -181,6 +181,7 @@ config PPC
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_TRACEHOOK
select HAVE_CBPF_JITif !PPC64
+ select HAVE_STACKPROTECTOR if
$(cc-option,-mstack-protector-guard=global)
select HAVE_CONTEXT_TRACKINGif PPC64
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 8397c7bd5880..0dbfdb6a145d 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -112,6 +112,10 @@ LDFLAGS+= -m elf$(BITS)$(LDEMULATION)
KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
endif
+ifdef CONFIG_STACKPROTECTOR
+KBUILD_CFLAGS += -mstack-protector-guard=global
+endif
+
LDFLAGS_vmlinux-y := -Bstatic
LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
LDFLAGS_vmlinux:= $(LDFLAGS_vmlinux-y)
diff --git a/arch/powerpc/include/asm/stackprotector.h
b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index ..2556e227cdb2
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GCC stack protector support.
+ *
+ * Stack protector works by putting predefined pattern at the start of
+ * the stack frame and verifying that it hasn't been overwritten when
+ * returning from the function. The pattern is called stack canary
+ * and gcc expects it to be defined by a global variable called
+ * "__stack_chk_guard" on PPC. This unfortunately means that on SMP
+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H
+
+#include
+#include
+#include
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+ unsigned long canary;
+
+ /* Try to get a semi random initial value. */
+ get_random_bytes(&canary, sizeof(canary));
+ canary ^= mftb();
+ canary ^= LINUX_VERSION_CODE;
+
+ current->stack_canary = canary;
+ __stack_chk_guard = current->stack_canary;
+}
+
+#endif /* _ASM_STACKPROTECTOR_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 3b66f2c19c84..0556a7243d2a 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -20,6 +20,10 @@ CFLAGS_prom_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
+# -fstack-protector triggers protection checks in this code,
+# but it is being used too early to link to meaningful stack_chk logic.
+CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)
+
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
CFLAGS_REMOVE_cputable.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
diff --git a/arch/powerpc/kernel/process
Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
Le 17/11/2016 à 12:05, Michael Ellerman a écrit :
Hi Michael,
I took your comments into account in v2. Shame on me, I forgot to add
the list of changes from v1 to v2 in the commit log.
Christophe
Christophe Leroy writes:
diff --git a/arch/powerpc/include/asm/stackprotector.h
b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 000..de00332
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,38 @@
+/*
+ * GCC stack protector support.
+ *
+ * Stack protector works by putting predefined pattern at the start of
+ * the stack frame and verifying that it hasn't been overwritten when
+ * returning from the function. The pattern is called stack canary
+ * and gcc expects it to be defined by a global variable called
+ * "__stack_chk_guard" on ARM. This unfortunately means that on SMP
^
PPC
+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H 1
We usually just define it, not define it to 1.
+
+#include
+#include
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+ unsigned long canary;
+
+ /* Try to get a semi random initial value. */
+ get_random_bytes(&canary, sizeof(canary));
+ canary ^= LINUX_VERSION_CODE;
What about mixing in an mftb() as well ?
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index e59ed6a..4a62179 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -19,6 +19,11 @@ CFLAGS_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
+# -fstack-protector triggers protection checks in this code,
+# but it is being used too early to link to meaningful stack_chk logic.
+nossp_flags := $(call cc-option, -fno-stack-protector)
+CFLAGS_prom_init.o := $(nossp_flags)
We've already assigned to CFLAGS_prom_init.o so I think you should be
using += not := shouldn't you?
Also it could just be a single line:
CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)
cheers
Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
Christophe Leroy writes:
> diff --git a/arch/powerpc/include/asm/stackprotector.h
> b/arch/powerpc/include/asm/stackprotector.h
> new file mode 100644
> index 000..de00332
> --- /dev/null
> +++ b/arch/powerpc/include/asm/stackprotector.h
> @@ -0,0 +1,38 @@
> +/*
> + * GCC stack protector support.
> + *
> + * Stack protector works by putting predefined pattern at the start of
> + * the stack frame and verifying that it hasn't been overwritten when
> + * returning from the function. The pattern is called stack canary
> + * and gcc expects it to be defined by a global variable called
> + * "__stack_chk_guard" on ARM. This unfortunately means that on SMP
^
PPC
> + * we cannot have a different canary value per task.
> + */
> +
> +#ifndef _ASM_STACKPROTECTOR_H
> +#define _ASM_STACKPROTECTOR_H 1
We usually just define it, not define it to 1.
> +
> +#include
> +#include
> +
> +extern unsigned long __stack_chk_guard;
> +
> +/*
> + * Initialize the stackprotector canary value.
> + *
> + * NOTE: this must only be called from functions that never return,
> + * and it must always be inlined.
> + */
> +static __always_inline void boot_init_stack_canary(void)
> +{
> + unsigned long canary;
> +
> + /* Try to get a semi random initial value. */
> + get_random_bytes(&canary, sizeof(canary));
> + canary ^= LINUX_VERSION_CODE;
What about mixing in an mftb() as well ?
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index e59ed6a..4a62179 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -19,6 +19,11 @@ CFLAGS_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
> CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
> CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
>
> +# -fstack-protector triggers protection checks in this code,
> +# but it is being used too early to link to meaningful stack_chk logic.
> +nossp_flags := $(call cc-option, -fno-stack-protector)
> +CFLAGS_prom_init.o := $(nossp_flags)
We've already assigned to CFLAGS_prom_init.o so I think you should be
using += not := shouldn't you?
Also it could just be a single line:
CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)
cheers
[PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
Partialy copied from commit c743f38013aef ("ARM: initial stack protector
(-fstack-protector) support")
This is the very basic stuff without the changing canary upon
task switch yet. Just the Kconfig option and a constant canary
value initialized at boot time.
Cc: Nicolas Pitre
Signed-off-by: Christophe Leroy
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/stackprotector.h | 38 +++
arch/powerpc/kernel/Makefile | 5
arch/powerpc/kernel/process.c | 6 +
4 files changed, 50 insertions(+)
create mode 100644 arch/powerpc/include/asm/stackprotector.h
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 59e53f4..2947dab 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -162,6 +162,7 @@ config PPC
select HAVE_VIRT_CPU_ACCOUNTING
select HAVE_ARCH_HARDENED_USERCOPY
select HAVE_KERNEL_GZIP
+ select HAVE_CC_STACKPROTECTOR
config GENERIC_CSUM
def_bool CPU_LITTLE_ENDIAN
diff --git a/arch/powerpc/include/asm/stackprotector.h
b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 000..de00332
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,38 @@
+/*
+ * GCC stack protector support.
+ *
+ * Stack protector works by putting predefined pattern at the start of
+ * the stack frame and verifying that it hasn't been overwritten when
+ * returning from the function. The pattern is called stack canary
+ * and gcc expects it to be defined by a global variable called
+ * "__stack_chk_guard" on ARM. This unfortunately means that on SMP
+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H 1
+
+#include
+#include
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+ unsigned long canary;
+
+ /* Try to get a semi random initial value. */
+ get_random_bytes(&canary, sizeof(canary));
+ canary ^= LINUX_VERSION_CODE;
+
+ current->stack_canary = canary;
+ __stack_chk_guard = current->stack_canary;
+}
+
+#endif /* _ASM_STACKPROTECTOR_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index e59ed6a..4a62179 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -19,6 +19,11 @@ CFLAGS_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
+# -fstack-protector triggers protection checks in this code,
+# but it is being used too early to link to meaningful stack_chk logic.
+nossp_flags := $(call cc-option, -fno-stack-protector)
+CFLAGS_prom_init.o := $(nossp_flags)
+
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
CFLAGS_REMOVE_cputable.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index ce8a26a..ba8f32a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -64,6 +64,12 @@
#include
#include
+#ifdef CONFIG_CC_STACKPROTECTOR
+#include
+unsigned long __stack_chk_guard __read_mostly;
+EXPORT_SYMBOL(__stack_chk_guard);
+#endif
+
/* Transactional Memory debug */
#ifdef TM_DEBUG_SW
#define TM_DEBUG(x...) printk(KERN_INFO x)
--
2.1.0
