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
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
