ARM: support THREAD_INFO_IN_TASK

We will be enabling THREAD_INFO_IN_TASK support for ARM, which means that we can no longer load the stack canary value by masking the stack pointer and taking the copy that lives in thread_info. Instead, we will be able to load it from the task_struct directly, by using the TPIDRURO register which will hold the current task pointer when THREAD_INFO_IN_TASK is in effect. This is much more straight-forward, and allows us to declutter this code a bit while at it.

Note that this means that ARMv6 (non-v6K) SMP systems can no longer use this feature, but those are quite rare to begin with, so this is a reasonable trade off.

Reviewed-by Kees Cook <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

From: Keith Packard <[email protected]>

This avoids needing to compute the task pointer in this function, which will no longer be possible once we move thread_info off the stack.

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

To prepare for a subsequent patch that stores the current task pointer in the user space TLS register while running in the kernel, modify the set_tls and switch_tls routines not to touch the register directly, and update the return to user space code to load the correct value.

Signed-off-by Ard Biesheuvel <[email protected]>

Now that the user space TLS register is assigned on every return to user space, we can use it to keep the 'current' pointer while running in the kernel. This removes the need to access it via thread_info, which is located at the base of the stack, but will be moved out of there in a subsequent patch.

Use the __builtin_thread_pointer() helper when available - this will help GCC understand that reloading the value within the same function is not necessary, even when using the per-task stack protector (which also generates accesses via the TLS register). For example, the generated code below loads TPIDRURO only once, and uses it to access both the stack canary and the preempt_count fields.

<do_one_initcall>:

       e92d 41f0       stmdb   sp!, {r4, r5, r6, r7, r8, lr}
       ee1d 4f70       mrc     15, 0, r4, cr13, cr0, {3}
       4606            mov     r6, r0
       b094            sub     sp, #80 ; 0x50
       f8d4 34e8       ldr.w   r3, [r4, #1256] ; 0x4e8  <- stack canary
       9313            str     r3, [sp, #76]   ; 0x4c
       f8d4 8004       ldr.w   r8, [r4, #4]             <- preempt count

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Quoted 22 lines · Ard Biesheuvel

Now that the user space TLS register is assigned on every return to user space, we can use it to keep the 'current' pointer while running in the kernel. This removes the need to access it via thread_info, which is located at the base of the stack, but will be moved out of there in a subsequent patch.

Use the __builtin_thread_pointer() helper when available - this will help GCC understand that reloading the value within the same function is not necessary, even when using the per-task stack protector (which also generates accesses via the TLS register). For example, the generated code below loads TPIDRURO only once, and uses it to access both the stack canary and the preempt_count fields.

<do_one_initcall>:

       e92d 41f0       stmdb   sp!, {r4, r5, r6, r7, r8, lr}
       ee1d 4f70       mrc     15, 0, r4, cr13, cr0, {3}
       4606            mov     r6, r0
       b094            sub     sp, #80 ; 0x50
       f8d4 34e8       ldr.w   r3, [r4, #1256] ; 0x4e8  <- stack canary
       9313            str     r3, [sp, #76]   ; 0x4c
       f8d4 8004       ldr.w   r8, [r4, #4]             <- preempt count

Quoted 3 lines

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

I like the __builtin trick, I had to look up the patch that adds this to GCC to understand what is going on.

Reviewed-by Linus Walleij <[email protected]>

Now that we no longer rely on thread_info living at the base of the task stack to be able to access the 'current' pointer, we can wire up the generic support for moving thread_info into the task struct itself.

Note that this requires us to update the cpu field in thread_info explicitly, now that the core code no longer does so. Ideally, we would switch the percpu code to access the cpu field in task_struct instead, but this unleashes #include circular dependency hell.

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
  arch/arm/Kconfig                   |  1 +
  arch/arm/include/asm/assembler.h   |  5 +++++
  arch/arm/include/asm/switch_to.h   | 14 ++++++++++++++
  arch/arm/include/asm/thread_info.h | 10 +++++++++-
  arch/arm/kernel/asm-offsets.c      |  2 ++
  arch/arm/kernel/entry-armv.S       |  2 +-
  arch/arm/kernel/smp.c              |  3 +++
  7 files changed, 35 insertions(+), 2 deletions(-)

arch/arm/Kconfig +1 -0

index cd195e6f4ea6..4f61c9789e7f 100644
@@ -125,6 +125,7 @@ config ARM
  	select PERF_USE_VMALLOC
  	select RTC_LIB
  	select SYS_SUPPORTS_APM_EMULATION
+	select THREAD_INFO_IN_TASK if CURRENT_POINTER_IN_TPIDRURO
  	select TRACE_IRQFLAGS_SUPPORT if !CPU_V7M
  	# Above selects are sorted alphabetically; please add new ones
  	# according to that.  Thanks.

arch/arm/include/asm/assembler.h +0 -0

index c1551dee28be..7d23d4bb2168 100644

arch/arm/include/asm/assembler.h +2 -0

+#else
   ARM(	mov	\rd, sp, lsr #THREAD_SIZE_ORDER + PAGE_SHIFT	)
   THUMB(	mov	\rd, sp			)
   THUMB(	lsr	\rd, \rd, #THREAD_SIZE_ORDER + PAGE_SHIFT	)
  	mov	\rd, \rd, lsl #THREAD_SIZE_ORDER + PAGE_SHIFT
+#endif
  	.endm
 
  /*

arch/arm/include/asm/switch_to.h +14 -0

index 61e4a3c4ca6e..b55c7b2755e4 100644
@@ -23,9 +23,23 @@
   */
  extern struct task_struct *__switch_to(struct task_struct *, struct thread_info *, struct thread_info *);
 
+static inline void set_ti_cpu(struct task_struct *p)
+{
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+	/*
+	 * The core code no longer maintains the thread_info::cpu field once
+	 * CONFIG_THREAD_INFO_IN_TASK is in effect, but we rely on it for
+	 * raw_smp_processor_id(), which cannot access struct task_struct*
+	 * directly for reasons of circular #inclusion hell.
+	 */
+	task_thread_info(p)->cpu = task_cpu(p);
+#endif
+}
+
  #define switch_to(prev,next,last)					\
  do {									\
  	__complete_pending_tlbi();					\
+	set_ti_cpu(next);						\
  	if (IS_ENABLED(CONFIG_CURRENT_POINTER_IN_TPIDRURO))		\
  		__this_cpu_write(__entry_task, next);			\
  	last = __switch_to(prev,task_thread_info(prev), task_thread_info(next));	\

arch/arm/include/asm/thread_info.h +9 -1

index 76b6fbd5540c..787511396f3f 100644
@@ -54,7 +54,9 @@ struct cpu_context_save {
  struct thread_info {
  	unsigned long		flags;		/* low level flags */
  	int			preempt_count;	/* 0 => preemptable, <0 => bug */
+#ifndef CONFIG_THREAD_INFO_IN_TASK
  	struct task_struct	*task;		/* main task structure */
+#endif
  	__u32			cpu;		/* cpu */
  	__u32			cpu_domain;	/* cpu domain */
  	struct cpu_context_save	cpu_context;	/* cpu context */

@@ -70,11 +72,16 @@ struct thread_info {
 
  #define INIT_THREAD_INFO(tsk)						\
  {									\
-	.task		= &tsk,						\
+	INIT_THREAD_INFO_TASK(tsk)					\
  	.flags		= 0,						\
  	.preempt_count	= INIT_PREEMPT_COUNT,				\
  }
 
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+#define INIT_THREAD_INFO_TASK(tsk)
+#else
+#define INIT_THREAD_INFO_TASK(tsk)	.task = &(tsk),
+
  /*
   * how to get the thread information struct from C
   */

@@ -85,6 +92,7 @@ static inline struct thread_info *current_thread_info(void)
  	return (struct thread_info *)
  		(current_stack_pointer & ~(THREAD_SIZE - 1));
  }
+#endif
 
  #define thread_saved_pc(tsk)	\
  	((unsigned long)(task_thread_info(tsk)->cpu_context.pc))

arch/arm/kernel/asm-offsets.c +2 -0

index 9c864ee76107..645845e4982a 100644
@@ -43,7 +43,9 @@ int main(void)
    BLANK();
    DEFINE(TI_FLAGS,		offsetof(struct thread_info, flags));
    DEFINE(TI_PREEMPT,		offsetof(struct thread_info, preempt_count));
+#ifndef CONFIG_THREAD_INFO_IN_TASK
    DEFINE(TI_TASK,		offsetof(struct thread_info, task));
+#endif
    DEFINE(TI_CPU,		offsetof(struct thread_info, cpu));
    DEFINE(TI_CPU_DOMAIN,		offsetof(struct thread_info, cpu_domain));
    DEFINE(TI_CPU_SAVE,		offsetof(struct thread_info, cpu_context));

arch/arm/kernel/entry-armv.S +1 -1

index 7263a45abf3d..a54b5044d406 100644
@@ -765,7 +765,7 @@ ENTRY(__switch_to)
  	.endif
  	ldr	r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
  #elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
-	ldr	r7, [r2, #TI_TASK]
+	mov	r7, r2				@ Preserve 'next'
  #endif
  #ifdef CONFIG_CPU_USE_DOMAINS
  	mcr	p15, 0, r6, c3, c0, 0		@ Set domain register

arch/arm/kernel/smp.c +0 -0

index 97ee6b1567e9..cde5b6d8bac5 100644

Quoted 42 lines · Ard Biesheuvel

Now that we no longer rely on thread_info living at the base of the task stack to be able to access the 'current' pointer, we can wire up the generic support for moving thread_info into the task struct itself.

Note that this requires us to update the cpu field in thread_info explicitly, now that the core code no longer does so. Ideally, we would switch the percpu code to access the cpu field in task_struct instead, but this unleashes #include circular dependency hell.

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
  arch/arm/Kconfig                   |  1 +
  arch/arm/include/asm/assembler.h   |  5 +++++
  arch/arm/include/asm/switch_to.h   | 14 ++++++++++++++
  arch/arm/include/asm/thread_info.h | 10 +++++++++-
  arch/arm/kernel/asm-offsets.c      |  2 ++
  arch/arm/kernel/entry-armv.S       |  2 +-
  arch/arm/kernel/smp.c              |  3 +++
  7 files changed, 35 insertions(+), 2 deletions(-)

arch/arm/Kconfig +1 -0

index cd195e6f4ea6..4f61c9789e7f 100644
@@ -125,6 +125,7 @@ config ARM
      select PERF_USE_VMALLOC
      select RTC_LIB
      select SYS_SUPPORTS_APM_EMULATION
+     select THREAD_INFO_IN_TASK if CURRENT_POINTER_IN_TPIDRURO
      select TRACE_IRQFLAGS_SUPPORT if !CPU_V7M
      # Above selects are sorted alphabetically; please add new ones
      # according to that.  Thanks.

arch/arm/include/asm/assembler.h +0 -0

index c1551dee28be..7d23d4bb2168 100644
@@ -227,10 +227,15 @@
   * Get current thread_info.
   */
      .macro  get_thread_info, rd

arch/arm/include/asm/assembler.h line 232

Quoted 3 lines

arch/arm/include/asm/assembler.h +3 -0

+#ifdef CONFIG_THREAD_INFO_IN_TASK
+     /* thread_info is the first member of struct task_struct */
+     get_current \rd

I have a minor review comment here,
By looking at the this code it looks like get_thread_info calls get_current and get_current again calls get_thread_info. Probably recursion may never happen due to config dependency order.

Now that we no longer rely on thread_info living at the base of the task stack to be able to access the 'current' pointer, we can wire up the generic support for moving thread_info into the task struct itself.

Note that this requires us to update the cpu field in thread_info explicitly, now that the core code no longer does so. Ideally, we would switch the percpu code to access the cpu field in task_struct instead, but this unleashes #include circular dependency hell.

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Placing thread_info in the kernel stack leaves it vulnerable to stack overflow attacks. This short series addresses that by using the existing THREAD_INFO_IN_TASK infrastructure.

Changes since v4:

- Pass -mtp=cp15 to the compiler to force the use of the TLS register when __builtin_thread_pointer() is used - this allows us to enable its use on Clang as well.

- Tweak the __switch_to() changes not to affect builds that have the feature disabled (as requested by Russell), and to defer update of the TLS register to the point where the stack pointer is updated as well.

- Tweak the #ifdef's so we avoid touching the TLS registers on builds that may target v6 systems without HWCAP_TLS.

- Use task_cpu() in the final patch to assign thread_info->cpu, so that this series can be carried independently of the series I proposed for moving the CPU field back into thread_info [1].

Changes since v3:

- Leave the CPU field in thread_info, and keep it in sync at context switch time. This is by far the easiest and cleanest way to work around the fact that it is infeasible to implement raw_smp_processor_id() in terms of task_struct::cpu (for reasons of header soup).

- Drop the VFP changes, they are no longer necessary given the previous point.

- Drop the change to pass the CPU number to secondary_start_kernel(). Given that we also need to pass the idle task pointer, which carries the CPU number, passing the CPU number directly is redundant.

- Use the TPIDRURO register to carry 'current' while running in the kernel, and keep using TPIDRPRW for the per-CPU offset as before. This way, there is no need to make any changes to the way the per-CPU offsets are programmed. It also avoids the concurrency issues that would result from carrying the 'current' pointer in a per-CPU variable.

- Update the per-task stack protector plugin to pull the stack canary value directly from the task struct.

Cc Keith Packard <[email protected]>

Cc Russell King <[email protected]>

Cc Kees Cook <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Linus Walleij <[email protected]>

[0] https://lore.kernel.org/all/[email protected]/
[1] https://lore.kernel.org/all/[email protected]/

Ard Biesheuvel (4):
gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
ARM: smp: Free up the TLS register while running in the kernel
ARM: smp: Store current pointer in TPIDRURO register if available
ARM: smp: Enable THREAD_INFO_IN_TASK

Keith Packard (1):
ARM: smp: Pass task to secondary_start_kernel

  arch/arm/Kconfig                              |  8 +++-
  arch/arm/Makefile                             |  9 ++--
  arch/arm/include/asm/assembler.h              | 29 ++++++++++++
  arch/arm/include/asm/current.h                | 50 ++++++++++++++++++++
  arch/arm/include/asm/smp.h                    |  3 +-
  arch/arm/include/asm/stackprotector.h         |  2 -
  arch/arm/include/asm/switch_to.h              | 16 +++++++
  arch/arm/include/asm/thread_info.h            | 15 ++++--
  arch/arm/include/asm/tls.h                    | 10 ++--
  arch/arm/kernel/asm-offsets.c                 |  6 +--
  arch/arm/kernel/entry-armv.S                  |  5 ++
  arch/arm/kernel/entry-common.S                |  1 +
  arch/arm/kernel/entry-header.S                |  8 ++++
  arch/arm/kernel/head-common.S                 |  5 ++
  arch/arm/kernel/head-nommu.S                  |  1 +
  arch/arm/kernel/head.S                        |  5 +-
  arch/arm/kernel/process.c                     |  8 ++--
  arch/arm/kernel/smp.c                         | 13 ++++-
  arch/arm/mm/proc-macros.S                     |  3 +-
  scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 27 +++--------
  20 files changed, 174 insertions(+), 50 deletions(-)

create mode 100644 arch/arm/include/asm/current.h

Quoted 42 lines · Ard Biesheuvel

Placing thread_info in the kernel stack leaves it vulnerable to stack overflow attacks. This short series addresses that by using the existing THREAD_INFO_IN_TASK infrastructure.

Changes since v4:

- Pass -mtp=cp15 to the compiler to force the use of the TLS register when __builtin_thread_pointer() is used - this allows us to enable its use on Clang as well.

- Tweak the __switch_to() changes not to affect builds that have the feature disabled (as requested by Russell), and to defer update of the TLS register to the point where the stack pointer is updated as well.

- Tweak the #ifdef's so we avoid touching the TLS registers on builds that may target v6 systems without HWCAP_TLS.

- Use task_cpu() in the final patch to assign thread_info->cpu, so that this series can be carried independently of the series I proposed for moving the CPU field back into thread_info [1].

Changes since v3:

- Leave the CPU field in thread_info, and keep it in sync at context switch time. This is by far the easiest and cleanest way to work around the fact that it is infeasible to implement raw_smp_processor_id() in terms of task_struct::cpu (for reasons of header soup).

- Drop the VFP changes, they are no longer necessary given the previous point.

- Drop the change to pass the CPU number to secondary_start_kernel(). Given that we also need to pass the idle task pointer, which carries the CPU number, passing the CPU number directly is redundant.

- Use the TPIDRURO register to carry 'current' while running in the kernel, and keep using TPIDRPRW for the per-CPU offset as before. This way, there is no need to make any changes to the way the per-CPU offsets are programmed. It also avoids the concurrency issues that would result from carrying the 'current' pointer in a per-CPU variable.

Quoted 8 lines

- Update the per-task stack protector plugin to pull the stack canary value directly from the task struct.

Cc Keith Packard <[email protected]>

Cc Russell King <[email protected]>

Cc Kees Cook <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Linus Walleij <[email protected]>

This series boots with both CURRENT_POINTER_IN_TPIDRURO and !CURRENT_POINTER_IN_TPIDRURO mode so,

Tested-by Amit Daniel Kachhap <[email protected]>