[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[Laura Abbott]

arm64 currently lacks support for -fstack-protector. Add
similar functionality to arm to detect stack corruption.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm64/Kconfig                      |   12 +++++++++
 arch/arm64/Makefile                     |    4 +++
 arch/arm64/include/asm/stackprotector.h |   38 +++++++++++++++++++++++++++++++
 arch/arm64/kernel/process.c             |    9 +++++++
 4 files changed, 63 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm64/include/asm/stackprotector.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6d4dd22..4f86874 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -168,6 +168,18 @@ config HOTPLUG_CPU
 	  Say Y here to experiment with turning CPUs off and on.  CPUs
 	  can be controlled through /sys/devices/system/cpu.
 
+config CC_STACKPROTECTOR
+	bool "Enable -fstack-protector buffer overflow detection"
+	help
+	  This option turns on the -fstack-protector GCC feature. This
+	  feature puts, at the beginning of functions, a canary value on
+	  the stack just before the return address, and validates
+	  the value just before actually returning.  Stack based buffer
+	  overflows (that need to overwrite this return address) now also
+	  overwrite the canary, which gets detected and the attack is then
+	  neutralized via a kernel panic.
+	  This feature requires gcc version 4.2 or above.
+
 source kernel/Kconfig.preempt
 
 config HZ
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 2fceb71..1ce221e 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -48,6 +48,10 @@ core-$(CONFIG_XEN) += arch/arm64/xen/
 libs-y		:= arch/arm64/lib/ $(libs-y)
 libs-y		+= $(LIBGCC)
 
+ifeq ($(CONFIG_CC_STACKPROTECTOR),y)
+KBUILD_CFLAGS	+=-fstack-protector
+endif
+
 # Default target when executing plain make
 KBUILD_IMAGE	:= Image.gz
 KBUILD_DTBS	:= dtbs
diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
new file mode 100644
index 0000000..de00332
--- /dev/null
+++ b/arch/arm64/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 <linux/random.h>
+#include <linux/version.h>
+
+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/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index de17c89..592d630 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -50,6 +50,12 @@
 #include <asm/processor.h>
 #include <asm/stacktrace.h>
 
+#ifdef CONFIG_CC_STACKPROTECTOR
+#include <linux/stackprotector.h>
+unsigned long __stack_chk_guard __read_mostly;
+EXPORT_SYMBOL(__stack_chk_guard);
+#endif
+
 static void setup_restart(void)
 {
 	/*
@@ -288,6 +294,9 @@ struct task_struct *__switch_to(struct task_struct *prev,
 {
 	struct task_struct *last;
 
+#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
+	__stack_chk_guard = next->stack_canary;
+#endif
 	fpsimd_thread_switch(next);
 	tls_thread_switch(next);
 	hw_breakpoint_thread_switch(next);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[Will Deacon]

Hi Laura,

On Tue, Jan 21, 2014 at 05:26:06PM +0000, Laura Abbott wrote:
> arm64 currently lacks support for -fstack-protector. Add
> similar functionality to arm to detect stack corruption.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm64/Kconfig                      |   12 +++++++++
>  arch/arm64/Makefile                     |    4 +++
>  arch/arm64/include/asm/stackprotector.h |   38 +++++++++++++++++++++++++++++++
>  arch/arm64/kernel/process.c             |    9 +++++++
>  4 files changed, 63 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm64/include/asm/stackprotector.h
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6d4dd22..4f86874 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -168,6 +168,18 @@ config HOTPLUG_CPU
>  	  Say Y here to experiment with turning CPUs off and on.  CPUs
>  	  can be controlled through /sys/devices/system/cpu.
>  
> +config CC_STACKPROTECTOR
> +	bool "Enable -fstack-protector buffer overflow detection"
> +	help
> +	  This option turns on the -fstack-protector GCC feature. This
> +	  feature puts, at the beginning of functions, a canary value on
> +	  the stack just before the return address, and validates
> +	  the value just before actually returning.  Stack based buffer
> +	  overflows (that need to overwrite this return address) now also
> +	  overwrite the canary, which gets detected and the attack is then
> +	  neutralized via a kernel panic.
> +	  This feature requires gcc version 4.2 or above.

You can remove that bit about GCC -- GCC 4.2 doesn't support AArch64.

> +
>  source kernel/Kconfig.preempt
>  
>  config HZ
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 2fceb71..1ce221e 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -48,6 +48,10 @@ core-$(CONFIG_XEN) += arch/arm64/xen/
>  libs-y		:= arch/arm64/lib/ $(libs-y)
>  libs-y		+= $(LIBGCC)
>  
> +ifeq ($(CONFIG_CC_STACKPROTECTOR),y)
> +KBUILD_CFLAGS	+=-fstack-protector
> +endif
> +
>  # Default target when executing plain make
>  KBUILD_IMAGE	:= Image.gz
>  KBUILD_DTBS	:= dtbs
> diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
> new file mode 100644
> index 0000000..de00332
> --- /dev/null
> +++ b/arch/arm64/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

__ASM_ for consistency.

> +#define _ASM_STACKPROTECTOR_H 1

Why #define explicitly to 1?

> +
> +#include <linux/random.h>
> +#include <linux/version.h>
> +
> +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/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index de17c89..592d630 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -50,6 +50,12 @@
>  #include <asm/processor.h>
>  #include <asm/stacktrace.h>
>  
> +#ifdef CONFIG_CC_STACKPROTECTOR
> +#include <linux/stackprotector.h>
> +unsigned long __stack_chk_guard __read_mostly;
> +EXPORT_SYMBOL(__stack_chk_guard);
> +#endif
> +
>  static void setup_restart(void)
>  {
>  	/*
> @@ -288,6 +294,9 @@ struct task_struct *__switch_to(struct task_struct *prev,
>  {
>  	struct task_struct *last;
>  
> +#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
> +	__stack_chk_guard = next->stack_canary;
> +#endif

I don't get the dependency on !SMP. Assumedly, the update of
__stack_chk_guard would be racy otherwise, but that sounds solvable with
atomics. Is the stack_canary updated periodically somewhere else?

Will

[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[Laura Abbott]

On 1/22/2014 3:28 AM, Will Deacon wrote:
> Hi Laura,
>
> On Tue, Jan 21, 2014 at 05:26:06PM +0000, Laura Abbott wrote:
>> arm64 currently lacks support for -fstack-protector. Add
>> similar functionality to arm to detect stack corruption.
>>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>   arch/arm64/Kconfig                      |   12 +++++++++
>>   arch/arm64/Makefile                     |    4 +++
>>   arch/arm64/include/asm/stackprotector.h |   38 +++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/process.c             |    9 +++++++
>>   4 files changed, 63 insertions(+), 0 deletions(-)
>>   create mode 100644 arch/arm64/include/asm/stackprotector.h
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 6d4dd22..4f86874 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -168,6 +168,18 @@ config HOTPLUG_CPU
>>   	  Say Y here to experiment with turning CPUs off and on.  CPUs
>>   	  can be controlled through /sys/devices/system/cpu.
>>
>> +config CC_STACKPROTECTOR
>> +	bool "Enable -fstack-protector buffer overflow detection"
>> +	help
>> +	  This option turns on the -fstack-protector GCC feature. This
>> +	  feature puts, at the beginning of functions, a canary value on
>> +	  the stack just before the return address, and validates
>> +	  the value just before actually returning.  Stack based buffer
>> +	  overflows (that need to overwrite this return address) now also
>> +	  overwrite the canary, which gets detected and the attack is then
>> +	  neutralized via a kernel panic.
>> +	  This feature requires gcc version 4.2 or above.
>
> You can remove that bit about GCC -- GCC 4.2 doesn't support AArch64.
>

Yeah, copied and pasted from ARM

>> +
>>   source kernel/Kconfig.preempt
>>
>>   config HZ
>> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
>> index 2fceb71..1ce221e 100644
>> --- a/arch/arm64/Makefile
>> +++ b/arch/arm64/Makefile
>> @@ -48,6 +48,10 @@ core-$(CONFIG_XEN) += arch/arm64/xen/
>>   libs-y		:= arch/arm64/lib/ $(libs-y)
>>   libs-y		+= $(LIBGCC)
>>
>> +ifeq ($(CONFIG_CC_STACKPROTECTOR),y)
>> +KBUILD_CFLAGS	+=-fstack-protector
>> +endif
>> +
>>   # Default target when executing plain make
>>   KBUILD_IMAGE	:= Image.gz
>>   KBUILD_DTBS	:= dtbs
>> diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
>> new file mode 100644
>> index 0000000..de00332
>> --- /dev/null
>> +++ b/arch/arm64/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
>
> __ASM_ for consistency.
>
>> +#define _ASM_STACKPROTECTOR_H 1
>
> Why #define explicitly to 1?
>

Again, borrowed from ARM. I'll remove it.

>> +
>> +#include <linux/random.h>
>> +#include <linux/version.h>
>> +
>> +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/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index de17c89..592d630 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -50,6 +50,12 @@
>>   #include <asm/processor.h>
>>   #include <asm/stacktrace.h>
>>
>> +#ifdef CONFIG_CC_STACKPROTECTOR
>> +#include <linux/stackprotector.h>
>> +unsigned long __stack_chk_guard __read_mostly;
>> +EXPORT_SYMBOL(__stack_chk_guard);
>> +#endif
>> +
>>   static void setup_restart(void)
>>   {
>>   	/*
>> @@ -288,6 +294,9 @@ struct task_struct *__switch_to(struct task_struct *prev,
>>   {
>>   	struct task_struct *last;
>>
>> +#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
>> +	__stack_chk_guard = next->stack_canary;
>> +#endif
>
> I don't get the dependency on !SMP. Assumedly, the update of
> __stack_chk_guard would be racy otherwise, but that sounds solvable with
> atomics. Is the stack_canary updated periodically somewhere else?
>

It has nothing to do with atomics, it's the fact that __stack_chk_guard 
is a global variable and with SMP you can have n different processes 
running each with a different canary (see kernel/fork.c, 
dup_task_struct) . c.f the commit added by Nicolas Pitre:

commit df0698be14c6683606d5df2d83e3ae40f85ed0d9
Author: Nicolas Pitre <nico@fluxnic.net>
Date:   Mon Jun 7 21:50:33 2010 -0400

     ARM: stack protector: change the canary value per task

     A new random value for the canary is stored in the task struct whenever
     a new task is forked.  This is meant to allow for different canary 
values
     per task.  On ARM, GCC expects the canary value to be found in a global
     variable called __stack_chk_guard.  So this variable has to be updated
     with the value stored in the task struct whenever a task switch occurs.

     Because the variable GCC expects is global, this cannot work on SMP
     unfortunately.  So, on SMP, the same initial canary value is kept
     throughout, making this feature a bit less effective although it is 
still
     useful.

     One way to overcome this GCC limitation would be to locate the
     __stack_chk_guard variable into a memory page of its own for each CPU,
     and then use TLB locking to have each CPU see its own page at the same
     virtual address for each of them.

     Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>


> Will
>

Thanks,
Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[Nicolas Pitre]

On Wed, 22 Jan 2014, Laura Abbott wrote:
> On 1/22/2014 3:28 AM, Will Deacon wrote:
> > On Tue, Jan 21, 2014 at 05:26:06PM +0000, Laura Abbott wrote:
> > > @@ -288,6 +294,9 @@ struct task_struct *__switch_to(struct task_struct
> > > @@ *prev,
> > >   {
> > >    struct task_struct *last;
> > >
> > > +#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
> > > +	__stack_chk_guard = next->stack_canary;
> > > +#endif
> >
> > I don't get the dependency on !SMP. Assumedly, the update of
> > __stack_chk_guard would be racy otherwise, but that sounds solvable with
> > atomics. Is the stack_canary updated periodically somewhere else?
> >
> 
> It has nothing to do with atomics, it's the fact that __stack_chk_guard is a
> global variable and with SMP you can have n different processes running each
> with a different canary (see kernel/fork.c, dup_task_struct) . c.f the commit
> added by Nicolas Pitre:
> 
> commit df0698be14c6683606d5df2d83e3ae40f85ed0d9
> Author: Nicolas Pitre <nico@fluxnic.net>
> Date:   Mon Jun 7 21:50:33 2010 -0400
> 
>     ARM: stack protector: change the canary value per task
> 
>     A new random value for the canary is stored in the task struct whenever
>     a new task is forked.  This is meant to allow for different canary 
> values
>     per task.  On ARM, GCC expects the canary value to be found in a global
>     variable called __stack_chk_guard.  So this variable has to be updated
>     with the value stored in the task struct whenever a task switch occurs.
> 
>     Because the variable GCC expects is global, this cannot work on SMP
>     unfortunately.  So, on SMP, the same initial canary value is kept
>     throughout, making this feature a bit less effective although it is 
> still
>     useful.
> 
>     One way to overcome this GCC limitation would be to locate the
>     __stack_chk_guard variable into a memory page of its own for each CPU,
>     and then use TLB locking to have each CPU see its own page at the same
>     virtual address for each of them.
> 
>     Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Did gcc for Aarch64 replicate the same global variable arrangement?
That would be unfortunate...


Nicolas

[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[Laura Abbott]

On 1/23/2014 11:23 AM, Nicolas Pitre wrote:
> On Wed, 22 Jan 2014, Laura Abbott wrote:
>> On 1/22/2014 3:28 AM, Will Deacon wrote:
>>> On Tue, Jan 21, 2014 at 05:26:06PM +0000, Laura Abbott wrote:
>>>> @@ -288,6 +294,9 @@ struct task_struct *__switch_to(struct task_struct
>>>> @@ *prev,
>>>>    {
>>>>     struct task_struct *last;
>>>>
>>>> +#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
>>>> +	__stack_chk_guard = next->stack_canary;
>>>> +#endif
>>>
>>> I don't get the dependency on !SMP. Assumedly, the update of
>>> __stack_chk_guard would be racy otherwise, but that sounds solvable with
>>> atomics. Is the stack_canary updated periodically somewhere else?
>>>
>>
>> It has nothing to do with atomics, it's the fact that __stack_chk_guard is a
>> global variable and with SMP you can have n different processes running each
>> with a different canary (see kernel/fork.c, dup_task_struct) . c.f the commit
>> added by Nicolas Pitre:
>>
>> commit df0698be14c6683606d5df2d83e3ae40f85ed0d9
>> Author: Nicolas Pitre <nico@fluxnic.net>
>> Date:   Mon Jun 7 21:50:33 2010 -0400
>>
>>      ARM: stack protector: change the canary value per task
>>
>>      A new random value for the canary is stored in the task struct whenever
>>      a new task is forked.  This is meant to allow for different canary
>> values
>>      per task.  On ARM, GCC expects the canary value to be found in a global
>>      variable called __stack_chk_guard.  So this variable has to be updated
>>      with the value stored in the task struct whenever a task switch occurs.
>>
>>      Because the variable GCC expects is global, this cannot work on SMP
>>      unfortunately.  So, on SMP, the same initial canary value is kept
>>      throughout, making this feature a bit less effective although it is
>> still
>>      useful.
>>
>>      One way to overcome this GCC limitation would be to locate the
>>      __stack_chk_guard variable into a memory page of its own for each CPU,
>>      and then use TLB locking to have each CPU see its own page at the same
>>      virtual address for each of them.
>>
>>      Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
>
> Did gcc for Aarch64 replicate the same global variable arrangement?
> That would be unfortunate...
>

Based on my experiments they did unfortunately do so.

Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm64: Add CONFIG_CC_STACKPROTECTOR

[Will Deacon]

On Thu, Jan 23, 2014 at 07:32:18PM +0000, Laura Abbott wrote:
> On 1/23/2014 11:23 AM, Nicolas Pitre wrote:
> > Did gcc for Aarch64 replicate the same global variable arrangement?
> > That would be unfortunate...
> >
> 
> Based on my experiments they did unfortunately do so.

Yes, and speaking to the tools guys, it would require an new ABI to fix
this. Given that I don't expect to see any UP AArch64 cores, I'd say drop
the #ifndef SMP part from this patch.

Will