[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[Ard Biesheuvel]

This is a proof of concept implementation of per-task stack canaries for
arm64. The purpose is to reach agreement between the arm64 kernel and GCC
maintainers on how to implement support for this in the compiler.

What these patches show is that we can support per-task stack canaries
on arm64 using only very minor changes on the kernel side, i.e., all
that is needed is exposing the offset of stack_canary in task_struct
via an ELF symbol. With that in place, the compiler needs to emit the
following sequence when -fstack-protector-guard=tls is enabled

  movz    xN, :abs_g0:__stack_chk_guard_offset
  msr     xM, sp_el0
  ldr     xM, [xM, xN]

Note that this does not involve per-CPU variables, and so there are no
concurrency issues to be addressed. sp_el0 is the current task pointer,
whose value never changes from the POV of the task, even when migrating
to another CPU.

Patch #1 implements a GCC plugin that patches the sequence

  adrp    xN, __stack_chk_guard
  add     xN, Xn, :lo12:__stack_chk_guard

into

  mrs     xN, sp_el0
  add     xN, xN, :lo12:__stack_chk_guard_offset

which is a poor man's version of the movz/msr/ldr sequence above (and only
works for small model code), but is sufficient as a proof of concept.

Patch #2 exposes the __stack_chk_guard_offset symbol and wires up the plugin
(if enabled in Kconfig)

Again, the point is not to use GCC plugin based hacks, but to reach agreement
on how to proceed with this for GCC.

Comments welcome.

Ard Biesheuvel (2):
  gcc-plugins: add support plugin for arm64 per-task stack canaries
  arm64: kernel: use a unique stack canary value for each task

 arch/Kconfig                                    |   4 +
 arch/arm64/Kconfig                              |   7 ++
 arch/arm64/include/asm/stackprotector.h         |   4 +-
 arch/arm64/kernel/asm-offsets.c                 |   3 +
 arch/arm64/kernel/process.c                     |   4 +
 arch/arm64/kernel/vmlinux.lds.S                 |   8 ++
 scripts/Makefile.gcc-plugins                    |   2 +
 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121 ++++++++++++++++++++
 8 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c

-- 
2.11.0

[RFC/RFT PATCH 1/2] gcc-plugins: add support plugin for arm64 per-task stack canaries

[Ard Biesheuvel]

By default, GCC for AArch64 uses a global variable __stack_chk_guard
which is shared between all CPUs, and hence between all tasks, which
means the stack canary value only changes between reboots.

This plugin renames the symbol to __stack_chk_guard_tsk_offset, and
adds an RTL pass to replace the first instruction in each adrp/add pair
referring to the symbol with a load of sp_el0, which holds the address
of 'current'. In a subsequent patch, we will tweak the kernel build to
expose the offset of the stack_canary field in task_struct via the
symbol, so that the resulting reference resolves to the task's unique
canary value directly.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/Kconfig                                    |   4 +
 scripts/Makefile.gcc-plugins                    |   2 +
 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121 ++++++++++++++++++++
 3 files changed, 127 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index 400b9e1b2f27..e7f110a89a91 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -517,6 +517,10 @@ config GCC_PLUGIN_RANDSTRUCT_PERFORMANCE
 	  in structures.  This reduces the performance hit of RANDSTRUCT
 	  at the cost of weakened randomization.
 
+config GCC_PLUGIN_ARM64_SSP_PER_TASK
+	bool
+	depends on GCC_PLUGINS
+
 config HAVE_CC_STACKPROTECTOR
 	bool
 	help
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index b2a95af7df18..aa1ca74b3d70 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -35,6 +35,8 @@ ifdef CONFIG_GCC_PLUGINS
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_RANDSTRUCT)	+= -DRANDSTRUCT_PLUGIN
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_RANDSTRUCT_PERFORMANCE)	+= -fplugin-arg-randomize_layout_plugin-performance-mode
 
+  gcc-plugin-$(CONFIG_GCC_PLUGIN_ARM64_SSP_PER_TASK) += arm64_ssp_per_task_plugin.so
+
   GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
 
   export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
diff --git a/scripts/gcc-plugins/arm64_ssp_per_task_plugin.c b/scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
new file mode 100644
index 000000000000..611036a459c3
--- /dev/null
+++ b/scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2018 Ard Biesheuvel <ard.biesheuvel@linaro.org>
+ */
+
+#include "gcc-common.h"
+
+__visible int plugin_is_GPL_compatible;
+
+static GTY(()) tree stack_chk_guard_decl;
+
+static void init_stack_chk_guard_decl(void)
+{
+	tree t;
+	rtx x;
+
+	t = build_decl (UNKNOWN_LOCATION,
+			VAR_DECL,
+			get_identifier ("__stack_chk_guard_tsk_offset"),
+			ptr_type_node);
+	TREE_STATIC (t) = 1;
+	TREE_PUBLIC (t) = 1;
+	DECL_EXTERNAL (t) = 1;
+	TREE_USED (t) = 1;
+	TREE_THIS_VOLATILE (t) = 1;
+	DECL_ARTIFICIAL (t) = 1;
+	DECL_IGNORED_P (t) = 1;
+
+	/* Do not share RTL as the declaration is visible outside of
+	   current function.  */
+	x = DECL_RTL (t);
+	RTX_FLAG (x, used) = 1;
+
+	stack_chk_guard_decl = t;
+}
+
+static tree arm64_pertask_ssp_stack_protect_guard(void)
+{
+	if (stack_chk_guard_decl == NULL);
+		init_stack_chk_guard_decl();
+	return stack_chk_guard_decl;
+}
+
+static bool arm64_pertask_ssp_rtl_gate(void)
+{
+	return true;
+}
+
+static unsigned int arm64_pertask_ssp_rtl_execute(void)
+{
+	rtx_insn *insn, *next;
+	int regno;
+
+	for (insn = get_insns(); insn; insn = next) {
+		const char *sym;
+		rtx body;
+
+		//
+		// Find a SET insn involving a HIGH SYMBOL_REF to
+		// __stack_chk_guard_tsk_offset
+		//
+		next = NEXT_INSN(insn);
+		if (!INSN_P(insn))
+			continue;
+		body = PATTERN(insn);
+		if (GET_CODE(body) != SET ||
+		    GET_CODE(SET_SRC(body)) != HIGH ||
+		    GET_CODE(XEXP(SET_SRC (body), 0)) != SYMBOL_REF)
+			continue;
+		if (!REG_P(SET_DEST(body)))
+			continue;
+		sym = XSTR(XEXP(SET_SRC (body), 0), 0);
+		if (strcmp(sym, "__stack_chk_guard_tsk_offset"))
+			continue;
+		regno = REGNO(SET_DEST(body));
+
+		//
+		// We have found the ADRP assignment of the relative address
+		// of __stack_chk_guard_tsk_offset. Replace it with an asm
+		// expression returning the value of sp_el0 into the same
+		// register.
+		//
+		SET_SRC(body) = gen_rtx_ASM_OPERANDS(Pmode,
+						     "mrs %0, sp_el0",
+						     "=r",
+						     regno,
+						     rtvec_alloc (0),
+						     rtvec_alloc (0),
+						     rtvec_alloc (0),
+						     UNKNOWN_LOCATION);
+	}
+	return 0;
+}
+
+#define PASS_NAME arm64_pertask_ssp_rtl
+#define TODO_FLAGS_FINISH TODO_dump_func
+#include "gcc-generate-rtl-pass.h"
+
+static void arm64_pertask_ssp_start_unit(void *gcc_data, void *user_data)
+{
+	targetm.stack_protect_guard = arm64_pertask_ssp_stack_protect_guard;
+}
+
+__visible int plugin_init(struct plugin_name_args *plugin_info,
+			  struct plugin_gcc_version *version)
+{
+	if (!plugin_default_version_check(version, &gcc_version)) {
+		error(G_("incompatible gcc/plugin versions"));
+		return 1;
+	}
+
+	PASS_INFO(arm64_pertask_ssp_rtl, "final", 1, PASS_POS_INSERT_BEFORE);
+
+	register_callback(plugin_info->base_name, PLUGIN_START_UNIT,
+			  arm64_pertask_ssp_start_unit, NULL);
+
+	register_callback(plugin_info->base_name, PLUGIN_PASS_MANAGER_SETUP,
+			  NULL, &arm64_pertask_ssp_rtl_pass_info);
+
+	return 0;
+}
-- 
2.11.0

[RFC/RFT PATCH 2/2] arm64: kernel: use a unique stack canary value for each task

[Ard Biesheuvel]

Enable the support plugin and expose an appropriate value for
__stack_chk_guard_tsk_offset so that function prologues and
epilogues emitted by GCC read the stack canary value straight
from the task_struct. This sidesteps any concurrency issues
resulting from the use of per-CPU variables to store the canary
value of the currently running task.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/Kconfig                      | 7 +++++++
 arch/arm64/include/asm/stackprotector.h | 4 +++-
 arch/arm64/kernel/asm-offsets.c         | 3 +++
 arch/arm64/kernel/process.c             | 4 ++++
 arch/arm64/kernel/vmlinux.lds.S         | 8 ++++++++
 5 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index d1515fdf7d82..096f23ebfa02 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1083,6 +1083,13 @@ config RANDOMIZE_MODULE_REGION_FULL
 	  a limited range that contains the [_stext, _etext] interval of the
 	  core kernel, so branch relocations are always in range.
 
+config CC_STACK_PROTECTOR_PER_TASK
+	bool "Use a unique stack canary value for each task"
+	depends on GCC_PLUGINS
+	select GCC_PLUGIN_ARM64_SSP_PER_TASK
+	help
+	  Use a unique value for the stack canary value for each task.
+
 endmenu
 
 menu "Boot options"
diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
index 58d15be11c4d..fc17a66ec400 100644
--- a/arch/arm64/include/asm/stackprotector.h
+++ b/arch/arm64/include/asm/stackprotector.h
@@ -17,6 +17,7 @@
 #include <linux/version.h>
 
 extern unsigned long __stack_chk_guard;
+extern unsigned long __stack_chk_guard_tsk_offset;
 
 /*
  * Initialize the stackprotector canary value.
@@ -34,7 +35,8 @@ static __always_inline void boot_init_stack_canary(void)
 	canary &= CANARY_MASK;
 
 	current->stack_canary = canary;
-	__stack_chk_guard = current->stack_canary;
+	if (!IS_ENABLED(CONFIG_CC_STACK_PROTECTOR_PER_TASK))
+		__stack_chk_guard = current->stack_canary;
 }
 
 #endif	/* _ASM_STACKPROTECTOR_H */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 71bf088f1e4b..ef1ff04ec064 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -43,6 +43,9 @@ int main(void)
   DEFINE(TSK_TI_TTBR0,		offsetof(struct task_struct, thread_info.ttbr0));
 #endif
   DEFINE(TSK_STACK,		offsetof(struct task_struct, stack));
+#ifdef CONFIG_CC_STACK_PROTECTOR_PER_TASK
+  DEFINE(TSK_STACK_CANARY,	offsetof(struct task_struct, stack_canary));
+#endif
   BLANK();
   DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
   BLANK();
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6b7dcf4310ac..d9fd04748f95 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -62,8 +62,12 @@
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 #include <linux/stackprotector.h>
+#ifndef CONFIG_CC_STACK_PROTECTOR_PER_TASK
 unsigned long __stack_chk_guard __read_mostly;
 EXPORT_SYMBOL(__stack_chk_guard);
+#else
+EXPORT_SYMBOL(__stack_chk_guard_tsk_offset);
+#endif
 #endif
 
 /*
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 7da3e5c366a0..633cfc1f940c 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -6,6 +6,7 @@
  */
 
 #include <asm-generic/vmlinux.lds.h>
+#include <asm/asm-offsets.h>
 #include <asm/cache.h>
 #include <asm/kernel-pgtable.h>
 #include <asm/thread_info.h>
@@ -239,3 +240,10 @@ ASSERT(__hibernate_exit_text_end - (__hibernate_exit_text_start & ~(SZ_4K - 1))
  * If padding is applied before .head.text, virt<->phys conversions will fail.
  */
 ASSERT(_text == (KIMAGE_VADDR + TEXT_OFFSET), "HEAD is misaligned")
+
+#ifdef CONFIG_CC_STACK_PROTECTOR_PER_TASK
+PROVIDE(__stack_chk_guard_tsk_offset = ABSOLUTE(TSK_STACK_CANARY));
+
+ASSERT(__stack_chk_guard_tsk_offset < 0x1000,
+       "__stack_chk_guard_tsk_offset out of range")
+#endif
-- 
2.11.0

[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[Kees Cook]

On Wed, Jan 24, 2018 at 12:03 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> This is a proof of concept implementation of per-task stack canaries for
> arm64. The purpose is to reach agreement between the arm64 kernel and GCC
> maintainers on how to implement support for this in the compiler.
>
> What these patches show is that we can support per-task stack canaries
> on arm64 using only very minor changes on the kernel side, i.e., all
> that is needed is exposing the offset of stack_canary in task_struct
> via an ELF symbol. With that in place, the compiler needs to emit the
> following sequence when -fstack-protector-guard=tls is enabled
>
>   movz    xN, :abs_g0:__stack_chk_guard_offset
>   msr     xM, sp_el0
>   ldr     xM, [xM, xN]
>
> Note that this does not involve per-CPU variables, and so there are no
> concurrency issues to be addressed. sp_el0 is the current task pointer,
> whose value never changes from the POV of the task, even when migrating
> to another CPU.
>
> Patch #1 implements a GCC plugin that patches the sequence
>
>   adrp    xN, __stack_chk_guard
>   add     xN, Xn, :lo12:__stack_chk_guard
>
> into
>
>   mrs     xN, sp_el0
>   add     xN, xN, :lo12:__stack_chk_guard_offset
>
> which is a poor man's version of the movz/msr/ldr sequence above (and only
> works for small model code), but is sufficient as a proof of concept.
>
> Patch #2 exposes the __stack_chk_guard_offset symbol and wires up the plugin
> (if enabled in Kconfig)
>
> Again, the point is not to use GCC plugin based hacks, but to reach agreement
> on how to proceed with this for GCC.
>
> Comments welcome.

This is great! I can confirm I'm seeing changing canaries on the
stack, they match current->stack_canary, and my system still panics
when I stomp on them in memory. :)

-Kees

>
> Ard Biesheuvel (2):
>   gcc-plugins: add support plugin for arm64 per-task stack canaries
>   arm64: kernel: use a unique stack canary value for each task
>
>  arch/Kconfig                                    |   4 +
>  arch/arm64/Kconfig                              |   7 ++
>  arch/arm64/include/asm/stackprotector.h         |   4 +-
>  arch/arm64/kernel/asm-offsets.c                 |   3 +
>  arch/arm64/kernel/process.c                     |   4 +
>  arch/arm64/kernel/vmlinux.lds.S                 |   8 ++
>  scripts/Makefile.gcc-plugins                    |   2 +
>  scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121 ++++++++++++++++++++
>  8 files changed, 152 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
>
> --
> 2.11.0
>



-- 
Kees Cook
Pixel Security

[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[Laura Abbott]

On 01/23/2018 05:03 AM, Ard Biesheuvel wrote:
> This is a proof of concept implementation of per-task stack canaries for
> arm64. The purpose is to reach agreement between the arm64 kernel and GCC
> maintainers on how to implement support for this in the compiler.
> 
> What these patches show is that we can support per-task stack canaries
> on arm64 using only very minor changes on the kernel side, i.e., all
> that is needed is exposing the offset of stack_canary in task_struct
> via an ELF symbol. With that in place, the compiler needs to emit the
> following sequence when -fstack-protector-guard=tls is enabled
> 
>    movz    xN, :abs_g0:__stack_chk_guard_offset
>    msr     xM, sp_el0
>    ldr     xM, [xM, xN]
> 
> Note that this does not involve per-CPU variables, and so there are no
> concurrency issues to be addressed. sp_el0 is the current task pointer,
> whose value never changes from the POV of the task, even when migrating
> to another CPU.
> 
> Patch #1 implements a GCC plugin that patches the sequence
> 
>    adrp    xN, __stack_chk_guard
>    add     xN, Xn, :lo12:__stack_chk_guard
> 
> into
> 
>    mrs     xN, sp_el0
>    add     xN, xN, :lo12:__stack_chk_guard_offset
> 
> which is a poor man's version of the movz/msr/ldr sequence above (and only
> works for small model code), but is sufficient as a proof of concept.
> 
> Patch #2 exposes the __stack_chk_guard_offset symbol and wires up the plugin
> (if enabled in Kconfig)
> 
> Again, the point is not to use GCC plugin based hacks, but to reach agreement
> on how to proceed with this for GCC.
> 
> Comments welcome.
> 

I was seeing some crashes with these when I tried to boot up on my
full Fedora system. It looked like a compiler bug with grabbing
the wrong literal but I don't think it's worth looking at it since
it's probably just something with the plugin which isn't the real
focus here. I can send along the crash if you are interested.

It looked good to me otherwise.

> Ard Biesheuvel (2):
>    gcc-plugins: add support plugin for arm64 per-task stack canaries
>    arm64: kernel: use a unique stack canary value for each task
> 
>   arch/Kconfig                                    |   4 +
>   arch/arm64/Kconfig                              |   7 ++
>   arch/arm64/include/asm/stackprotector.h         |   4 +-
>   arch/arm64/kernel/asm-offsets.c                 |   3 +
>   arch/arm64/kernel/process.c                     |   4 +
>   arch/arm64/kernel/vmlinux.lds.S                 |   8 ++
>   scripts/Makefile.gcc-plugins                    |   2 +
>   scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121 ++++++++++++++++++++
>   8 files changed, 152 insertions(+), 1 deletion(-)
>   create mode 100644 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
> 

[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[Ard Biesheuvel]

On 9 February 2018 at 20:20, Laura Abbott <labbott@redhat.com> wrote:
> On 01/23/2018 05:03 AM, Ard Biesheuvel wrote:
>>
>> This is a proof of concept implementation of per-task stack canaries for
>> arm64. The purpose is to reach agreement between the arm64 kernel and GCC
>> maintainers on how to implement support for this in the compiler.
>>
>> What these patches show is that we can support per-task stack canaries
>> on arm64 using only very minor changes on the kernel side, i.e., all
>> that is needed is exposing the offset of stack_canary in task_struct
>> via an ELF symbol. With that in place, the compiler needs to emit the
>> following sequence when -fstack-protector-guard=tls is enabled
>>
>>    movz    xN, :abs_g0:__stack_chk_guard_offset
>>    msr     xM, sp_el0
>>    ldr     xM, [xM, xN]
>>
>> Note that this does not involve per-CPU variables, and so there are no
>> concurrency issues to be addressed. sp_el0 is the current task pointer,
>> whose value never changes from the POV of the task, even when migrating
>> to another CPU.
>>
>> Patch #1 implements a GCC plugin that patches the sequence
>>
>>    adrp    xN, __stack_chk_guard
>>    add     xN, Xn, :lo12:__stack_chk_guard
>>
>> into
>>
>>    mrs     xN, sp_el0
>>    add     xN, xN, :lo12:__stack_chk_guard_offset
>>
>> which is a poor man's version of the movz/msr/ldr sequence above (and only
>> works for small model code), but is sufficient as a proof of concept.
>>
>> Patch #2 exposes the __stack_chk_guard_offset symbol and wires up the
>> plugin
>> (if enabled in Kconfig)
>>
>> Again, the point is not to use GCC plugin based hacks, but to reach
>> agreement
>> on how to proceed with this for GCC.
>>
>> Comments welcome.
>>
>
> I was seeing some crashes with these when I tried to boot up on my
> full Fedora system. It looked like a compiler bug with grabbing
> the wrong literal but I don't think it's worth looking at it since
> it's probably just something with the plugin which isn't the real
> focus here. I can send along the crash if you are interested.
>

Yes please. Did it crash in a modules? Did you build with
KASLR/erratum 843419 disabled?


> It looked good to me otherwise.
>

Thanks. I intend to try and restart the discussion on this topic this week.

>
>> Ard Biesheuvel (2):
>>    gcc-plugins: add support plugin for arm64 per-task stack canaries
>>    arm64: kernel: use a unique stack canary value for each task
>>
>>   arch/Kconfig                                    |   4 +
>>   arch/arm64/Kconfig                              |   7 ++
>>   arch/arm64/include/asm/stackprotector.h         |   4 +-
>>   arch/arm64/kernel/asm-offsets.c                 |   3 +
>>   arch/arm64/kernel/process.c                     |   4 +
>>   arch/arm64/kernel/vmlinux.lds.S                 |   8 ++
>>   scripts/Makefile.gcc-plugins                    |   2 +
>>   scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121
>> ++++++++++++++++++++
>>   8 files changed, 152 insertions(+), 1 deletion(-)
>>   create mode 100644 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
>>
>

[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[Laura Abbott]

On 02/12/2018 02:49 AM, Ard Biesheuvel wrote:
> On 9 February 2018 at 20:20, Laura Abbott <labbott@redhat.com> wrote:
>> On 01/23/2018 05:03 AM, Ard Biesheuvel wrote:
>>>
>>> This is a proof of concept implementation of per-task stack canaries for
>>> arm64. The purpose is to reach agreement between the arm64 kernel and GCC
>>> maintainers on how to implement support for this in the compiler.
>>>
>>> What these patches show is that we can support per-task stack canaries
>>> on arm64 using only very minor changes on the kernel side, i.e., all
>>> that is needed is exposing the offset of stack_canary in task_struct
>>> via an ELF symbol. With that in place, the compiler needs to emit the
>>> following sequence when -fstack-protector-guard=tls is enabled
>>>
>>>     movz    xN, :abs_g0:__stack_chk_guard_offset
>>>     msr     xM, sp_el0
>>>     ldr     xM, [xM, xN]
>>>
>>> Note that this does not involve per-CPU variables, and so there are no
>>> concurrency issues to be addressed. sp_el0 is the current task pointer,
>>> whose value never changes from the POV of the task, even when migrating
>>> to another CPU.
>>>
>>> Patch #1 implements a GCC plugin that patches the sequence
>>>
>>>     adrp    xN, __stack_chk_guard
>>>     add     xN, Xn, :lo12:__stack_chk_guard
>>>
>>> into
>>>
>>>     mrs     xN, sp_el0
>>>     add     xN, xN, :lo12:__stack_chk_guard_offset
>>>
>>> which is a poor man's version of the movz/msr/ldr sequence above (and only
>>> works for small model code), but is sufficient as a proof of concept.
>>>
>>> Patch #2 exposes the __stack_chk_guard_offset symbol and wires up the
>>> plugin
>>> (if enabled in Kconfig)
>>>
>>> Again, the point is not to use GCC plugin based hacks, but to reach
>>> agreement
>>> on how to proceed with this for GCC.
>>>
>>> Comments welcome.
>>>
>>
>> I was seeing some crashes with these when I tried to boot up on my
>> full Fedora system. It looked like a compiler bug with grabbing
>> the wrong literal but I don't think it's worth looking at it since
>> it's probably just something with the plugin which isn't the real
>> focus here. I can send along the crash if you are interested.
>>
> 
> Yes please. Did it crash in a modules? Did you build with
> KASLR/erratum 843419 disabled?
> 

Turning off erratum 843419 makes it work. The crashes were in
modules:

[    5.105030] Internal error: Accessing user space memory outside uaccess.hP
[    5.115500] Modules linked in: sdhci_of_arasan(+) sdhci_pltfm i2s
[    5.127261] CPU: 5 PID: 336 Comm: systemd-udevd Not tainted 4.15.0-canary+ #6
[    5.135909] Hardware name: AppliedMicro X-Gene Mustang Board/X-Gene M6
[    5.146981] pstate: 20400005 (nzCv daif +PAN -UAO)
[    5.153130] pc : sdhci_arasan_probe+0x30/0x638 [sdhci_of_ara]
[    5.160482] lr : platform_drv_probe+0x60/0xc0
[    5.166190] sp : ffff00000c17b970
[    5.170866] x29: ffff00000c17b970 x28: ffff000008ae48a8
[    5.177530] x27: ffff000000b5f298 x26: ffff000000b5f108
[    5.184194] x25: ffff000000b5f280 x24: ffff8003fffe6ea8
[    5.190857] x23: ffff000000b5f028 x22: 0000000000000000
[    5.197002] x21: 0000000000000590 x20: ffff8003ec98e010
[    5.202288] x19: ffff8003ec98e000 x18: 0000000000000001
[    5.207573] x17: 0000000000000004 x16: 0000000000000000
[    5.212859] x15: ffffffffffffffff x14: ffff8003e3e85f90
[    5.218145] x13: ffff8003937afb0a x12: 0000000000000030
[    5.223430] x11: 0000000000000003 x10: 0101010101010101
[    5.228716] x9 : fffffffffffffffa x8 : 7f7f7f7f7f7f7f7f
[    5.234000] x7 : fefefeff646c606d x6 : 1e0e1a00f2ade4ef
[    5.239286] x5 : 6f642d72001a0e1e x4 : 8080808000000000
[    5.244572] x3 : 837bbb2b916b2378 x2 : 82962d9749f72000
[    5.249856] x1 : ffff000000b5e850 x0 : ffff000008726598
[    5.255142] Process systemd-udevd (pid: 336, stack limit = 0x00000000d88c80b)
[    5.262329] Call trace:
[    5.264765]  sdhci_arasan_probe+0x30/0x638 [sdhci_of_arasan]
[    5.270395]  platform_drv_probe+0x60/0xc0
[    5.274384]  driver_probe_device+0x2a0/0x468
[    5.278632]  __driver_attach+0x124/0x128
[    5.282534]  bus_for_each_dev+0x84/0xd8
[    5.286349]  driver_attach+0x30/0x40
[    5.289906]  bus_add_driver+0x26c/0x298
[    5.293721]  driver_register+0x6c/0x110
[    5.297537]  __platform_driver_register+0x54/0x60
[    5.302218]  sdhci_arasan_driver_init+0x1c/0x1000 [sdhci_of_arasan]
[    5.308455]  do_one_initcall+0x58/0x160
[    5.312271]  do_init_module+0x60/0x1f0
[    5.316001]  load_module+0x13fc/0x1728
[    5.319730]  SyS_finit_module+0xfc/0x118
[    5.323632]  __sys_trace_return+0x0/0x4
[    5.327449] Code: d503201f 58002ae1 58002a55 f9413e78 (f94002a0)
[    5.333513] ---[ end trace aabd65049cbf2f2e ]---

0000000000000840 <sdhci_arasan_probe>:
  840:   a9b67bfd        stp     x29, x30, [sp, #-160]!
  844:   910003fd        mov     x29, sp
  848:   a90153f3        stp     x19, x20, [sp, #16]
  84c:   f90013f5        str     x21, [sp, #32]
  850:   a90363f7        stp     x23, x24, [sp, #48]
  854:   f90023f9        str     x25, [sp, #64]
  858:   aa0003f3        mov     x19, x0
  85c:   aa1e03e0        mov     x0, x30
  860:   94000000        bl      0 <_mcount>
  864:   58002ae1        ldr     x1, dc0 <sdhci_arasan_probe+0x580>
  868:   58002a55        ldr     x21, db0 <sdhci_arasan_probe+0x570> <---- This looks like garbage
  86c:   f9413e78        ldr     x24, [x19, #632]
  870:   f94002a0        ldr     x0, [x21]

There was another crash in xfs:

[    6.273348] Internal error: Accessing user space memory outside uaccess.h roP
[    6.282530] Modules linked in: xfs libcrc32c sdhci_of_arasan(+) sdhci_pltfm s
[    6.294138] CPU: 1 PID: 467 Comm: mount Tainted: G      D          4.15.0-ca6
[    6.302016] Hardware name: AppliedMicro X-Gene Mustang Board/X-Gene Mustang 6
[    6.311710] pstate: 40400005 (nZcv daif +PAN -UAO)
[    6.316666] pc : xfs_parseargs+0x2c/0x668 [xfs]
[    6.321330] lr : xfs_fs_fill_super+0x120/0x550 [xfs]
[    6.326269] sp : ffff00000c82bba0
[    6.329566] x29: ffff00000c82bba0 x28: ffff800393869e00
[    6.334852] x27: ffff000008ad1000 x26: 0000000fffffffe0
[    6.340138] x25: 0000000000000590 x24: 0000000000000000
[    6.345424] x23: ffff000000c5df68 x22: ffff00000819cef8
[    6.350709] x21: ffff8003929b1000 x20: ffff800393431000
[    6.355995] x19: ffff8003929b1000 x18: 0000000000000009
[    6.361280] x17: 0000000000000000 x16: 0000000000000000
[    6.366566] x15: ffffffffffffffff x14: ffffffffff000000
[    6.371851] x13: ffffffffffffffff x12: 0000000000000018
[    6.377136] x11: 0101010101010101 x10: ffff00000c82bcb0
[    6.382421] x9 : 0000000000000000 x8 : ffff800393432000
[    6.387706] x7 : 0000000000000000 x6 : ffff000000bdfaf8
[    6.392992] x5 : ffff000000be0670 x4 : 0000000000000000
[    6.398277] x3 : 0000000000000000 x2 : 00000000000000c0
[    6.403562] x1 : 0000000000000000 x0 : ffff000000bf71f8
[    6.408850] Process mount (pid: 467, stack limit = 0x00000000cda0c16d)
[    6.415344] Call trace:
[    6.417933]  xfs_parseargs+0x2c/0x668 [xfs]
[    6.422256]  xfs_fs_fill_super+0x120/0x550 [xfs]
[    6.426854]  mount_bdev+0x1a0/0x1d0
[    6.430477]  xfs_fs_mount+0x40/0x58 [xfs]
[    6.434468]  mount_fs+0x5c/0x190
[    6.437680]  vfs_kern_mount.part.9+0x54/0x178
[    6.442014]  do_mount+0x1f8/0xc68
[    6.445312]  SyS_mount+0xfc/0x118
[    6.448612]  __sys_trace_return+0x0/0x4
[    6.452431] Code: d503201f 58002ed9 52801802 f9400293 (f9400320)
[    6.458496] ---[ end trace aabd65049cbf2f2f ]---

0000000000088a00 <xfs_parseargs>:
    88a00:       a9b47bfd        stp     x29, x30, [sp, #-192]!
    88a04:       910003fd        mov     x29, sp
    88a08:       a90153f3        stp     x19, x20, [sp, #16]
    88a0c:       f90023f9        str     x25, [sp, #64]
    88a10:       aa0003f4        mov     x20, x0
    88a14:       aa1e03e0        mov     x0, x30
    88a18:       f90037e1        str     x1, [sp, #104]
    88a1c:       94000000        bl      0 <_mcount>
    88a20:       58002ed9        ldr     x25, 88ff8 <xfs_parseargs+0x5f8> <---- also looks bogus
    88a24:       52801802        mov     w2, #0xc0                       // #192
    88a28:       f9400293        ldr     x19, [x20]
    88a2c:       f9400320        ldr     x0, [x25]
...
    88fec:       a90363f7        stp     x23, x24, [sp, #48]
    88ff0:       a904effa        stp     x26, x27, [sp, #72]
    88ff4:       94000000        bl      0 <__stack_chk_fail>
         ...


> 
>> It looked good to me otherwise.
>>
> 
> Thanks. I intend to try and restart the discussion on this topic this week.
> 
>>
>>> Ard Biesheuvel (2):
>>>     gcc-plugins: add support plugin for arm64 per-task stack canaries
>>>     arm64: kernel: use a unique stack canary value for each task
>>>
>>>    arch/Kconfig                                    |   4 +
>>>    arch/arm64/Kconfig                              |   7 ++
>>>    arch/arm64/include/asm/stackprotector.h         |   4 +-
>>>    arch/arm64/kernel/asm-offsets.c                 |   3 +
>>>    arch/arm64/kernel/process.c                     |   4 +
>>>    arch/arm64/kernel/vmlinux.lds.S                 |   8 ++
>>>    scripts/Makefile.gcc-plugins                    |   2 +
>>>    scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121
>>> ++++++++++++++++++++
>>>    8 files changed, 152 insertions(+), 1 deletion(-)
>>>    create mode 100644 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
>>>
>>

[RFC/RFT PATCH 0/2] arm64: per-task stack canaries

[Ard Biesheuvel]

On 13 February 2018 at 18:32, Laura Abbott <labbott@redhat.com> wrote:
> On 02/12/2018 02:49 AM, Ard Biesheuvel wrote:
>>
>> On 9 February 2018 at 20:20, Laura Abbott <labbott@redhat.com> wrote:
>>>
>>> On 01/23/2018 05:03 AM, Ard Biesheuvel wrote:
>>>>
>>>>
>>>> This is a proof of concept implementation of per-task stack canaries for
>>>> arm64. The purpose is to reach agreement between the arm64 kernel and
>>>> GCC
>>>> maintainers on how to implement support for this in the compiler.
>>>>
>>>> What these patches show is that we can support per-task stack canaries
>>>> on arm64 using only very minor changes on the kernel side, i.e., all
>>>> that is needed is exposing the offset of stack_canary in task_struct
>>>> via an ELF symbol. With that in place, the compiler needs to emit the
>>>> following sequence when -fstack-protector-guard=tls is enabled
>>>>
>>>>     movz    xN, :abs_g0:__stack_chk_guard_offset
>>>>     msr     xM, sp_el0
>>>>     ldr     xM, [xM, xN]
>>>>
>>>> Note that this does not involve per-CPU variables, and so there are no
>>>> concurrency issues to be addressed. sp_el0 is the current task pointer,
>>>> whose value never changes from the POV of the task, even when migrating
>>>> to another CPU.
>>>>
>>>> Patch #1 implements a GCC plugin that patches the sequence
>>>>
>>>>     adrp    xN, __stack_chk_guard
>>>>     add     xN, Xn, :lo12:__stack_chk_guard
>>>>
>>>> into
>>>>
>>>>     mrs     xN, sp_el0
>>>>     add     xN, xN, :lo12:__stack_chk_guard_offset
>>>>
>>>> which is a poor man's version of the movz/msr/ldr sequence above (and
>>>> only
>>>> works for small model code), but is sufficient as a proof of concept.
>>>>
>>>> Patch #2 exposes the __stack_chk_guard_offset symbol and wires up the
>>>> plugin
>>>> (if enabled in Kconfig)
>>>>
>>>> Again, the point is not to use GCC plugin based hacks, but to reach
>>>> agreement
>>>> on how to proceed with this for GCC.
>>>>
>>>> Comments welcome.
>>>>
>>>
>>> I was seeing some crashes with these when I tried to boot up on my
>>> full Fedora system. It looked like a compiler bug with grabbing
>>> the wrong literal but I don't think it's worth looking at it since
>>> it's probably just something with the plugin which isn't the real
>>> focus here. I can send along the crash if you are interested.
>>>
>>
>> Yes please. Did it crash in a modules? Did you build with
>> KASLR/erratum 843419 disabled?
>>
>
> Turning off erratum 843419 makes it work. The crashes were in
> modules:
>

Thanks for going back and testing that.

This is expected: the plugin only deals with small model code, and
currently, we use large model code in modules if either the erratum or
KASLR are enabled.

Nobody is thrilled with the idea of using such a plugin as the real
solution to this problem, so as you said, the focus is elsewhere. But
still useful to understand the nature of the crash.

Thanks,
Ard.

> [    5.105030] Internal error: Accessing user space memory outside
> uaccess.hP
> [    5.115500] Modules linked in: sdhci_of_arasan(+) sdhci_pltfm i2s
> [    5.127261] CPU: 5 PID: 336 Comm: systemd-udevd Not tainted
> 4.15.0-canary+ #6
> [    5.135909] Hardware name: AppliedMicro X-Gene Mustang Board/X-Gene M6
> [    5.146981] pstate: 20400005 (nzCv daif +PAN -UAO)
> [    5.153130] pc : sdhci_arasan_probe+0x30/0x638 [sdhci_of_ara]
> [    5.160482] lr : platform_drv_probe+0x60/0xc0
> [    5.166190] sp : ffff00000c17b970
> [    5.170866] x29: ffff00000c17b970 x28: ffff000008ae48a8
> [    5.177530] x27: ffff000000b5f298 x26: ffff000000b5f108
> [    5.184194] x25: ffff000000b5f280 x24: ffff8003fffe6ea8
> [    5.190857] x23: ffff000000b5f028 x22: 0000000000000000
> [    5.197002] x21: 0000000000000590 x20: ffff8003ec98e010
> [    5.202288] x19: ffff8003ec98e000 x18: 0000000000000001
> [    5.207573] x17: 0000000000000004 x16: 0000000000000000
> [    5.212859] x15: ffffffffffffffff x14: ffff8003e3e85f90
> [    5.218145] x13: ffff8003937afb0a x12: 0000000000000030
> [    5.223430] x11: 0000000000000003 x10: 0101010101010101
> [    5.228716] x9 : fffffffffffffffa x8 : 7f7f7f7f7f7f7f7f
> [    5.234000] x7 : fefefeff646c606d x6 : 1e0e1a00f2ade4ef
> [    5.239286] x5 : 6f642d72001a0e1e x4 : 8080808000000000
> [    5.244572] x3 : 837bbb2b916b2378 x2 : 82962d9749f72000
> [    5.249856] x1 : ffff000000b5e850 x0 : ffff000008726598
> [    5.255142] Process systemd-udevd (pid: 336, stack limit =
> 0x00000000d88c80b)
> [    5.262329] Call trace:
> [    5.264765]  sdhci_arasan_probe+0x30/0x638 [sdhci_of_arasan]
> [    5.270395]  platform_drv_probe+0x60/0xc0
> [    5.274384]  driver_probe_device+0x2a0/0x468
> [    5.278632]  __driver_attach+0x124/0x128
> [    5.282534]  bus_for_each_dev+0x84/0xd8
> [    5.286349]  driver_attach+0x30/0x40
> [    5.289906]  bus_add_driver+0x26c/0x298
> [    5.293721]  driver_register+0x6c/0x110
> [    5.297537]  __platform_driver_register+0x54/0x60
> [    5.302218]  sdhci_arasan_driver_init+0x1c/0x1000 [sdhci_of_arasan]
> [    5.308455]  do_one_initcall+0x58/0x160
> [    5.312271]  do_init_module+0x60/0x1f0
> [    5.316001]  load_module+0x13fc/0x1728
> [    5.319730]  SyS_finit_module+0xfc/0x118
> [    5.323632]  __sys_trace_return+0x0/0x4
> [    5.327449] Code: d503201f 58002ae1 58002a55 f9413e78 (f94002a0)
> [    5.333513] ---[ end trace aabd65049cbf2f2e ]---
>
> 0000000000000840 <sdhci_arasan_probe>:
>  840:   a9b67bfd        stp     x29, x30, [sp, #-160]!
>  844:   910003fd        mov     x29, sp
>  848:   a90153f3        stp     x19, x20, [sp, #16]
>  84c:   f90013f5        str     x21, [sp, #32]
>  850:   a90363f7        stp     x23, x24, [sp, #48]
>  854:   f90023f9        str     x25, [sp, #64]
>  858:   aa0003f3        mov     x19, x0
>  85c:   aa1e03e0        mov     x0, x30
>  860:   94000000        bl      0 <_mcount>
>  864:   58002ae1        ldr     x1, dc0 <sdhci_arasan_probe+0x580>
>  868:   58002a55        ldr     x21, db0 <sdhci_arasan_probe+0x570> <----
> This looks like garbage
>  86c:   f9413e78        ldr     x24, [x19, #632]
>  870:   f94002a0        ldr     x0, [x21]
>
> There was another crash in xfs:
>
> [    6.273348] Internal error: Accessing user space memory outside uaccess.h
> roP
> [    6.282530] Modules linked in: xfs libcrc32c sdhci_of_arasan(+)
> sdhci_pltfm s
> [    6.294138] CPU: 1 PID: 467 Comm: mount Tainted: G      D
> 4.15.0-ca6
> [    6.302016] Hardware name: AppliedMicro X-Gene Mustang Board/X-Gene
> Mustang 6
> [    6.311710] pstate: 40400005 (nZcv daif +PAN -UAO)
> [    6.316666] pc : xfs_parseargs+0x2c/0x668 [xfs]
> [    6.321330] lr : xfs_fs_fill_super+0x120/0x550 [xfs]
> [    6.326269] sp : ffff00000c82bba0
> [    6.329566] x29: ffff00000c82bba0 x28: ffff800393869e00
> [    6.334852] x27: ffff000008ad1000 x26: 0000000fffffffe0
> [    6.340138] x25: 0000000000000590 x24: 0000000000000000
> [    6.345424] x23: ffff000000c5df68 x22: ffff00000819cef8
> [    6.350709] x21: ffff8003929b1000 x20: ffff800393431000
> [    6.355995] x19: ffff8003929b1000 x18: 0000000000000009
> [    6.361280] x17: 0000000000000000 x16: 0000000000000000
> [    6.366566] x15: ffffffffffffffff x14: ffffffffff000000
> [    6.371851] x13: ffffffffffffffff x12: 0000000000000018
> [    6.377136] x11: 0101010101010101 x10: ffff00000c82bcb0
> [    6.382421] x9 : 0000000000000000 x8 : ffff800393432000
> [    6.387706] x7 : 0000000000000000 x6 : ffff000000bdfaf8
> [    6.392992] x5 : ffff000000be0670 x4 : 0000000000000000
> [    6.398277] x3 : 0000000000000000 x2 : 00000000000000c0
> [    6.403562] x1 : 0000000000000000 x0 : ffff000000bf71f8
> [    6.408850] Process mount (pid: 467, stack limit = 0x00000000cda0c16d)
> [    6.415344] Call trace:
> [    6.417933]  xfs_parseargs+0x2c/0x668 [xfs]
> [    6.422256]  xfs_fs_fill_super+0x120/0x550 [xfs]
> [    6.426854]  mount_bdev+0x1a0/0x1d0
> [    6.430477]  xfs_fs_mount+0x40/0x58 [xfs]
> [    6.434468]  mount_fs+0x5c/0x190
> [    6.437680]  vfs_kern_mount.part.9+0x54/0x178
> [    6.442014]  do_mount+0x1f8/0xc68
> [    6.445312]  SyS_mount+0xfc/0x118
> [    6.448612]  __sys_trace_return+0x0/0x4
> [    6.452431] Code: d503201f 58002ed9 52801802 f9400293 (f9400320)
> [    6.458496] ---[ end trace aabd65049cbf2f2f ]---
>
> 0000000000088a00 <xfs_parseargs>:
>    88a00:       a9b47bfd        stp     x29, x30, [sp, #-192]!
>    88a04:       910003fd        mov     x29, sp
>    88a08:       a90153f3        stp     x19, x20, [sp, #16]
>    88a0c:       f90023f9        str     x25, [sp, #64]
>    88a10:       aa0003f4        mov     x20, x0
>    88a14:       aa1e03e0        mov     x0, x30
>    88a18:       f90037e1        str     x1, [sp, #104]
>    88a1c:       94000000        bl      0 <_mcount>
>    88a20:       58002ed9        ldr     x25, 88ff8 <xfs_parseargs+0x5f8>
> <---- also looks bogus
>    88a24:       52801802        mov     w2, #0xc0                       //
> #192
>    88a28:       f9400293        ldr     x19, [x20]
>    88a2c:       f9400320        ldr     x0, [x25]
> ...
>    88fec:       a90363f7        stp     x23, x24, [sp, #48]
>    88ff0:       a904effa        stp     x26, x27, [sp, #72]
>    88ff4:       94000000        bl      0 <__stack_chk_fail>
>         ...
>
>
>
>>
>>> It looked good to me otherwise.
>>>
>>
>> Thanks. I intend to try and restart the discussion on this topic this
>> week.
>>
>>>
>>>> Ard Biesheuvel (2):
>>>>     gcc-plugins: add support plugin for arm64 per-task stack canaries
>>>>     arm64: kernel: use a unique stack canary value for each task
>>>>
>>>>    arch/Kconfig                                    |   4 +
>>>>    arch/arm64/Kconfig                              |   7 ++
>>>>    arch/arm64/include/asm/stackprotector.h         |   4 +-
>>>>    arch/arm64/kernel/asm-offsets.c                 |   3 +
>>>>    arch/arm64/kernel/process.c                     |   4 +
>>>>    arch/arm64/kernel/vmlinux.lds.S                 |   8 ++
>>>>    scripts/Makefile.gcc-plugins                    |   2 +
>>>>    scripts/gcc-plugins/arm64_ssp_per_task_plugin.c | 121
>>>> ++++++++++++++++++++
>>>>    8 files changed, 152 insertions(+), 1 deletion(-)
>>>>    create mode 100644 scripts/gcc-plugins/arm64_ssp_per_task_plugin.c
>>>>
>>>
>