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.

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

Hi Ard,

thanks for this patch series!

arch/arm/Kconfig config XEN · line 1604

Quoted 5 lines · Ard Biesheuvel

config STACKPROTECTOR_PER_TASK
bool "Use a unique stack canary value for each task"

arch/arm/Kconfig +1 -1

-       depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
+       depends on GCC_PLUGINS && STACKPROTECTOR && THREAD_INFO_IN_TASK && !XIP_DEFLATED_DATA
        select GCC_PLUGIN_ARM_SSP_PER_TASK

Am I reading this right that STACKPROTECTOR_PER_TASK was available for ARMv6 with SMP before this change and after this change it will only be available for ARMv7 since THREAD_INFO_IN_TASK which is now a dependency will only be available for ARMv7 (or ARMv6k)?

I suppose this is a reasonable compromise, as I don't use it or need it on my ARM PB11MPCore and neither do I think the Oxnas router, but we should write this explicitly in the commit message.

Quoted 13 lines · Linus Walleij

Hi Ard,

thanks for this patch series!

arch/arm/Kconfig config XEN · line 1604

Quoted 5 lines · Ard Biesheuvel

config STACKPROTECTOR_PER_TASK
bool "Use a unique stack canary value for each task"

arch/arm/Kconfig +1 -1

-       depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
+       depends on GCC_PLUGINS && STACKPROTECTOR && THREAD_INFO_IN_TASK && !XIP_DEFLATED_DATA
        select GCC_PLUGIN_ARM_SSP_PER_TASK

Quoted 6 lines

Am I reading this right that STACKPROTECTOR_PER_TASK was available for ARMv6 with SMP before this change and after this change it will only be available for ARMv7 since THREAD_INFO_IN_TASK which is now a dependency will only be available for ARMv7 (or ARMv6k)?

Yes. We could potentially keep the old code path in the plugin as well, but what I would really prefer to do is get -mstack-protector-guard=tls implemented in GCC, so we can phase out the plugin entirely.

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

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

@@ -767,13 +769,18 @@ ENTRY(__switch_to)
 	mcr	p15, 0, r6, c3, c0, 0		@ Set domain register
 #endif
 	mov	r5, r0
-	add	r4, r2, #TI_CPU_SAVE
+	mov	r4, r2
 	ldr	r0, =thread_notify_head
 	mov	r1, #THREAD_NOTIFY_SWITCH
 	bl	atomic_notifier_call_chain
 #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)

Quoted 10 lines · Ard Biesheuvel

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

@@ -767,13 +769,18 @@ ENTRY(__switch_to)
      mcr     p15, 0, r6, c3, c0, 0           @ Set domain register
 #endif
      mov     r5, r0
-     add     r4, r2, #TI_CPU_SAVE
+     mov     r4, r2
      ldr     r0, =thread_notify_head
      mov     r1, #THREAD_NOTIFY_SWITCH
      bl      atomic_notifier_call_chain
 #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)

arch/arm/kernel/entry-armv.S ENTRY(__switch_to) · line 786

Quoted 10 lines

arch/arm/kernel/entry-armv.S +5 -0

      str     r7, [r8]
 #endif
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+     ldr     ip, [r4, #TI_TASK]
+     set_current ip
+#endif
+     add     r4, r4, #TI_CPU_SAVE
  THUMB(      mov     ip, r4                     )
      mov     r0, r5
  ARM(        ldmia   r4, {r4 - sl, fp, sp, pc}  )    @ Load all regs saved previously

I would like to keep this as optimal as possible, and the setting of TPIDRURO to be as close to the stack pointer change too, so I'd suggest a slightly different approach:

        switch_tls r1, r4, r5, r3, r7

(quoted diff) +4 -0

+#if (defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)) || \
+    defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
+       ldr     r9, [r2, #TI_TASK]
+#endif

#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)

(quoted diff) +4 -3

-       ldr     r7, [r2, #TI_TASK]
        ldr     r8, =__stack_chk_guard
        .if (TSK_STACK_CANARY > IMM12_MASK)
-       add     r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
-       .endif
+       add     r7, r9, #TSK_STACK_CANARY & ~IMM12_MASK
        ldr     r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
+       .else
+       ldr     r7, [r9, #TSK_STACK_CANARY & IMM12_MASK]
+       .endif

#endif
...
mov r0, r5

arch/arm/kernel/entry-armv.S +3 -0

+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+       set_current r9
+#endif
 ARM(   ldmia   r4, {r4 - sl, fp, sp, pc}  )    @ Load all regs saved previously

It's a slightly more complex change, but one I think we should do.

Hi Russell,

Thanks for taking a look.

However, the next patch drops the 'ldr ip, [r4, #TI_TASK]' I am adding here, so I won't be able to use this as-is.

I could just split the ldr/set_current sequence into two, and change the first one into a mov in the following patch, and move the set_current to right before where sp is updated.

I.e., in this patch

(quoted diff) +3 -0

@@ -762,6 +764,8 @@ ENTRY(__switch_to)
        add     r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
        .endif
        ldr     r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
+#elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
+       ldr     r7, [r2, #TI_TASK]
 #endif
 #ifdef CONFIG_CPU_USE_DOMAINS
        mcr     p15, 0, r6, c3, c0, 0           @ Set domain register

@@ -776,6 +780,7 @@ ENTRY(__switch_to)
 #endif
  THUMB(        mov     ip, r4                     )
        mov     r0, r5
+       set_current r7
  ARM(  ldmia   r4, {r4 - sl, fp, sp, pc}  )
  THUMB(        ldmia   ip!, {r4 - sl, fp}         )
  THUMB(        ldr     sp, [ip], #4               )

and in the next one

(quoted diff) +1 -1

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

Would that work for you?

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.

This v4 is a follow-up to Keith's v3 [0], which did not address all concerns I raised in response to v2. After collaborating with Keith off-list, we decided that I should go ahead and post our joint v4.

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.

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.

This v4 is a follow-up to Keith's v3 [0], which did not address all concerns I raised in response to v2. After collaborating with Keith off-list, we decided that I should go ahead and post our joint v4.

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.

This v4 is a follow-up to Keith's v3 [0], which did not address all concerns I raised in response to v2. After collaborating with Keith off-list, we decided that I should go ahead and post our joint v4.

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.

Quoted 24 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.

This v4 is a follow-up to Keith's v3 [0], which did not address all concerns I raised in response to v2. After collaborating with Keith off-list, we decided that I should go ahead and post our joint v4.

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.

Quoted 8 lines

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

I haven't looked in detail, but it all looks great to me so far.

On a related note, this reminds me of the CONFIG_IRQSTACK series that was posted last year[1] and probably attempted before that. I'd have to get back to understanding all the details of that discussion, but I have some hope that adding irqstacks would become much easier after your series. Any thoughts on that?

Quoted 7 lines · Arnd Bergmann

I haven't looked in detail, but it all looks great to me so far.

On a related note, this reminds me of the CONFIG_IRQSTACK series that was posted last year[1] and probably attempted before that. I'd have to get back to understanding all the details of that discussion, but I have some hope that adding irqstacks would become much easier after your series. Any thoughts on that?

Yes, not having to rely on the stack pointer for thread_info/current seems to make this a lot easier. And eith mentioned that he is also interested in vmap'ed stacks for ARM, so I have a suspicion that we will end up picking up some IRQ stack changes in the process as well.

Quoted 10 lines · Ard Biesheuvel

Quoted 7 lines · Arnd Bergmann

I haven't looked in detail, but it all looks great to me so far.

On a related note, this reminds me of the CONFIG_IRQSTACK series that was posted last year[1] and probably attempted before that. I'd have to get back to understanding all the details of that discussion, but I have some hope that adding irqstacks would become much easier after your series. Any thoughts on that?

Quoted 4 lines

Yes, not having to rely on the stack pointer for thread_info/current seems to make this a lot easier. And eith mentioned that he is also interested in vmap'ed stacks for ARM, so I have a suspicion that we will end up picking up some IRQ stack changes in the process as well.

Ah, perfect! Adding vmap stack also solves ones of the issues for the 4G:4G split: since vmalloc/vmap space is always mapped in kernel mode, copy_{from,to}_user() can then just use memcpy() with TTBR0 switched to used memory if the kernel buffer is on the stack, or use an intermediate buffer on the stack if it's not.