[RFC PATCH 0/1] implement TLS register based stack canary for ARM

[RFC PATCH 0/1] implement TLS register based stack canary for ARM

[Ard Biesheuvel]

Bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102352

In the Linux kernel, user processes calling into the kernel are
essentially threads running in the same address space, of a program that
never terminates. This means that using a global variable for the stack
protector canary value is problematic on SMP systems, as we can never
change it unless we reboot the system. (Processes that sleep for any
reason will do so on a call into the kernel, which means that there will
always be live kernel stack frames carrying copies of the canary taken
when the function was entered)

AArch64 implements -mstack-protector-guard=sysreg for this purpose, as
this permits the kernel to use different memory addresses for the stack
canary for each CPU, and context switch the chosen system register with
the rest of the process, allowing each process to use its own unique
value for the stack canary.

This patch implements something similar, but for the 32-bit ARM kernel,
which will start using the user space TLS register TPIDRURO to index
per-process metadata while running in the kernel. This means we can just
add an offset to TPIDRURO to obtain the address from which to load the
canary value.

The patch is a bit rough around the edges, but produces the correct
results as far as I can tell. However, I couldn't quite figure out how
to modify the patterns so that the offset will be moved into the
immediate offset field of the LDR instructions, so currently, the ADD of
the offset is always a distinct instruction.

As for the spilling issues that have been fixed in this code in the
past: I suppose a register carrying the TLS register value will never
get spilled to begin with? How about a register that carries TLS+<n>?

Comments/suggestions welcome.

Cc: thomas.preudhomme@celest.fr
Cc: adhemerval.zanella@linaro.org
Cc: Qing Zhao <qing.zhao@oracle.com>
Cc: Richard Sandiford <richard.sandiford@arm.com>
Cc: gcc-patches@gcc.gnu.org

Ard Biesheuvel (1):
  [ARM] Add support for TLS register based stack protector canary access

 gcc/config/arm/arm-opts.h |  6 +++
 gcc/config/arm/arm.c      | 39 +++++++++++++++++
 gcc/config/arm/arm.md     | 44 ++++++++++++++++++--
 gcc/config/arm/arm.opt    | 22 ++++++++++
 gcc/doc/invoke.texi       |  9 ++++
 5 files changed, 116 insertions(+), 4 deletions(-)

-- 
2.30.2

$ cat |arm-linux-gnueabihf-gcc -march=armv7-a -mstack-protector-guard=tls -mstack-protector-guard-offset=10 -mtp=cp15 -S -o - -xc - -fstack-protector-all -O3
int foo(void *);
int bar(void)
{

	return foo(__builtin_thread_pointer()) + 1;
}
	.arch armv7-a
	.fpu softvfp
	.eabi_attribute 20, 1
	.eabi_attribute 21, 1
	.eabi_attribute 23, 3
	.eabi_attribute 24, 1
	.eabi_attribute 25, 1
	.eabi_attribute 26, 2
	.eabi_attribute 30, 2
	.eabi_attribute 34, 1
	.eabi_attribute 18, 4
	.file	"<stdin>"
	.text
	.align	2
	.global	bar
	.syntax unified
	.arm
	.type	bar, %function
bar:
	@ args = 0, pretend = 0, frame = 8
	@ frame_needed = 0, uses_anonymous_args = 0
	push	{r4, lr}
	mrc	p15, 0, r4, c13, c0, 3	@ load_tp_hard
	add	r3, r4, #10
	sub	sp, sp, #8
	mov	r0, r4
	add	r4, r4, #10
	ldr	r3, [r3]
	str	r3, [sp, #4]
	mov	r3, #0
	bl	foo
	ldr	r3, [r4]
	ldr	r4, [sp, #4]
	eors	r3, r4, r3
	mov	r4, #0
	bne	.L5
	add	r0, r0, #1
	add	sp, sp, #8
	@ sp needed
	pop	{r4, pc}
.L5:
	bl	__stack_chk_fail
	.size	bar, .-bar
	.ident	"GCC: (GNU) 12.0.0 20211019 (experimental)"
	.section	.note.GNU-stack,"",%progbits

[RFC PATCH 1/1] [ARM] Add support for TLS register based stack protector canary access

[Ard Biesheuvel]

Add support for accessing the stack canary value via the TLS register,
so that multiple threads running in the same address space can use
distinct canary values. This is intended for the Linux kernel running in
SMP mode, where processes entering the kernel are essentially threads
running the same program concurrently: using a global variable for the
canary in that context is problematic because it can never be rotated,
and so the OS is forced to use the same value as long as it remains up.

Using the TLS register to index the stack canary helps with this, as it
allows each CPU to context switch the TLS register along with the rest
of the process, permitting each process to use its own value for the
stack canary.

2021-10-20 Ard Biesheuvel <ardb@kernel.org>

	* config/arm/arm-opts.h (enum stack_protector_guard): New
	* config/arm/arm.c (arm_option_override_internal): Handle
	and put in error checks for stack protector guard options.
	(arm_stack_protect_guard): New.
	(TARGET_STACK_PROTECT_GUARD): Define.
	* config/arm/arm.md (reg_stack_protect_address<mode>): New.
        (stack_protect_set): Adjust for SSP_GLOBAL.
        (stack_protect_test): Likewise.
	* config/arm/arm.opt (-mstack-protector-guard): New
	(-mstack-protector-guard-offset): New.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 gcc/config/arm/arm-opts.h |  6 +++
 gcc/config/arm/arm.c      | 39 +++++++++++++++++
 gcc/config/arm/arm.md     | 44 ++++++++++++++++++--
 gcc/config/arm/arm.opt    | 22 ++++++++++
 gcc/doc/invoke.texi       |  9 ++++
 5 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/gcc/config/arm/arm-opts.h b/gcc/config/arm/arm-opts.h
index 5c4b62f404f7..581ba3c4fbbb 100644
--- a/gcc/config/arm/arm-opts.h
+++ b/gcc/config/arm/arm-opts.h
@@ -69,4 +69,10 @@ enum arm_tls_type {
   TLS_GNU,
   TLS_GNU2
 };
+
+/* Where to get the canary for the stack protector.  */
+enum stack_protector_guard {
+  SSP_TLSREG,                  /* per-thread canary in TLS register */
+  SSP_GLOBAL                   /* global canary */
+};
 #endif
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index e51f60a1841d..deccc88e006e 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -829,6 +829,9 @@ static const struct attribute_spec arm_attribute_table[] =
 
 #undef TARGET_MD_ASM_ADJUST
 #define TARGET_MD_ASM_ADJUST arm_md_asm_adjust
+
+#undef TARGET_STACK_PROTECT_GUARD
+#define TARGET_STACK_PROTECT_GUARD arm_stack_protect_guard
 \f
 /* Obstack for minipool constant handling.  */
 static struct obstack minipool_obstack;
@@ -3157,6 +3160,26 @@ arm_option_override_internal (struct gcc_options *opts,
   if (TARGET_THUMB2_P (opts->x_target_flags))
     opts->x_inline_asm_unified = true;
 
+  if (arm_stack_protector_guard == SSP_GLOBAL
+      && opts->x_arm_stack_protector_guard_offset_str)
+    {
+      error ("incompatible options %'-mstack-protector-guard=global%' and"
+	     "%'-mstack-protector-guard-offset=%qs%'",
+	     arm_stack_protector_guard_offset_str);
+    }
+
+  if (opts->x_arm_stack_protector_guard_offset_str)
+    {
+      char *end;
+      const char *str = arm_stack_protector_guard_offset_str;
+      errno = 0;
+      long offs = strtol (arm_stack_protector_guard_offset_str, &end, 0);
+      if (!*str || *end || errno)
+	error ("%qs is not a valid offset in %qs", str,
+	       "-mstack-protector-guard-offset=");
+      arm_stack_protector_guard_offset = offs;
+    }
+
 #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS
   SUBTARGET_OVERRIDE_INTERNAL_OPTIONS;
 #endif
@@ -3824,6 +3847,10 @@ arm_option_reconfigure_globals (void)
       else
 	target_thread_pointer = TP_SOFT;
     }
+
+  if (arm_stack_protector_guard == SSP_TLSREG
+      && target_thread_pointer != TP_CP15)
+    error("%'-mstack-protector-guard=tls%' needs a hardware TLS register");
 }
 
 /* Perform some validation between the desired architecture and the rest of the
@@ -34052,6 +34079,18 @@ arm_run_selftests (void)
 }
 } /* Namespace selftest.  */
 
+/* Implement TARGET_STACK_PROTECT_GUARD. In case of a
+   global variable based guard use the default else
+   return a null tree.  */
+static tree
+arm_stack_protect_guard (void)
+{
+  if (arm_stack_protector_guard == SSP_GLOBAL)
+    return default_stack_protect_guard ();
+
+  return NULL_TREE;
+}
+
 #undef TARGET_RUN_TARGET_SELFTESTS
 #define TARGET_RUN_TARGET_SELFTESTS selftest::arm_run_selftests
 #endif /* CHECKING_P */
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 4adc976b8b67..f57e1db07e6a 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -9183,8 +9183,21 @@
 		      UNSPEC_SP_SET))
       (clobber (match_scratch:SI 2 ""))
       (clobber (match_scratch:SI 3 ""))])]
+  "arm_stack_protector_guard == SSP_GLOBAL"
   ""
-  ""
+)
+
+(define_expand "stack_protect_set"
+  [(match_operand:SI 0 "memory_operand")
+   (match_operand:SI 1 "")]
+  "arm_stack_protector_guard == SSP_TLSREG"
+  "
+{
+  rtx tp_reg = gen_reg_rtx (SImode);
+  emit_insn (gen_load_tp_hard (tp_reg));
+  emit_insn (gen_stack_protect_combined_set_insn (operands[0], tp_reg));
+  DONE;
+}"
 )
 
 ;; Use a separate insn from the above expand to be able to have the mem outside
@@ -9192,7 +9205,7 @@
 ;; try to reload the guard since we need to control how PIC access is done in
 ;; the -fpic/-fPIC case (see COMPUTE_NOW parameter when calling
 ;; legitimize_pic_address ()).
-(define_insn_and_split "*stack_protect_combined_set_insn"
+(define_insn_and_split "stack_protect_combined_set_insn"
   [(set (match_operand:SI 0 "memory_operand" "=m,m")
 	(unspec:SI [(mem:SI (match_operand:SI 1 "guard_addr_operand" "X,X"))]
 		   UNSPEC_SP_SET))
@@ -9206,7 +9219,10 @@
 	      (clobber (match_dup 2))])]
   "
 {
-  if (flag_pic)
+  if (arm_stack_protector_guard == SSP_TLSREG)
+    emit_insn (gen_addsi3 (operands[2], operands[1],
+			   GEN_INT (arm_stack_protector_guard_offset)));
+  else if (flag_pic)
     {
       rtx pic_reg;
 
@@ -9267,8 +9283,28 @@
       (clobber (match_scratch:SI 3 ""))
       (clobber (match_scratch:SI 4 ""))
       (clobber (reg:CC CC_REGNUM))])]
+  "arm_stack_protector_guard == SSP_GLOBAL"
   ""
-  ""
+)
+
+(define_expand "stack_protect_test"
+  [(match_operand:SI 0 "memory_operand")
+   (match_operand:SI 1 "")
+   (match_operand:SI 2 "")]
+  "arm_stack_protector_guard == SSP_TLSREG"
+  "
+{
+  rtx tp_reg = gen_reg_rtx (SImode);
+  emit_insn (gen_load_tp_hard (tp_reg));
+  emit_insn (gen_addsi3 (tp_reg, tp_reg,
+			 GEN_INT (arm_stack_protector_guard_offset)));
+  emit_insn (gen_arm_stack_protect_test_insn (gen_reg_rtx (SImode),
+					      operands[0], tp_reg));
+  rtx eq, cc_reg = gen_rtx_REG (CC_Zmode, CC_REGNUM);
+  eq = gen_rtx_EQ (CC_Zmode, cc_reg, const0_rtx);
+  emit_jump_insn (gen_arm_cond_branch (operands[2], eq, cc_reg));
+  DONE;
+}"
 )
 
 ;; Use a separate insn from the above expand to be able to have the mem outside
diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt
index a7677eeb45c8..4b3e17bc319c 100644
--- a/gcc/config/arm/arm.opt
+++ b/gcc/config/arm/arm.opt
@@ -311,3 +311,25 @@ Generate code which uses the core registers only (r0-r14).
 mfdpic
 Target Mask(FDPIC)
 Enable Function Descriptor PIC mode.
+
+mstack-protector-guard=
+Target RejectNegative Joined Enum(stack_protector_guard) Var(arm_stack_protector_guard) Init(SSP_GLOBAL)
+Use given stack-protector guard.
+
+Enum
+Name(stack_protector_guard) Type(enum stack_protector_guard)
+Valid arguments to -mstack-protector-guard=:
+
+EnumValue
+Enum(stack_protector_guard) String(tls) Value(SSP_TLSREG)
+
+EnumValue
+Enum(stack_protector_guard) String(global) Value(SSP_GLOBAL)
+
+mstack-protector-guard-offset=
+Target Joined RejectNegative String Var(arm_stack_protector_guard_offset_str)
+Use an immediate to offset from the TLS register. This option is for use with
+fstack-protector-guard=tls and not for use in user-land code.
+
+TargetVariable
+long arm_stack_protector_guard_offset = 0
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 0cc8a8edd058..0da551600884 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -810,6 +810,7 @@ Objective-C and Objective-C++ Dialects}.
 -mpure-code @gol
 -mcmse @gol
 -mfix-cmse-cve-2021-35465 @gol
+-mstack-protector-guard=@var{guard} -mstack-protector-guard-offset=@var{offset} @gol
 -mfdpic}
 
 @emph{AVR Options}
@@ -20933,6 +20934,14 @@ enabled by default when the option @option{-mcpu=} is used with
 @code{cortex-m33}, @code{cortex-m35p} or @code{cortex-m55}.  The option
 @option{-mno-fix-cmse-cve-2021-35465} can be used to disable the mitigation.
 
+@item -mstack-protector-guard=@var{guard}
+@itemx -mstack-protector-guard-offset=@var{offset}
+@opindex mstack-protector-guard
+@opindex mstack-protector-guard-offset
+Generate stack protection code using canary at @var{guard}.  Supported
+locations are @samp{global} for a global canary or @samp{tls} for a
+canary accessible via the TLS register.
+
 @item -mfdpic
 @itemx -mno-fdpic
 @opindex mfdpic
-- 
2.30.2

Re: [RFC PATCH 0/1] implement TLS register based stack canary for ARM

[Ard Biesheuvel]

On Thu, 21 Oct 2021 at 12:23, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102352
>
> In the Linux kernel, user processes calling into the kernel are
> essentially threads running in the same address space, of a program that
> never terminates. This means that using a global variable for the stack
> protector canary value is problematic on SMP systems, as we can never
> change it unless we reboot the system. (Processes that sleep for any
> reason will do so on a call into the kernel, which means that there will
> always be live kernel stack frames carrying copies of the canary taken
> when the function was entered)
>
> AArch64 implements -mstack-protector-guard=sysreg for this purpose, as
> this permits the kernel to use different memory addresses for the stack
> canary for each CPU, and context switch the chosen system register with
> the rest of the process, allowing each process to use its own unique
> value for the stack canary.
>
> This patch implements something similar, but for the 32-bit ARM kernel,
> which will start using the user space TLS register TPIDRURO to index
> per-process metadata while running in the kernel. This means we can just
> add an offset to TPIDRURO to obtain the address from which to load the
> canary value.
>
> The patch is a bit rough around the edges, but produces the correct
> results as far as I can tell.

This is a lie

> However, I couldn't quite figure out how
> to modify the patterns so that the offset will be moved into the
> immediate offset field of the LDR instructions, so currently, the ADD of
> the offset is always a distinct instruction.
>

... and this is no longer true now that I fixed the correctness
problem. I will be sending out a v2 shortly, so please disregard this
one for now.


> As for the spilling issues that have been fixed in this code in the
> past: I suppose a register carrying the TLS register value will never
> get spilled to begin with? How about a register that carries TLS+<n>?
>
> Comments/suggestions welcome.
>
> Cc: thomas.preudhomme@celest.fr
> Cc: adhemerval.zanella@linaro.org
> Cc: Qing Zhao <qing.zhao@oracle.com>
> Cc: Richard Sandiford <richard.sandiford@arm.com>
> Cc: gcc-patches@gcc.gnu.org
>
> Ard Biesheuvel (1):
>   [ARM] Add support for TLS register based stack protector canary access
>
>  gcc/config/arm/arm-opts.h |  6 +++
>  gcc/config/arm/arm.c      | 39 +++++++++++++++++
>  gcc/config/arm/arm.md     | 44 ++++++++++++++++++--
>  gcc/config/arm/arm.opt    | 22 ++++++++++
>  gcc/doc/invoke.texi       |  9 ++++
>  5 files changed, 116 insertions(+), 4 deletions(-)
>
> --
> 2.30.2
>
> $ cat |arm-linux-gnueabihf-gcc -march=armv7-a -mstack-protector-guard=tls -mstack-protector-guard-offset=10 -mtp=cp15 -S -o - -xc - -fstack-protector-all -O3
> int foo(void *);
> int bar(void)
> {
>
>         return foo(__builtin_thread_pointer()) + 1;
> }
>         .arch armv7-a
>         .fpu softvfp
>         .eabi_attribute 20, 1
>         .eabi_attribute 21, 1
>         .eabi_attribute 23, 3
>         .eabi_attribute 24, 1
>         .eabi_attribute 25, 1
>         .eabi_attribute 26, 2
>         .eabi_attribute 30, 2
>         .eabi_attribute 34, 1
>         .eabi_attribute 18, 4
>         .file   "<stdin>"
>         .text
>         .align  2
>         .global bar
>         .syntax unified
>         .arm
>         .type   bar, %function
> bar:
>         @ args = 0, pretend = 0, frame = 8
>         @ frame_needed = 0, uses_anonymous_args = 0
>         push    {r4, lr}
>         mrc     p15, 0, r4, c13, c0, 3  @ load_tp_hard
>         add     r3, r4, #10
>         sub     sp, sp, #8
>         mov     r0, r4
>         add     r4, r4, #10
>         ldr     r3, [r3]
>         str     r3, [sp, #4]
>         mov     r3, #0
>         bl      foo
>         ldr     r3, [r4]
>         ldr     r4, [sp, #4]
>         eors    r3, r4, r3
>         mov     r4, #0
>         bne     .L5
>         add     r0, r0, #1
>         add     sp, sp, #8
>         @ sp needed
>         pop     {r4, pc}
> .L5:
>         bl      __stack_chk_fail
>         .size   bar, .-bar
>         .ident  "GCC: (GNU) 12.0.0 20211019 (experimental)"
>         .section        .note.GNU-stack,"",%progbits
>

Re: [RFC PATCH 0/1] implement TLS register based stack canary for ARM

[Kees Cook]

On Thu, Oct 21, 2021 at 06:34:04PM +0200, Ard Biesheuvel wrote:
> On Thu, 21 Oct 2021 at 12:23, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102352
> >
> > In the Linux kernel, user processes calling into the kernel are
> > essentially threads running in the same address space, of a program that
> > never terminates. This means that using a global variable for the stack
> > protector canary value is problematic on SMP systems, as we can never
> > change it unless we reboot the system. (Processes that sleep for any
> > reason will do so on a call into the kernel, which means that there will
> > always be live kernel stack frames carrying copies of the canary taken
> > when the function was entered)
> >
> > AArch64 implements -mstack-protector-guard=sysreg for this purpose, as
> > this permits the kernel to use different memory addresses for the stack
> > canary for each CPU, and context switch the chosen system register with
> > the rest of the process, allowing each process to use its own unique
> > value for the stack canary.
> >
> > This patch implements something similar, but for the 32-bit ARM kernel,
> > which will start using the user space TLS register TPIDRURO to index
> > per-process metadata while running in the kernel. This means we can just
> > add an offset to TPIDRURO to obtain the address from which to load the
> > canary value.
> >
> > The patch is a bit rough around the edges, but produces the correct
> > results as far as I can tell.
> 
> This is a lie

LOL.

> 
> > However, I couldn't quite figure out how
> > to modify the patterns so that the offset will be moved into the
> > immediate offset field of the LDR instructions, so currently, the ADD of
> > the offset is always a distinct instruction.
> >
> 
> ... and this is no longer true now that I fixed the correctness
> problem. I will be sending out a v2 shortly, so please disregard this
> one for now.

Heh, I hadn't even had a chance to test it, so I'll hold off. :)

Thanks!

-Kees

-- 
Kees Cook