2018-09-24 15:16:17

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v3 1/2] powerpc/32: add stack protector support

This functionality 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 of 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=tls is supported by GCC.

On PPC32, as register r2 points to current task_struct at
all time, the stack_canary located inside task_struct can be
used directly by using the following GCC options:
-mstack-protector-guard=tls
-mstack-protector-guard-reg=r2
-mstack-protector-guard-offset=offsetof(struct task_struct, stack_canary))

$ 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] [ffffffff] 0xffffffff

Signed-off-by: Christophe Leroy <[email protected]>
---
v3: the offset is now defined by a rule in the Makefile. No need anymore
to take stack_canary out of the randomised area of task_struct

arch/powerpc/Kconfig | 1 +
arch/powerpc/Makefile | 10 +++++++++
arch/powerpc/include/asm/stackprotector.h | 34 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/Makefile | 4 ++++
arch/powerpc/kernel/asm-offsets.c | 3 +++
5 files changed, 52 insertions(+)
create mode 100644 arch/powerpc/include/asm/stackprotector.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a80669209155..3bcb05929931 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -180,6 +180,7 @@ config PPC
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_TRACEHOOK
select HAVE_CBPF_JIT if !PPC64
+ select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls) && PPC32
select HAVE_CONTEXT_TRACKING if PPC64
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 07d9dce7eda6..45b8eb4d8fe7 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -112,6 +112,9 @@ KBUILD_LDFLAGS += -m elf$(BITS)$(LDEMULATION)
KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
endif

+cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
+cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
+
LDFLAGS_vmlinux-y := -Bstatic
LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-y)
@@ -404,6 +407,13 @@ archclean:

archprepare: checkbin

+ifdef CONFIG_STACKPROTECTOR
+prepare: stack_protector_prepare
+
+stack_protector_prepare: prepare0
+ $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
+endif
+
# Use the file '.tmp_gas_check' for binutils tests, as gas won't output
# to stdout and these checks are run even on install targets.
TOUT := .tmp_gas_check
diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 000000000000..263e2aab1862
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GCC stack protector support.
+ *
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H
+
+#include <linux/random.h>
+#include <linux/version.h>
+#include <asm/reg.h>
+#include <asm/current.h>
+#include <asm/paca.h>
+
+/*
+ * 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;
+}
+
+#endif /* _ASM_STACKPROTECTOR_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 1e64cfe22a83..01274ac45968 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 = $(CC_FLAGS_FTRACE)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index ba9d0fc98730..a992f7a53cf3 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -79,6 +79,9 @@ int main(void)
{
OFFSET(THREAD, task_struct, thread);
OFFSET(MM, task_struct, mm);
+#ifdef CONFIG_STACKPROTECTOR
+ OFFSET(TASK_CANARY, task_struct, stack_canary);
+#endif
OFFSET(MMCONTEXTID, mm_struct, context.id);
#ifdef CONFIG_PPC64
DEFINE(SIGSEGV, SIGSEGV);
--
2.13.3



2018-09-24 15:16:09

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v3 2/2] powerpc/64: add stack protector support

On PPC64, as register r13 points to the paca_struct at all time,
this patch adds a copy of the canary there, which is copied at
task_switch.
That new canary is then used by using the following GCC options:
-mstack-protector-guard=tls
-mstack-protector-guard-reg=r13
-mstack-protector-guard-offset=offsetof(struct paca_struct, canary))

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/Makefile | 8 ++++++++
arch/powerpc/include/asm/paca.h | 3 +++
arch/powerpc/include/asm/stackprotector.h | 3 +++
arch/powerpc/kernel/asm-offsets.c | 3 +++
arch/powerpc/kernel/entry_64.S | 4 ++++
6 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 3bcb05929931..602eea723624 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -180,7 +180,7 @@ config PPC
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_TRACEHOOK
select HAVE_CBPF_JIT if !PPC64
- select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls) && PPC32
+ select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls)
select HAVE_CONTEXT_TRACKING if PPC64
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 45b8eb4d8fe7..81552c7b46eb 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -113,7 +113,11 @@ KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
endif

cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
+ifdef CONFIG_PPC64
+cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r13
+else
cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
+endif

LDFLAGS_vmlinux-y := -Bstatic
LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
@@ -411,8 +415,12 @@ ifdef CONFIG_STACKPROTECTOR
prepare: stack_protector_prepare

stack_protector_prepare: prepare0
+ifdef CONFIG_PPC64
+ $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "PACA_CANARY") print $$3;}' include/generated/asm-offsets.h))
+else
$(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
endif
+endif

# Use the file '.tmp_gas_check' for binutils tests, as gas won't output
# to stdout and these checks are run even on install targets.
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 6d6b3706232c..98d883e58945 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -246,6 +246,9 @@ struct paca_struct {
struct slb_entry *mce_faulty_slbs;
u16 slb_save_cache_ptr;
#endif /* CONFIG_PPC_BOOK3S_64 */
+#ifdef CONFIG_STACKPROTECTOR
+ unsigned long canary;
+#endif
} ____cacheline_aligned;

extern struct paca_struct **paca_ptrs;
diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
index 263e2aab1862..e81991955c0d 100644
--- a/arch/powerpc/include/asm/stackprotector.h
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -29,6 +29,9 @@ static __always_inline void boot_init_stack_canary(void)
canary ^= LINUX_VERSION_CODE;

current->stack_canary = canary;
+#ifdef CONFIG_PPC64
+ get_paca()->canary = canary;
+#endif
}

#endif /* _ASM_STACKPROTECTOR_H */
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a992f7a53cf3..773dee55b3f6 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -81,6 +81,9 @@ int main(void)
OFFSET(MM, task_struct, mm);
#ifdef CONFIG_STACKPROTECTOR
OFFSET(TASK_CANARY, task_struct, stack_canary);
+#ifdef CONFIG_PPC64
+ OFFSET(PACA_CANARY, paca_struct, canary);
+#endif
#endif
OFFSET(MMCONTEXTID, mm_struct, context.id);
#ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 77a888bfcb53..573fa879d785 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -624,6 +624,10 @@ _GLOBAL(_switch)

addi r6,r4,-THREAD /* Convert THREAD to 'current' */
std r6,PACACURRENT(r13) /* Set new 'current' */
+#if defined(CONFIG_STACKPROTECTOR)
+ ld r6, TASK_CANARY(r6)
+ std r6, PACA_CANARY(r13)
+#endif

ld r8,KSP(r4) /* new stack pointer */
#ifdef CONFIG_PPC_BOOK3S_64
--
2.13.3


2018-09-25 15:56:38

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support

Snowpatch reports failure on pmac32_defconfig, as follows:

arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
/var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
undefined reference to `__stack_chk_fail_local'
arch/powerpc/platforms/powermac/bootx_init.o: In function
`bootx_add_display_props':
/var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:211:
undefined reference to `__stack_chk_fail_local'
arch/powerpc/platforms/powermac/bootx_init.o: In function
`bootx_scan_dt_build_struct':
/var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:350:
undefined reference to `__stack_chk_fail_local'
arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_init':
/var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:598:
undefined reference to `__stack_chk_fail_local'
ld: .tmp_vmlinux1: hidden symbol `__stack_chk_fail_local' isn't defined
ld: final link failed: Bad value
Makefile:1040: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

I don't have this issue with GCC 8.1, bootx_init.o has references to
__stack_chk_fail, not to __stack_chk_fail_local

Looking online, it may be due to an old build and a lack of 'make
clean', could it be the case here ?

Otherwise, can you retry the build to see if it's happen again ?

Christophe

Le 24/09/2018 à 17:15, Christophe Leroy a écrit :
> On PPC64, as register r13 points to the paca_struct at all time,
> this patch adds a copy of the canary there, which is copied at
> task_switch.
> That new canary is then used by using the following GCC options:
> -mstack-protector-guard=tls
> -mstack-protector-guard-reg=r13
> -mstack-protector-guard-offset=offsetof(struct paca_struct, canary))
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/Kconfig | 2 +-
> arch/powerpc/Makefile | 8 ++++++++
> arch/powerpc/include/asm/paca.h | 3 +++
> arch/powerpc/include/asm/stackprotector.h | 3 +++
> arch/powerpc/kernel/asm-offsets.c | 3 +++
> arch/powerpc/kernel/entry_64.S | 4 ++++
> 6 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 3bcb05929931..602eea723624 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -180,7 +180,7 @@ config PPC
> select HAVE_ARCH_SECCOMP_FILTER
> select HAVE_ARCH_TRACEHOOK
> select HAVE_CBPF_JIT if !PPC64
> - select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls) && PPC32
> + select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-guard=tls)
> select HAVE_CONTEXT_TRACKING if PPC64
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DEBUG_STACKOVERFLOW
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 45b8eb4d8fe7..81552c7b46eb 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -113,7 +113,11 @@ KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
> endif
>
> cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
> +ifdef CONFIG_PPC64
> +cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r13
> +else
> cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
> +endif
>
> LDFLAGS_vmlinux-y := -Bstatic
> LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
> @@ -411,8 +415,12 @@ ifdef CONFIG_STACKPROTECTOR
> prepare: stack_protector_prepare
>
> stack_protector_prepare: prepare0
> +ifdef CONFIG_PPC64
> + $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "PACA_CANARY") print $$3;}' include/generated/asm-offsets.h))
> +else
> $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
> endif
> +endif
>
> # Use the file '.tmp_gas_check' for binutils tests, as gas won't output
> # to stdout and these checks are run even on install targets.
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index 6d6b3706232c..98d883e58945 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -246,6 +246,9 @@ struct paca_struct {
> struct slb_entry *mce_faulty_slbs;
> u16 slb_save_cache_ptr;
> #endif /* CONFIG_PPC_BOOK3S_64 */
> +#ifdef CONFIG_STACKPROTECTOR
> + unsigned long canary;
> +#endif
> } ____cacheline_aligned;
>
> extern struct paca_struct **paca_ptrs;
> diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
> index 263e2aab1862..e81991955c0d 100644
> --- a/arch/powerpc/include/asm/stackprotector.h
> +++ b/arch/powerpc/include/asm/stackprotector.h
> @@ -29,6 +29,9 @@ static __always_inline void boot_init_stack_canary(void)
> canary ^= LINUX_VERSION_CODE;
>
> current->stack_canary = canary;
> +#ifdef CONFIG_PPC64
> + get_paca()->canary = canary;
> +#endif
> }
>
> #endif /* _ASM_STACKPROTECTOR_H */
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index a992f7a53cf3..773dee55b3f6 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -81,6 +81,9 @@ int main(void)
> OFFSET(MM, task_struct, mm);
> #ifdef CONFIG_STACKPROTECTOR
> OFFSET(TASK_CANARY, task_struct, stack_canary);
> +#ifdef CONFIG_PPC64
> + OFFSET(PACA_CANARY, paca_struct, canary);
> +#endif
> #endif
> OFFSET(MMCONTEXTID, mm_struct, context.id);
> #ifdef CONFIG_PPC64
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 77a888bfcb53..573fa879d785 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -624,6 +624,10 @@ _GLOBAL(_switch)
>
> addi r6,r4,-THREAD /* Convert THREAD to 'current' */
> std r6,PACACURRENT(r13) /* Set new 'current' */
> +#if defined(CONFIG_STACKPROTECTOR)
> + ld r6, TASK_CANARY(r6)
> + std r6, PACA_CANARY(r13)
> +#endif
>
> ld r8,KSP(r4) /* new stack pointer */
> #ifdef CONFIG_PPC_BOOK3S_64
>

2018-09-26 07:06:59

by Russell Currey

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support

On Tue, 2018-09-25 at 17:56 +0200, Christophe LEROY wrote:
> Snowpatch reports failure on pmac32_defconfig, as follows:
>
> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
> undefined reference to `__stack_chk_fail_local'
> arch/powerpc/platforms/powermac/bootx_init.o: In function
> `bootx_add_display_props':
> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:211:
> undefined reference to `__stack_chk_fail_local'
> arch/powerpc/platforms/powermac/bootx_init.o: In function
> `bootx_scan_dt_build_struct':
> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:350:
> undefined reference to `__stack_chk_fail_local'
> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_init':
> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:598:
> undefined reference to `__stack_chk_fail_local'
> ld: .tmp_vmlinux1: hidden symbol `__stack_chk_fail_local' isn't defined
> ld: final link failed: Bad value
> Makefile:1040: recipe for target 'vmlinux' failed
> make: *** [vmlinux] Error 1
>
> I don't have this issue with GCC 8.1, bootx_init.o has references to
> __stack_chk_fail, not to __stack_chk_fail_local
>
> Looking online, it may be due to an old build and a lack of 'make
> clean', could it be the case here ?

We do make mrproper AND clean, just to be sure :)

>
> Otherwise, can you retry the build to see if it's happen again ?

I've retried the build and the same thing happens. Our build environment is using
GCC 7.3 on Ubuntu 18.04. Reproduced on a different machine too, running the same
OS and compiler.

You can see exactly what gets run here:
https://github.com/ajdlinux/openpower.xyz-snowpatch/blob/master/kernel-build-sparse.sh

- Russell

>
> Christophe
>
> Le 24/09/2018 à 17:15, Christophe Leroy a écrit :
> > On PPC64, as register r13 points to the paca_struct at all time,
> > this patch adds a copy of the canary there, which is copied at
> > task_switch.
> > That new canary is then used by using the following GCC options:
> > -mstack-protector-guard=tls
> > -mstack-protector-guard-reg=r13
> > -mstack-protector-guard-offset=offsetof(struct paca_struct, canary))
> >
> > Signed-off-by: Christophe Leroy <[email protected]>
> > ---
> > arch/powerpc/Kconfig | 2 +-
> > arch/powerpc/Makefile | 8 ++++++++
> > arch/powerpc/include/asm/paca.h | 3 +++
> > arch/powerpc/include/asm/stackprotector.h | 3 +++
> > arch/powerpc/kernel/asm-offsets.c | 3 +++
> > arch/powerpc/kernel/entry_64.S | 4 ++++
> > 6 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 3bcb05929931..602eea723624 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -180,7 +180,7 @@ config PPC
> > select HAVE_ARCH_SECCOMP_FILTER
> > select HAVE_ARCH_TRACEHOOK
> > select HAVE_CBPF_JIT if !PPC64
> > - select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-
> > guard=tls) && PPC32
> > + select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-
> > guard=tls)
> > select HAVE_CONTEXT_TRACKING if PPC64
> > select HAVE_DEBUG_KMEMLEAK
> > select HAVE_DEBUG_STACKOVERFLOW
> > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> > index 45b8eb4d8fe7..81552c7b46eb 100644
> > --- a/arch/powerpc/Makefile
> > +++ b/arch/powerpc/Makefile
> > @@ -113,7 +113,11 @@ KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
> > endif
> >
> > cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
> > +ifdef CONFIG_PPC64
> > +cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r13
> > +else
> > cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
> > +endif
> >
> > LDFLAGS_vmlinux-y := -Bstatic
> > LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
> > @@ -411,8 +415,12 @@ ifdef CONFIG_STACKPROTECTOR
> > prepare: stack_protector_prepare
> >
> > stack_protector_prepare: prepare0
> > +ifdef CONFIG_PPC64
> > + $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if
> > ($$2 == "PACA_CANARY") print $$3;}' include/generated/asm-offsets.h))
> > +else
> > $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if
> > ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
> > endif
> > +endif
> >
> > # Use the file '.tmp_gas_check' for binutils tests, as gas won't output
> > # to stdout and these checks are run even on install targets.
> > diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> > index 6d6b3706232c..98d883e58945 100644
> > --- a/arch/powerpc/include/asm/paca.h
> > +++ b/arch/powerpc/include/asm/paca.h
> > @@ -246,6 +246,9 @@ struct paca_struct {
> > struct slb_entry *mce_faulty_slbs;
> > u16 slb_save_cache_ptr;
> > #endif /* CONFIG_PPC_BOOK3S_64 */
> > +#ifdef CONFIG_STACKPROTECTOR
> > + unsigned long canary;
> > +#endif
> > } ____cacheline_aligned;
> >
> > extern struct paca_struct **paca_ptrs;
> > diff --git a/arch/powerpc/include/asm/stackprotector.h
> > b/arch/powerpc/include/asm/stackprotector.h
> > index 263e2aab1862..e81991955c0d 100644
> > --- a/arch/powerpc/include/asm/stackprotector.h
> > +++ b/arch/powerpc/include/asm/stackprotector.h
> > @@ -29,6 +29,9 @@ static __always_inline void boot_init_stack_canary(void)
> > canary ^= LINUX_VERSION_CODE;
> >
> > current->stack_canary = canary;
> > +#ifdef CONFIG_PPC64
> > + get_paca()->canary = canary;
> > +#endif
> > }
> >
> > #endif /* _ASM_STACKPROTECTOR_H */
> > diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-
> > offsets.c
> > index a992f7a53cf3..773dee55b3f6 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -81,6 +81,9 @@ int main(void)
> > OFFSET(MM, task_struct, mm);
> > #ifdef CONFIG_STACKPROTECTOR
> > OFFSET(TASK_CANARY, task_struct, stack_canary);
> > +#ifdef CONFIG_PPC64
> > + OFFSET(PACA_CANARY, paca_struct, canary);
> > +#endif
> > #endif
> > OFFSET(MMCONTEXTID, mm_struct, context.id);
> > #ifdef CONFIG_PPC64
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 77a888bfcb53..573fa879d785 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -624,6 +624,10 @@ _GLOBAL(_switch)
> >
> > addi r6,r4,-THREAD /* Convert THREAD to 'current' */
> > std r6,PACACURRENT(r13) /* Set new 'current' */
> > +#if defined(CONFIG_STACKPROTECTOR)
> > + ld r6, TASK_CANARY(r6)
> > + std r6, PACA_CANARY(r13)
> > +#endif
> >
> > ld r8,KSP(r4) /* new stack pointer */
> > #ifdef CONFIG_PPC_BOOK3S_64
> >


2018-09-26 08:00:15

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support

Segher, any idea about this problem ?

Christophe

Le 26/09/2018 à 09:05, Russell Currey a écrit :
> On Tue, 2018-09-25 at 17:56 +0200, Christophe LEROY wrote:
>> Snowpatch reports failure on pmac32_defconfig, as follows:
>>
>> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
>> undefined reference to `__stack_chk_fail_local'
>> arch/powerpc/platforms/powermac/bootx_init.o: In function
>> `bootx_add_display_props':
>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:211:
>> undefined reference to `__stack_chk_fail_local'
>> arch/powerpc/platforms/powermac/bootx_init.o: In function
>> `bootx_scan_dt_build_struct':
>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:350:
>> undefined reference to `__stack_chk_fail_local'
>> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_init':
>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:598:
>> undefined reference to `__stack_chk_fail_local'
>> ld: .tmp_vmlinux1: hidden symbol `__stack_chk_fail_local' isn't defined
>> ld: final link failed: Bad value
>> Makefile:1040: recipe for target 'vmlinux' failed
>> make: *** [vmlinux] Error 1
>>
>> I don't have this issue with GCC 8.1, bootx_init.o has references to
>> __stack_chk_fail, not to __stack_chk_fail_local
>>
>> Looking online, it may be due to an old build and a lack of 'make
>> clean', could it be the case here ?
>
> We do make mrproper AND clean, just to be sure :)
>
>>
>> Otherwise, can you retry the build to see if it's happen again ?
>
> I've retried the build and the same thing happens. Our build environment is using
> GCC 7.3 on Ubuntu 18.04. Reproduced on a different machine too, running the same
> OS and compiler.
>
> You can see exactly what gets run here:
> https://github.com/ajdlinux/openpower.xyz-snowpatch/blob/master/kernel-build-sparse.sh
>
> - Russell
>
>>
>> Christophe
>>
>> Le 24/09/2018 à 17:15, Christophe Leroy a écrit :
>>> On PPC64, as register r13 points to the paca_struct at all time,
>>> this patch adds a copy of the canary there, which is copied at
>>> task_switch.
>>> That new canary is then used by using the following GCC options:
>>> -mstack-protector-guard=tls
>>> -mstack-protector-guard-reg=r13
>>> -mstack-protector-guard-offset=offsetof(struct paca_struct, canary))
>>>
>>> Signed-off-by: Christophe Leroy <[email protected]>
>>> ---
>>> arch/powerpc/Kconfig | 2 +-
>>> arch/powerpc/Makefile | 8 ++++++++
>>> arch/powerpc/include/asm/paca.h | 3 +++
>>> arch/powerpc/include/asm/stackprotector.h | 3 +++
>>> arch/powerpc/kernel/asm-offsets.c | 3 +++
>>> arch/powerpc/kernel/entry_64.S | 4 ++++
>>> 6 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index 3bcb05929931..602eea723624 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -180,7 +180,7 @@ config PPC
>>> select HAVE_ARCH_SECCOMP_FILTER
>>> select HAVE_ARCH_TRACEHOOK
>>> select HAVE_CBPF_JIT if !PPC64
>>> - select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-
>>> guard=tls) && PPC32
>>> + select HAVE_STACKPROTECTOR if $(cc-option,-mstack-protector-
>>> guard=tls)
>>> select HAVE_CONTEXT_TRACKING if PPC64
>>> select HAVE_DEBUG_KMEMLEAK
>>> select HAVE_DEBUG_STACKOVERFLOW
>>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>>> index 45b8eb4d8fe7..81552c7b46eb 100644
>>> --- a/arch/powerpc/Makefile
>>> +++ b/arch/powerpc/Makefile
>>> @@ -113,7 +113,11 @@ KBUILD_ARFLAGS += --target=elf$(BITS)-$(GNUTARGET)
>>> endif
>>>
>>> cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard=tls
>>> +ifdef CONFIG_PPC64
>>> +cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r13
>>> +else
>>> cflags-$(CONFIG_STACKPROTECTOR) += -mstack-protector-guard-reg=r2
>>> +endif
>>>
>>> LDFLAGS_vmlinux-y := -Bstatic
>>> LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
>>> @@ -411,8 +415,12 @@ ifdef CONFIG_STACKPROTECTOR
>>> prepare: stack_protector_prepare
>>>
>>> stack_protector_prepare: prepare0
>>> +ifdef CONFIG_PPC64
>>> + $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if
>>> ($$2 == "PACA_CANARY") print $$3;}' include/generated/asm-offsets.h))
>>> +else
>>> $(eval KBUILD_CFLAGS += -mstack-protector-guard-offset=$(shell awk '{if
>>> ($$2 == "TASK_CANARY") print $$3;}' include/generated/asm-offsets.h))
>>> endif
>>> +endif
>>>
>>> # Use the file '.tmp_gas_check' for binutils tests, as gas won't output
>>> # to stdout and these checks are run even on install targets.
>>> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
>>> index 6d6b3706232c..98d883e58945 100644
>>> --- a/arch/powerpc/include/asm/paca.h
>>> +++ b/arch/powerpc/include/asm/paca.h
>>> @@ -246,6 +246,9 @@ struct paca_struct {
>>> struct slb_entry *mce_faulty_slbs;
>>> u16 slb_save_cache_ptr;
>>> #endif /* CONFIG_PPC_BOOK3S_64 */
>>> +#ifdef CONFIG_STACKPROTECTOR
>>> + unsigned long canary;
>>> +#endif
>>> } ____cacheline_aligned;
>>>
>>> extern struct paca_struct **paca_ptrs;
>>> diff --git a/arch/powerpc/include/asm/stackprotector.h
>>> b/arch/powerpc/include/asm/stackprotector.h
>>> index 263e2aab1862..e81991955c0d 100644
>>> --- a/arch/powerpc/include/asm/stackprotector.h
>>> +++ b/arch/powerpc/include/asm/stackprotector.h
>>> @@ -29,6 +29,9 @@ static __always_inline void boot_init_stack_canary(void)
>>> canary ^= LINUX_VERSION_CODE;
>>>
>>> current->stack_canary = canary;
>>> +#ifdef CONFIG_PPC64
>>> + get_paca()->canary = canary;
>>> +#endif
>>> }
>>>
>>> #endif /* _ASM_STACKPROTECTOR_H */
>>> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-
>>> offsets.c
>>> index a992f7a53cf3..773dee55b3f6 100644
>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>> @@ -81,6 +81,9 @@ int main(void)
>>> OFFSET(MM, task_struct, mm);
>>> #ifdef CONFIG_STACKPROTECTOR
>>> OFFSET(TASK_CANARY, task_struct, stack_canary);
>>> +#ifdef CONFIG_PPC64
>>> + OFFSET(PACA_CANARY, paca_struct, canary);
>>> +#endif
>>> #endif
>>> OFFSET(MMCONTEXTID, mm_struct, context.id);
>>> #ifdef CONFIG_PPC64
>>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>>> index 77a888bfcb53..573fa879d785 100644
>>> --- a/arch/powerpc/kernel/entry_64.S
>>> +++ b/arch/powerpc/kernel/entry_64.S
>>> @@ -624,6 +624,10 @@ _GLOBAL(_switch)
>>>
>>> addi r6,r4,-THREAD /* Convert THREAD to 'current' */
>>> std r6,PACACURRENT(r13) /* Set new 'current' */
>>> +#if defined(CONFIG_STACKPROTECTOR)
>>> + ld r6, TASK_CANARY(r6)
>>> + std r6, PACA_CANARY(r13)
>>> +#endif
>>>
>>> ld r8,KSP(r4) /* new stack pointer */
>>> #ifdef CONFIG_PPC_BOOK3S_64
>>>

2018-09-26 09:30:56

by Segher Boessenkool

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support

On Wed, Sep 26, 2018 at 09:58:30AM +0200, Christophe LEROY wrote:
> Segher, any idea about this problem ?

> >>arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
> >>/var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
> >>sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
> >>undefined reference to `__stack_chk_fail_local'

Are you building as PIC? Are you linking libssp_nonshared.a? Why not?

:-)


Segher

2018-09-26 09:42:16

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support



Le 26/09/2018 à 11:28, Segher Boessenkool a écrit :
> On Wed, Sep 26, 2018 at 09:58:30AM +0200, Christophe LEROY wrote:
>> Segher, any idea about this problem ?
>
>>>> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
>>>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>>>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
>>>> undefined reference to `__stack_chk_fail_local'
>
> Are you building as PIC? Are you linking libssp_nonshared.a? Why not?

Spotted, thanks.

arch/powerpc/platforms/powermac/Makefile contains:

CFLAGS_bootx_init.o += -fPIC

Does it mean we should add -lssp_nonshared to LDFLAGS_vmlinux ?

Or maybe stack protection on bootx_init doesn't make much sense and we
could just do the following ?

CFLAGS_bootx_init.o += -fPIC $(call cc-option, -fno-stack-protector)

Christophe


>
> :-)
>
>
> Segher
>

2018-09-26 11:51:25

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support

Christophe LEROY <[email protected]> writes:
> Le 26/09/2018 à 11:28, Segher Boessenkool a écrit :
>> On Wed, Sep 26, 2018 at 09:58:30AM +0200, Christophe LEROY wrote:
>>> Segher, any idea about this problem ?
>>
>>>>> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
>>>>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>>>>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
>>>>> undefined reference to `__stack_chk_fail_local'
>>
>> Are you building as PIC? Are you linking libssp_nonshared.a? Why not?
>
> Spotted, thanks.
>
> arch/powerpc/platforms/powermac/Makefile contains:
>
> CFLAGS_bootx_init.o += -fPIC
...
>
> Or maybe stack protection on bootx_init doesn't make much sense and we
> could just do the following ?
>
> CFLAGS_bootx_init.o += -fPIC $(call cc-option, -fno-stack-protector)

That would be fine by me.

cheers

2018-09-26 11:55:43

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support



Le 26/09/2018 à 13:50, Michael Ellerman a écrit :
> Christophe LEROY <[email protected]> writes:
>> Le 26/09/2018 à 11:28, Segher Boessenkool a écrit :
>>> On Wed, Sep 26, 2018 at 09:58:30AM +0200, Christophe LEROY wrote:
>>>> Segher, any idea about this problem ?
>>>
>>>>>> arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
>>>>>> /var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
>>>>>> sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
>>>>>> undefined reference to `__stack_chk_fail_local'
>>>
>>> Are you building as PIC? Are you linking libssp_nonshared.a? Why not?
>>
>> Spotted, thanks.
>>
>> arch/powerpc/platforms/powermac/Makefile contains:
>>
>> CFLAGS_bootx_init.o += -fPIC
> ...
>>
>> Or maybe stack protection on bootx_init doesn't make much sense and we
>> could just do the following ?
>>
>> CFLAGS_bootx_init.o += -fPIC $(call cc-option, -fno-stack-protector)
>
> That would be fine by me.

Yes, that's what I did in v4, sent a few minutes ago. Indeed the same
was already done for prom_init, which is similar.

Christophe

2018-09-26 18:39:14

by Segher Boessenkool

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] powerpc/64: add stack protector support

On Wed, Sep 26, 2018 at 11:41:26AM +0200, Christophe LEROY wrote:
> Le 26/09/2018 ? 11:28, Segher Boessenkool a ?crit?:
> >On Wed, Sep 26, 2018 at 09:58:30AM +0200, Christophe LEROY wrote:
> >>Segher, any idea about this problem ?
> >
> >>>>arch/powerpc/platforms/powermac/bootx_init.o: In function
> >>>>`bootx_printf':
> >>>>/var/lib/jenkins-slave/workspace/snowpatch/snowpatch-linux-
> >>>>sparse/linux/arch/powerpc/platforms/powermac/bootx_init.c:88:
> >>>>undefined reference to `__stack_chk_fail_local'
> >
> >Are you building as PIC? Are you linking libssp_nonshared.a? Why not?
>
> Spotted, thanks.
>
> arch/powerpc/platforms/powermac/Makefile contains:
>
> CFLAGS_bootx_init.o += -fPIC
>
> Does it mean we should add -lssp_nonshared to LDFLAGS_vmlinux ?

Something like that.

> Or maybe stack protection on bootx_init doesn't make much sense and we
> could just do the following ?
>
> CFLAGS_bootx_init.o += -fPIC $(call cc-option,
> -fno-stack-protector)

But more likely this. Why does it use -fPIC?

If it is only boot-time init (which the name suggests) then disabling
ssp makes a lot of sense.


Segher

2018-09-30 02:17:30

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] powerpc/32: add stack 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-rc5 next-20180928]
[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-32-add-stack-protector-support/20180926-123625
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-mpc86xx_basic_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_add_display_props.isra.1':
>> bootx_init.c:(.init.text+0x504): 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+0x820): undefined reference to `__stack_chk_fail_local'
arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_init':
bootx_init.c:(.init.text+0xbbc): 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 infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation


Attachments:
(No filename) (1.69 kB)
.config.gz (8.22 kB)
Download all attachments