Clean up Shadow Call Stack patches for 5.8
Will Deacon
maintainercover Clean up Shadow Call Stack patches for 5.8
Hi all,
Here's a series of cleanups I hacked together on top of a modified v13 of the Shadow Call Stack patches from Sami:
https://lore.kernel.org/r/20200515172355.GD23334@willie-the-truck
The main changes are:
* Move code out of arch/arm64 and into the core implementation
* Store the full SCS stack pointer instead of the offset
* Code simplification and general style things
I'd like to queue this on top early next week so that it can spend some quality time in linux-next.
Cheers,
Will
--->8
Will Deacon (6):
arm64: scs: Store absolute SCS stack pointer value in thread_info
scs: Move accounting into alloc/free functions
arm64: scs: Use 'scs_sp' register alias for x18
scs: Move scs_overflow_check() out of architecture code
scs: Remove references to asm/scs.h from core code
scs: Move DEFINE_SCS macro into core code
arch/Kconfig | 4 +-- arch/arm64/include/asm/scs.h | 29 ++++------------ arch/arm64/include/asm/thread_info.h | 4 +-- arch/arm64/kernel/asm-offsets.c | 2 +- arch/arm64/kernel/entry.S | 10 +++--- arch/arm64/kernel/head.S | 2 +- arch/arm64/kernel/process.c | 2 -- arch/arm64/kernel/scs.c | 6 +--- include/linux/scs.h | 16 +++++---- kernel/sched/core.c | 3 ++ kernel/scs.c | 52 +++++++++++++--------------- 11 files changed, 55 insertions(+), 75 deletions(-)
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Will Deacon
maintainerPATCH 1/6 arm64: scs: Store absolute SCS stack pointer value in thread_info
Storing the SCS information in thread_info as a {base,offset} pair introduces an additional load instruction on the ret-to-user path, since the SCS stack pointer in x18 has to be converted back to an offset by subtracting the base.
Replace the offset with the absolute SCS stack pointer value instead and avoid the redundant load.
--- arch/arm64/include/asm/scs.h | 9 ++++----- arch/arm64/include/asm/thread_info.h | 4 ++-- arch/arm64/kernel/asm-offsets.c | 2 +- include/linux/scs.h | 8 ++++---- kernel/scs.c | 3 +-- 5 files changed, 12 insertions(+), 14 deletions(-)
arch/arm64/include/asm/scs.h +4 -5
@@ -4,16 +4,15 @@#ifdef __ASSEMBLY__ +#include <asm/asm-offsets.h> +#ifdef CONFIG_SHADOW_CALL_STACK .macro scs_load tsk, tmp - ldp x18, \tmp, [\tsk, #TSK_TI_SCS_BASE] - add x18, x18, \tmp + ldr x18, [\tsk, #TSK_TI_SCS_SP] .endm .macro scs_save tsk, tmp - ldr \tmp, [\tsk, #TSK_TI_SCS_BASE] - sub \tmp, x18, \tmp - str \tmp, [\tsk, #TSK_TI_SCS_OFFSET] + str x18, [\tsk, #TSK_TI_SCS_SP] .endm #else .macro scs_load tsk, tmp
arch/arm64/include/asm/thread_info.h +2 -2
@@ -43,7 +43,7 @@ struct thread_info { };#ifdef CONFIG_SHADOW_CALL_STACK void *scs_base; -unsigned long scs_offset; +void *scs_sp;#endif };
@@ -107,7 +107,7 @@ void arch_release_task_struct(struct task_struct *tsk);#ifdef CONFIG_SHADOW_CALL_STACK #define INIT_SCS \ .scs_base = init_shadow_call_stack, \ - .scs_offset = 0, + .scs_sp = init_shadow_call_stack,#else #define INIT_SCS #endif
arch/arm64/kernel/asm-offsets.c +1 -1
@@ -36,7 +36,7 @@ int main(void)#endif #ifdef CONFIG_SHADOW_CALL_STACK DEFINE(TSK_TI_SCS_BASE, offsetof(struct task_struct, thread_info.scs_base)); - DEFINE(TSK_TI_SCS_OFFSET, offsetof(struct task_struct, thread_info.scs_offset)); + DEFINE(TSK_TI_SCS_SP, offsetof(struct task_struct, thread_info.scs_sp));#endif DEFINE (TSK_STACK , offsetof(struct task_struct , stack));#ifdef CONFIG_STACKPROTECTOR
include/linux/scs.h +4 -4
@@ -27,7 +27,7 @@#define SCS_END_MAGIC (0x5f6UL + POISON_POINTER_DELTA)#define task_scs (tsk) (task_thread_info(tsk)->scs_base) -#define task_scs_offset (tsk) (task_thread_info(tsk)->scs_offset) +#define task_scs_sp (tsk) (task_thread_info(tsk)->scs_sp)void scs_init (void );int scs_prepare (struct task_struct *tsk,int node);
@@ -39,7 +39,7 @@ static inline void scs_task_reset(struct task_struct *tsk)* Reset the shadow stack to the base address in case the task * is reused. */ -task_scs_offset (tsk) =0 ; +task_scs_sp (tsk) =task_scs (tsk); }static inline unsigned long *__scs_magic (void *s)
@@ -50,9 +50,9 @@ static inline unsigned long *__scs_magic(void *s)static inline bool scs_corrupted (struct task_struct *tsk) {unsigned long *magic =__scs_magic (task_scs (tsk)); +unsigned long sz =task_scs_sp (tsk) -task_scs (tsk); -return (task_scs_offset (tsk) >=SCS_SIZE -1 || -READ_ONCE_NOCHECK (*magic) !=SCS_END_MAGIC ); +return sz >=SCS_SIZE -1 ||READ_ONCE_NOCHECK (*magic) !=SCS_END_MAGIC ; }#else /* CONFIG_SHADOW_CALL_STACK */
kernel/scs.c +1 -2
@@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)if (!s)return -ENOMEM ; -task_scs (tsk) = s; -task_scs_offset (tsk) =0 ; +task_scs (tsk) =task_scs_sp (tsk) = s;scs_account (tsk,1 );return 0 ; }
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info
Quoted 9 lines · Will Deacon
Storing the SCS information in
thread_infoas a {base,offset} pair introduces an additional load instruction on the ret-to-user path, since the SCS stack pointer in x18 has to be converted back to an offset by subtracting the base.Replace the offset with the absolute SCS stack pointer value instead and avoid the redundant load.
Signed-off-by Will Deacon <[email protected]>
One trivial nit below, but regardless this looks sound to me, and I certainly prefer having the absolute address rather than an offset, so:
kernel/scs.c int scs_prepare(struct task_struct *tsk, int node) · line 63
Quoted 11 lines
kernel/scs.c +1
-2@@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)if (!s)return -ENOMEM ; -task_scs (tsk) = s; -task_scs_offset (tsk) =0 ; +task_scs (tsk) =task_scs_sp (tsk) = s;
I think this would be more legible as two statements:
| task_sys(tsk) = s;
| task_scs_sp(tsk) = s;
... as we usually save foo = bar = baz stuff for the start of a function or within loop conditions.
Thanks,
Mark.
Quoted 6 lines
kernel/scs.c +0
-0scs_account (tsk,1 );return 0 ; }Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Will Deacon
maintainerRe: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info
Quoted 24 lines · Mark Rutland
Quoted 9 lines · Will Deacon
Storing the SCS information in
thread_infoas a {base,offset} pair introduces an additional load instruction on the ret-to-user path, since the SCS stack pointer in x18 has to be converted back to an offset by subtracting the base.Replace the offset with the absolute SCS stack pointer value instead and avoid the redundant load.
Signed-off-by Will Deacon <[email protected]>One trivial nit below, but regardless this looks sound to me, and I certainly prefer having the absolute address rather than an offset, so:
Reviewed-by Mark Rutland <[email protected]>Quoted 8 lines
kernel/scs.c +0
-0@@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)if (!s)return -ENOMEM ;
Quoted 8 lines
kernel/scs.c int scs_prepare(struct task_struct *tsk, int node) · line 63
Quoted 3 lines
kernel/scs.c +1
-2-task_scs (tsk) = s; -task_scs_offset (tsk) =0 ; +task_scs (tsk) =task_scs_sp (tsk) = s;I think this would be more legible as two statements:
|
task_sys(tsk)= s;
|task_scs_sp(tsk)= s;
I think it's nice to be able to say:
task_scs(tsk) = task_scs_sp(tsk);
because it makes it very clear that they are initialised to the same thing. Having it as two statements means somebody will update one and forget to update the other one.
Quoted 2 lines
... as we usually save
foo = bar = bazstuff for the start of a function or within loop conditions.
Hmm, I can't really find anything consistent in that regard, to be honest with you. Did I miss something in the coding style doc?
I'll leave it as-is for now.
Will
Will Deacon
maintainerPATCH 2/6 scs: Move accounting into alloc/free functions
In reply to Will DeaconThere's no need to perform the shadow stack page accounting independently of the lifetime of the underlying allocation, so call the accounting code from the {alloc,free}() functions and simplify the code in the process.
--- kernel/scs.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-)
kernel/scs.c +21 -24
@@ -14,25 +14,35 @@static struct kmem_cache *scs_cache; +static void __scs_account (void *s,int account) +{ +struct page *scs_page =virt_to_page (s); + +mod_zone_page_state (page_zone (scs_page),NR_KERNEL_SCS_KB , + account * (SCS_SIZE /SZ_1K )); +} +static void *scs_alloc (int node) { -void *s; - - s =kmem_cache_alloc_node (scs_cache,GFP_SCS , node); -if (s) { - *__scs_magic (s) =SCS_END_MAGIC ; -/* -* Poison the allocation to catch unintentional accesses to -* the shadow stack when KASAN is enabled. -*/ -kasan_poison_object_data (scs_cache, s); - } +void *s =kmem_cache_alloc_node (scs_cache,GFP_SCS , node); + +if (!s) +return NULL ; + *__scs_magic (s) =SCS_END_MAGIC ; + +/* +* Poison the allocation to catch unintentional accesses to +* the shadow stack when KASAN is enabled. +*/ +kasan_poison_object_data (scs_cache, s); +__scs_account (s,1 );return s; }static void scs_free (void *s) { +__scs_account (s,-1 );kasan_unpoison_object_data (scs_cache, s);kmem_cache_free (scs_cache, s); }
@@ -42,17 +52,6 @@ void __init scs_init(void) scs_cache =kmem_cache_create ("scs_cache" ,SCS_SIZE ,0 ,0 ,NULL ); } -static struct page *__scs_page (struct task_struct *tsk) -{ -return virt_to_page (task_scs (tsk)); -} - -static void scs_account (struct task_struct *tsk,int account) -{ -mod_zone_page_state (page_zone (__scs_page (tsk)),NR_KERNEL_SCS_KB , - account * (SCS_SIZE /1024 )); -} -int scs_prepare (struct task_struct *tsk,int node) {void *s =scs_alloc (node);
@@ -61,7 +60,6 @@ int scs_prepare(struct task_struct *tsk, int node)return -ENOMEM ;task_scs (tsk) =task_scs_sp (tsk) = s; -scs_account (tsk,1 );return 0 ; }
@@ -102,6 +100,5 @@ void scs_release(struct task_struct *tsk)WARN (scs_corrupted (tsk),"corrupted shadow stack detected when freeing task\n" );scs_check_usage (tsk); -scs_account (tsk,-1 );scs_free (s); }
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Re: [PATCH 2/6] scs: Move accounting into alloc/free functions
Quoted 8 lines · Will Deacon
There's no need to perform the shadow stack page accounting independently of the lifetime of the underlying allocation, so call the accounting code from the {alloc,free}() functions and simplify the code in the process.
Signed-off-by Will Deacon <[email protected]>--- kernel/scs.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-)
One (super trivial) nit below, but regardless this looks like a sound and sensible cleanup, so:
kernel/scs.c line 21
Quoted 13 lines
kernel/scs.c +4
-0@@ -14,25 +14,35 @@static void *scs_alloc (int node) { +void *s =kmem_cache_alloc_node (scs_cache,GFP_SCS , node); + +if (!s) +return NULL ;
Super trivial nit, but could we omit the line space between these two, to fit with usual style?
Mark.
Will Deacon
maintainerRe: [PATCH 2/6] scs: Move accounting into alloc/free functions
Quoted 24 lines · Mark Rutland
Quoted 8 lines · Will Deacon
There's no need to perform the shadow stack page accounting independently of the lifetime of the underlying allocation, so call the accounting code from the {alloc,free}() functions and simplify the code in the process.
Signed-off-by Will Deacon <[email protected]>--- kernel/scs.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-)One (super trivial) nit below, but regardless this looks like a sound and sensible cleanup, so:
Reviewed-by Mark Rutland <[email protected]>Quoted 8 lines
kernel/scs.c +0
-0@@ -14,25 +14,35 @@static void *scs_alloc (int node) {
Quoted 7 lines
kernel/scs.c line 30
Quoted 4 lines
kernel/scs.c +4
-0+void *s =kmem_cache_alloc_node (scs_cache,GFP_SCS , node); + +if (!s) +return NULL ;Super trivial nit, but could we omit the line space between these two, to fit with usual style?
I really like having the empty line after the last variable declaration.
Sorry,
Will
Will Deacon
maintainerPATCH 3/6 arm64: scs: Use 'scs_sp' register alias for x18
In reply to Will Deaconx18 holds the SCS stack pointer value, so introduce a register alias to make this easier to read in assembly code.
--- arch/arm64/include/asm/scs.h | 6 ++++-- arch/arm64/kernel/entry.S | 10 +++++----- arch/arm64/kernel/head.S | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-)
arch/arm64/include/asm/scs.h +4 -2
@@ -7,12 +7,14 @@#include <asm/asm-offsets.h> #ifdef CONFIG_SHADOW_CALL_STACK + scs_sp .req x18 + .macro scs_load tsk, tmp - ldr x18, [\tsk, #TSK_TI_SCS_SP] + ldr scs_sp, [\tsk, #TSK_TI_SCS_SP] .endm .macro scs_save tsk, tmp - str x18, [\tsk, #TSK_TI_SCS_SP] + str scs_sp, [\tsk, #TSK_TI_SCS_SP] .endm #else .macro scs_load tsk, tmp
arch/arm64/kernel/entry.S +5 -5
@@ -394,7 +394,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0 .macro irq_stack_entry mov x19, sp // preserve the original sp #ifdef CONFIG_SHADOW_CALL_STACK - mov x24, x18 // preserve the original shadow stack + mov x24, scs_sp // preserve the original shadow stack #endif /*
@@ -416,7 +416,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0 #ifdef CONFIG_SHADOW_CALL_STACK /* also switch to the irq shadow stack */ - adr_this_cpu x18, irq_shadow_call_stack, x26 + adr_this_cpu scs_sp, irq_shadow_call_stack, x26 #endif 9998:
@@ -430,7 +430,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0 .macro irq_stack_exit mov sp, x19 #ifdef CONFIG_SHADOW_CALL_STACK - mov x18, x24 + mov scs_sp, x24 #endif .endm
@@ -1071,9 +1071,9 @@ SYM_CODE_START(__sdei_asm_handler) #ifdef CONFIG_SHADOW_CALL_STACK /* Use a separate shadow call stack for normal and critical events */ cbnz w4, 3f - adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_normal, tmp=x6 + adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_normal, tmp=x6 b 4f -3: adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_critical, tmp=x6 +3: adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_critical, tmp=x6 4: #endif
arch/arm64/kernel/head.S +1 -1
@@ -426,7 +426,7 @@ SYM_FUNC_START_LOCAL(__primary_switched) mov x29, sp #ifdef CONFIG_SHADOW_CALL_STACK - adr_l x18, init_shadow_call_stack // Set shadow call stack + adr_l scs_sp, init_shadow_call_stack // Set shadow call stack #endif str_l x21, __fdt_pointer, x5 // Save FDT pointer
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18
Quoted 4 lines · Will Deacon
x18 holds the SCS stack pointer value, so introduce a register alias to make this easier to read in assembly code.
Signed-off-by Will Deacon <[email protected]>
I scanned through arm64 for all instances of x18, and it looks like you've covered all the relevant uses here. In kvm we save/restore x18 a bunch becasue it might be a platform register, but we do that unconditionally and without knowledge of what it contains, so I think that's fine to leave as-is. Therefore:
As an aside, the comment in entry-ftrace.S is now stale where it says that x18 is safe to clobber. I can send a patch to clean that up, unless you want to do that yourself.
Mark.
Quoted 86 lines
--- arch/arm64/include/asm/scs.h | 6 ++++-- arch/arm64/kernel/entry.S | 10 +++++----- arch/arm64/kernel/head.S | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-)arch/arm64/include/asm/scs.h +4
-2@@ -7,12 +7,14 @@#include <asm/asm-offsets.h> #ifdef CONFIG_SHADOW_CALL_STACK + scs_sp .req x18 + .macro scs_load tsk, tmp - ldr x18, [\tsk, #TSK_TI_SCS_SP] + ldr scs_sp, [\tsk, #TSK_TI_SCS_SP] .endm .macro scs_save tsk, tmp - str x18, [\tsk, #TSK_TI_SCS_SP] + str scs_sp, [\tsk, #TSK_TI_SCS_SP] .endm #else .macro scs_load tsk, tmparch/arm64/kernel/entry.S +5
-5@@ -394,7 +394,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0 .macro irq_stack_entry mov x19, sp // preserve the original sp #ifdef CONFIG_SHADOW_CALL_STACK - mov x24, x18 // preserve the original shadow stack + mov x24, scs_sp // preserve the original shadow stack #endif /*@@ -416,7 +416,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0 #ifdef CONFIG_SHADOW_CALL_STACK /* also switch to the irq shadow stack */ - adr_this_cpu x18, irq_shadow_call_stack, x26 + adr_this_cpu scs_sp, irq_shadow_call_stack, x26 #endif 9998:@@ -430,7 +430,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0 .macro irq_stack_exit mov sp, x19 #ifdef CONFIG_SHADOW_CALL_STACK - mov x18, x24 + mov scs_sp, x24 #endif .endm@@ -1071,9 +1071,9 @@ SYM_CODE_START(__sdei_asm_handler) #ifdef CONFIG_SHADOW_CALL_STACK /* Use a separate shadow call stack for normal and critical events */ cbnz w4, 3f - adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_normal, tmp=x6 + adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_normal, tmp=x6 b 4f -3: adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_critical, tmp=x6 +3: adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_critical, tmp=x6 4: #endifarch/arm64/kernel/head.S +1
-1@@ -426,7 +426,7 @@ SYM_FUNC_START_LOCAL(__primary_switched) mov x29, sp #ifdef CONFIG_SHADOW_CALL_STACK - adr_l x18, init_shadow_call_stack // Set shadow call stack + adr_l scs_sp, init_shadow_call_stack // Set shadow call stack #endif str_l x21, __fdt_pointer, x5 // Save FDT pointerSignature · 1 line
2.26.2.761.g0e0b3e54be-goog
Will Deacon
maintainerRe: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18
Quoted 12 lines · Mark Rutland
Quoted 4 lines · Will Deacon
x18 holds the SCS stack pointer value, so introduce a register alias to make this easier to read in assembly code.
Signed-off-by Will Deacon <[email protected]>I scanned through arm64 for all instances of x18, and it looks like you've covered all the relevant uses here. In kvm we save/restore x18 a bunch becasue it might be a platform register, but we do that unconditionally and without knowledge of what it contains, so I think that's fine to leave as-is. Therefore:
Quoted 5 lines
Reviewed-by Mark Rutland <[email protected]>As an aside, the comment in
entry-ftrace.Sis now stale where it says that x18 is safe to clobber. I can send a patch to clean that up, unless you want to do that yourself.
Thanks, I'll fix that up (see below). Also, apologies for typo'ing your email address when I sent this out on Friday.
Will
--->8
From 7e86208cd6541c1229bc1fcd206596308d1727f8 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Mon, 18 May 2020 14:01:01 +0100
Subject: [PATCH] arm64: entry-ftrace.S: Update comment to indicate that x18 is
live
The Shadow Call Stack pointer is held in x18, so update the ftrace entry comment to indicate that it cannot be safely clobbered.
--- arch/arm64/kernel/entry-ftrace.S | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm64/kernel/entry-ftrace.S +3 -2
@@ -23,8 +23,9 @@ * * ... where <entry> is either ftrace_caller or ftrace_regs_caller. * - * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are - * live, and x9-x18 are safe to clobber. + * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are + * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to + * clobber. * * We save the callsite's context into a pt_regs before invoking any ftrace * callbacks. So that we can get a sensible backtrace, we create a stack record
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Mark Rutland
reviewerRe: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18
Quoted 13 lines · Will Deacon
Quoted 12 lines · Mark Rutland
Quoted 4 lines · Will Deacon
x18 holds the SCS stack pointer value, so introduce a register alias to make this easier to read in assembly code.
Signed-off-by Will Deacon <[email protected]>I scanned through arm64 for all instances of x18, and it looks like you've covered all the relevant uses here. In kvm we save/restore x18 a bunch becasue it might be a platform register, but we do that unconditionally and without knowledge of what it contains, so I think that's fine to leave as-is. Therefore:
Quoted 8 lines
Quoted 5 lines
Reviewed-by Mark Rutland <[email protected]>As an aside, the comment in
entry-ftrace.Sis now stale where it says that x18 is safe to clobber. I can send a patch to clean that up, unless you want to do that yourself.Thanks, I'll fix that up (see below). Also, apologies for typo'ing your email address when I sent this out on Friday.
No worries; I'd already forgotten!
Quoted 25 lines
Will
--->8
From
7e86208cd6541c1229bc1fcd206596308d1727f8Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Mon, 18 May 2020 14:01:01 +0100
Subject: [PATCH] arm64:entry-ftrace.S: Update comment to indicate that x18 is
liveThe Shadow Call Stack pointer is held in x18, so update the ftrace entry comment to indicate that it cannot be safely clobbered.
Reported-by Mark Rutland <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/entry-ftrace.S | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/kernel/entry-ftrace.S +0
-0
arch/arm64/kernel/entry-ftrace.S line 28
Quoted 8 lines
arch/arm64/kernel/entry-ftrace.S +3
-2@@ -23,8 +23,9 @@ * * ... where <entry> is either ftrace_caller or ftrace_regs_caller. * - * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are - * live, and x9-x18 are safe to clobber. + * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are + * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to + * clobber.
I'd have called out x18 a bit more specifically:
| Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are | live, x18 is potentially live with a shadow call stack pointer, and | x9-x17 are safe to clobber.
... but either way this looks good; thanks!
Mark.
Will Deacon
maintainerPATCH 4/6 scs: Move scs_overflow_check() out of architecture code
In reply to Will DeaconThere is nothing architecture-specific about scs_overflow_check() as it's just a trivial wrapper around scs_corrupted().
For parity with task_stack_end_corrupted(), rename scs_corrupted() to task_scs_end_corrupted() and call it from schedule_debug() when CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused scs_overflow_check() function entirely.
This has absolutely no impact on architectures that do not support SCS (currently arm64 only).
--- arch/arm64/include/asm/scs.h | 18 ------------------ arch/arm64/kernel/process.c | 2 -- arch/arm64/kernel/scs.c | 2 +- include/linux/scs.h | 4 ++-- kernel/sched/core.c | 3 +++ kernel/scs.c | 3 ++- 6 files changed, 8 insertions(+), 24 deletions(-)
arch/arm64/include/asm/scs.h +0 -18
@@ -24,24 +24,6 @@ .endm#endif /* CONFIG_SHADOW_CALL_STACK */ -#else /* __ASSEMBLY__ */ - -#include <linux/scs.h> - -#ifdef CONFIG_SHADOW_CALL_STACK - -static inline void scs_overflow_check (struct task_struct *tsk) -{ -if (unlikely (scs_corrupted (tsk))) -panic ("corrupted shadow stack detected inside scheduler\n" ); -} - -#else /* CONFIG_SHADOW_CALL_STACK */ - -static inline void scs_overflow_check (struct task_struct *tsk) {} - -#endif /* CONFIG_SHADOW_CALL_STACK */ -#endif /* __ASSEMBLY __ */ #endif /* _ASM_SCS_H */
arch/arm64/kernel/process.c +0 -2
@@ -52,7 +52,6 @@#include <asm/mmu_context.h> #include <asm/processor.h> #include <asm/pointer_auth.h> -#include <asm/scs.h> #include <asm/stacktrace.h> #if defined(CONFIG_STACKPROTECTOR ) && !defined(CONFIG_STACKPROTECTOR_PER_TASK )
@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,entry_task_switch (next);uao_thread_switch (next);ssbs_thread_switch (next); -scs_overflow_check (next);/* * Complete any pending TLB or cache maintenance on this CPU in case
arch/arm64/kernel/scs.c +1 -1
@@ -6,7 +6,7 @@*/ #include <linux/percpu.h> -#include <asm/scs.h> +#include <linux/scs.h> /* Allocate a static per-CPU shadow stack */ #define DEFINE_SCS (name) \
include/linux/scs.h +2 -2
@@ -47,7 +47,7 @@ static inline unsigned long *__scs_magic(void *s)return (unsigned long *)(s +SCS_SIZE ) -1 ; } -static inline bool scs_corrupted (struct task_struct *tsk) +static inline bool task_scs_end_corrupted (struct task_struct *tsk) {unsigned long *magic =__scs_magic (task_scs (tsk));unsigned long sz =task_scs_sp (tsk) -task_scs (tsk);
@@ -60,8 +60,8 @@ static inline bool scs_corrupted(struct task_struct *tsk)static inline void scs_init (void ) {}static inline void scs_task_reset (struct task_struct *tsk) {}static inline int scs_prepare (struct task_struct *tsk,int node) {return 0 ; } -static inline bool scs_corrupted (struct task_struct *tsk) {return false; }static inline void scs_release (struct task_struct *tsk) {} +static inline bool task_scs_end_corrupted (struct task_struct *tsk) {return false; }#endif /* CONFIG_SHADOW_CALL_STACK */
kernel/sched/core.c +3 -0
@@ -3878,6 +3878,9 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)#ifdef CONFIG_SCHED_STACK_END_CHECK if (task_stack_end_corrupted (prev))panic ("corrupted stack end detected inside scheduler\n" ); + +if (task_scs_end_corrupted (prev)) +panic ("corrupted shadow stack detected inside scheduler\n" );#endif #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
kernel/scs.c +2 -1
@@ -98,7 +98,8 @@ void scs_release(struct task_struct *tsk)if (!s)return ; -WARN (scs_corrupted (tsk),"corrupted shadow stack detected when freeing task\n" ); +WARN (task_scs_end_corrupted (tsk), +"corrupted shadow stack detected when freeing task\n" );scs_check_usage (tsk);scs_free (s); }
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Mark Rutland
reviewerRe: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code
Quoted 8 lines · Will Deacon
There is nothing architecture-specific about
scs_overflow_check()as it's just a trivial wrapper aroundscs_corrupted().For parity with
task_stack_end_corrupted(), renamescs_corrupted()totask_scs_end_corrupted()and call it fromschedule_debug()whenCONFIG_SCHED_STACK_END_CHECK_isenabled. Finally, remove the unusedscs_overflow_check()function entirely.
Quoted 4 lines
This has absolutely no impact on architectures that do not support SCS (currently arm64 only).
Signed-off-by Will Deacon <[email protected]>
Pulling this out of arch code seems sane to me, and the arch-specific chanes look sound. However, I have a concern with the changes within the scheduler context-switch.
Quoted 13 lines
arch/arm64/kernel/process.c +0
-1@@ -52,7 +52,6 @@#include <asm/mmu_context.h> #include <asm/processor.h> #include <asm/pointer_auth.h> -#include <asm/scs.h> #include <asm/stacktrace.h> #if defined(CONFIG_STACKPROTECTOR ) && !defined(CONFIG_STACKPROTECTOR_PER_TASK )
arch/arm64/kernel/process.c __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, · line 519
Quoted 4 lines
arch/arm64/kernel/process.c +0
-1@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,entry_task_switch (next);uao_thread_switch (next);ssbs_thread_switch (next); -scs_overflow_check (next);
Prior to this patch, we'd never switch to a task whose SCS had already been corrupted.
With this patch, we only check that when switching away from a task, and only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first glance seems to weaken that.
Arguably:
* If the next task's SCS was corrupted by that task while it was running, we had already lost at that point.
* If the next task's SCS was corrupted by another task, then that could also happen immediately after the check (though timing to avoid the check but affect the process could be harder).
... and a VMAP'd SCS would be much nicer in this regard.
Do we think this is weakening the check, or do we think it wasn't all that helpful to begin with?
Mark.
Quoted 70 lines
arch/arm64/kernel/process.c +0
-0/* * Complete any pending TLB or cache maintenance on this CPU in case arch/arm64/kernel/scs.c +1
-1@@ -6,7 +6,7 @@*/ #include <linux/percpu.h> -#include <asm/scs.h> +#include <linux/scs.h> /* Allocate a static per-CPU shadow stack */ #define DEFINE_SCS (name) \include/linux/scs.h +2
-2@@ -47,7 +47,7 @@ static inline unsigned long *__scs_magic(void *s)return (unsigned long *)(s +SCS_SIZE ) -1 ; } -static inline bool scs_corrupted (struct task_struct *tsk) +static inline bool task_scs_end_corrupted (struct task_struct *tsk) {unsigned long *magic =__scs_magic (task_scs (tsk));unsigned long sz =task_scs_sp (tsk) -task_scs (tsk);@@ -60,8 +60,8 @@ static inline bool scs_corrupted(struct task_struct *tsk)static inline void scs_init (void ) {}static inline void scs_task_reset (struct task_struct *tsk) {}static inline int scs_prepare (struct task_struct *tsk,int node) {return 0 ; } -static inline bool scs_corrupted (struct task_struct *tsk) {return false; }static inline void scs_release (struct task_struct *tsk) {} +static inline bool task_scs_end_corrupted (struct task_struct *tsk) {return false; }#endif /* CONFIG_SHADOW_CALL_STACK */ kernel/sched/core.c +3
-0@@ -3878,6 +3878,9 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)#ifdef CONFIG_SCHED_STACK_END_CHECK if (task_stack_end_corrupted (prev))panic ("corrupted stack end detected inside scheduler\n" ); + +if (task_scs_end_corrupted (prev)) +panic ("corrupted shadow stack detected inside scheduler\n" );#endif #ifdef CONFIG_DEBUG_ATOMIC_SLEEP kernel/scs.c +2
-1@@ -98,7 +98,8 @@ void scs_release(struct task_struct *tsk)if (!s)return ; -WARN (scs_corrupted (tsk),"corrupted shadow stack detected when freeing task\n" ); +WARN (task_scs_end_corrupted (tsk), +"corrupted shadow stack detected when freeing task\n" );scs_check_usage (tsk);scs_free (s); }Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Will Deacon
maintainerRe: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code
Quoted 39 lines · Mark Rutland
Quoted 8 lines · Will Deacon
There is nothing architecture-specific about
scs_overflow_check()as it's just a trivial wrapper aroundscs_corrupted().For parity with
task_stack_end_corrupted(), renamescs_corrupted()totask_scs_end_corrupted()and call it fromschedule_debug()whenCONFIG_SCHED_STACK_END_CHECK_isenabled. Finally, remove the unusedscs_overflow_check()function entirely.Quoted 4 lines
This has absolutely no impact on architectures that do not support SCS (currently arm64 only).
Signed-off-by Will Deacon <[email protected]>Pulling this out of arch code seems sane to me, and the arch-specific chanes look sound. However, I have a concern with the changes within the scheduler context-switch.
Quoted 13 lines
arch/arm64/kernel/process.c +0
-1@@ -52,7 +52,6 @@#include <asm/mmu_context.h> #include <asm/processor.h> #include <asm/pointer_auth.h> -#include <asm/scs.h> #include <asm/stacktrace.h> #if defined(CONFIG_STACKPROTECTOR ) && !defined(CONFIG_STACKPROTECTOR_PER_TASK )arch/arm64/kernel/process.c __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, · line 519
Quoted 4 lines
arch/arm64/kernel/process.c +0
-1@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,entry_task_switch (next);uao_thread_switch (next);ssbs_thread_switch (next); -scs_overflow_check (next);Prior to this patch, we'd never switch to a task whose SCS had already been corrupted.
Quoted 3 lines
With this patch, we only check that when switching away from a task, and only when
CONFIG_SCHED_STACK_END_CHECKis selected, which at first glance seems to weaken that.
Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.
Quoted 4 lines
Arguably:
* If the next task's SCS was corrupted by that task while it was running, we had already lost at that point.
With this change, we'll at least catch this one sooner, and that might be useful if a bug has caused us to overflow the SCS but not the main stack.
Quoted 3 lines
* If the next task's SCS was corrupted by another task, then that could also happen immediately after the check (though timing to avoid the check but affect the process could be harder).
We're only checking the magic end value, so the cross-task case is basically if you overrun your own SCS as above, but then continue to overrun entire SCSs for other tasks as well. It's probably not very useful in that case.
Quoted 4 lines
... and a VMAP'd SCS would be much nicer in this regard.
Do we think this is weakening the check, or do we think it wasn't all that helpful to begin with?
I see it as a debug check to catch SCS overflow, rather than a hardening feature, and I agree that using something like vmap stack for the SCS would be better because we could have a guard page instead. This is something I would like to revisit, but we need more information from Sami about why Android rejected the larger allocation size, since I don't think there's an awful lot of point merging this series if Android doesn't pick it up.
Will
Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code
Quoted 47 lines · Will Deacon
Quoted 39 lines · Mark Rutland
Quoted 8 lines · Will Deacon
There is nothing architecture-specific about
scs_overflow_check()as it's just a trivial wrapper aroundscs_corrupted().For parity with
task_stack_end_corrupted(), renamescs_corrupted()totask_scs_end_corrupted()and call it fromschedule_debug()whenCONFIG_SCHED_STACK_END_CHECK_isenabled. Finally, remove the unusedscs_overflow_check()function entirely.Quoted 4 lines
This has absolutely no impact on architectures that do not support SCS (currently arm64 only).
Signed-off-by Will Deacon <[email protected]>Pulling this out of arch code seems sane to me, and the arch-specific chanes look sound. However, I have a concern with the changes within the scheduler context-switch.
Quoted 13 lines
arch/arm64/kernel/process.c +0
-1@@ -52,7 +52,6 @@#include <asm/mmu_context.h> #include <asm/processor.h> #include <asm/pointer_auth.h> -#include <asm/scs.h> #include <asm/stacktrace.h> #if defined(CONFIG_STACKPROTECTOR ) && !defined(CONFIG_STACKPROTECTOR_PER_TASK )arch/arm64/kernel/process.c __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, · line 519
Quoted 4 lines
arch/arm64/kernel/process.c +0
-1@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,entry_task_switch (next);uao_thread_switch (next);ssbs_thread_switch (next); -scs_overflow_check (next);Prior to this patch, we'd never switch to a task whose SCS had already been corrupted.
Quoted 3 lines
With this patch, we only check that when switching away from a task, and only when
CONFIG_SCHED_STACK_END_CHECKis selected, which at first glance seems to weaken that.Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with the main stack checking when
CONFIG_SCHED_STACK_END_CHECK=y.
Quoted 7 lines
Quoted 4 lines
Arguably:
* If the next task's SCS was corrupted by that task while it was running, we had already lost at that point.
With this change, we'll at least catch this one sooner, and that might be useful if a bug has caused us to overflow the SCS but not the main stack.
Sure, but only if CONFIG_SCHED_STACK_END_CHECK is selected.
Quoted 8 lines
Quoted 3 lines
* If the next task's SCS was corrupted by another task, then that could also happen immediately after the check (though timing to avoid the check but affect the process could be harder).
We're only checking the magic end value, so the cross-task case is basically if you overrun your own SCS as above, but then continue to overrun entire SCSs for other tasks as well. It's probably not very useful in that case.
Quoted 8 lines
Quoted 4 lines
... and a VMAP'd SCS would be much nicer in this regard.
Do we think this is weakening the check, or do we think it wasn't all that helpful to begin with?
I see it as a debug check to catch SCS overflow, rather than a hardening feature, and I agree that using something like vmap stack for the SCS would be better because we could have a guard page instead.
Fair enough. Could we put something into the commit message that more explicitly calls out debug-not-hardening? I agree that under that model this patch looks fine, and with something to that effect:
Mark.
Quoted 4 lines
This is something I would like to revisit, but we need more information from Sami about why Android rejected the larger allocation size, since I don't think there's an awful lot of point merging this series if Android doesn't pick it up.
Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do that.
Mark.
Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code
Quoted 83 lines · Mark Rutland
Quoted 47 lines · Will Deacon
Quoted 39 lines · Mark Rutland
Quoted 8 lines · Will Deacon
There is nothing architecture-specific about
scs_overflow_check()as it's just a trivial wrapper aroundscs_corrupted().For parity with
task_stack_end_corrupted(), renamescs_corrupted()totask_scs_end_corrupted()and call it fromschedule_debug()whenCONFIG_SCHED_STACK_END_CHECK_isenabled. Finally, remove the unusedscs_overflow_check()function entirely.Quoted 4 lines
This has absolutely no impact on architectures that do not support SCS (currently arm64 only).
Signed-off-by Will Deacon <[email protected]>Pulling this out of arch code seems sane to me, and the arch-specific chanes look sound. However, I have a concern with the changes within the scheduler context-switch.
Quoted 13 lines
arch/arm64/kernel/process.c +0
-1@@ -52,7 +52,6 @@#include <asm/mmu_context.h> #include <asm/processor.h> #include <asm/pointer_auth.h> -#include <asm/scs.h> #include <asm/stacktrace.h> #if defined(CONFIG_STACKPROTECTOR ) && !defined(CONFIG_STACKPROTECTOR_PER_TASK )arch/arm64/kernel/process.c __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, · line 519
Quoted 4 lines
arch/arm64/kernel/process.c +0
-1@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,entry_task_switch (next);uao_thread_switch (next);ssbs_thread_switch (next); -scs_overflow_check (next);Prior to this patch, we'd never switch to a task whose SCS had already been corrupted.
Quoted 3 lines
With this patch, we only check that when switching away from a task, and only when
CONFIG_SCHED_STACK_END_CHECKis selected, which at first glance seems to weaken that.Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with the main stack checking when
CONFIG_SCHED_STACK_END_CHECK=y.Quoted 7 lines
Quoted 4 lines
Arguably:
* If the next task's SCS was corrupted by that task while it was running, we had already lost at that point.
With this change, we'll at least catch this one sooner, and that might be useful if a bug has caused us to overflow the SCS but not the main stack.
Sure, but only if
CONFIG_SCHED_STACK_END_CHECKis selected.Quoted 8 lines
Quoted 3 lines
* If the next task's SCS was corrupted by another task, then that could also happen immediately after the check (though timing to avoid the check but affect the process could be harder).
We're only checking the magic end value, so the cross-task case is basically if you overrun your own SCS as above, but then continue to overrun entire SCSs for other tasks as well. It's probably not very useful in that case.
Quoted 8 lines
Quoted 4 lines
... and a VMAP'd SCS would be much nicer in this regard.
Do we think this is weakening the check, or do we think it wasn't all that helpful to begin with?
I see it as a debug check to catch SCS overflow, rather than a hardening feature, and I agree that using something like vmap stack for the SCS would be better because we could have a guard page instead.
Fair enough. Could we put something into the commit message that more explicitly calls out debug-not-hardening? I agree that under that model this patch looks fine, and with something to that effect:
Reviewed-by Mark Rutland <[email protected]>Mark.
Quoted 7 lines
Quoted 4 lines
This is something I would like to revisit, but we need more information from Sami about why Android rejected the larger allocation size, since I don't think there's an awful lot of point merging this series if Android doesn't pick it up.
Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do that.
For smaller devices, the memory overhead was too high. (i.e. 4x more memory allocated to kernel stacks -- 4k vs 1k per thread.) The series is much more than just a stack exhaustion defense, so I don't think that detail needs to block the entire series. FWIW, I'd like to have both modes (contiguous and vmap) available so that system builders can choose their trade-off. Both will gain return address corruption defense, but the vmap case will protect against neighboring SCS corruption in the face of very-unlikely-but-technically-possible stack exhaustion (remember that with the elimination of VLAs, the stack depth compile time checking, and the regular stack VMAP guard page, it will be quite difficult to exhaust the SCS -- either because there is no code path to accomplish it, or because it would trip the regular stack guard page first).
Signature · 1 line
Kees Cook
Will Deacon
maintainerRe: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code
Quoted 7 lines · Kees Cook
Quoted 6 lines · Mark Rutland
Quoted 4 lines · Will Deacon
This is something I would like to revisit, but we need more information from Sami about why Android rejected the larger allocation size, since I don't think there's an awful lot of point merging this series if Android doesn't pick it up.
Quoted 5 lines
Quoted 2 lines
Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do that.
For smaller devices, the memory overhead was too high. (i.e. 4x more memory allocated to kernel stacks -- 4k vs 1k per thread.)
I just don't see an extra 3k per thread as being a real issue (the main stack is 16k already). Even just the CPU register state is around 1k.
But I'd be very keen to see numbers/performance data that proves me wrong.
Will
Will Deacon
maintainerPATCH 5/6 scs: Remove references to asm/scs.h from core code
In reply to Will Deaconasm/scs.h is no longer needed by the core code, so remove a redundant header inclusion and update the stale Kconfig text.
--- arch/Kconfig | 4 ++-- kernel/scs.c | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)
arch/Kconfig +2 -2
@@ -537,8 +537,8 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK bool help An architecture should select this if it supports Clang's Shadow - Call Stack, has asm/scs.h, and implements runtime support for shadow - stack switching. + Call Stack and implements runtime support for shadow stack + switching. config SHADOW_CALL_STACK bool "Clang Shadow Call Stack"
kernel/scs.c +0 -1
@@ -10,7 +10,6 @@#include <linux/scs.h> #include <linux/slab.h> #include <linux/vmstat.h> -#include <asm/scs.h> static struct kmem_cache *scs_cache;
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Re: [PATCH 5/6] scs: Remove references to asm/scs.h from core code
Quoted 4 lines · Will Deacon
asm/scs.his no longer needed by the core code, so remove a redundant header inclusion and update the staleKconfigtext.Signed-off-by Will Deacon <[email protected]>
With the corruption checks moved out of arch code this looks sound to me, so modulo my comments on the prior patch, assuming we factor that out:
Mark.
Quoted 35 lines
--- arch/Kconfig | 4 ++-- kernel/scs.c | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)arch/Kconfig +2
-2@@ -537,8 +537,8 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK bool help An architecture should select this if it supports Clang's Shadow - Call Stack, has asm/scs.h, and implements runtime support for shadow - stack switching. + Call Stack and implements runtime support for shadow stack + switching. config SHADOW_CALL_STACK bool "Clang Shadow Call Stack"kernel/scs.c +0
-1@@ -10,7 +10,6 @@#include <linux/scs.h> #include <linux/slab.h> #include <linux/vmstat.h> -#include <asm/scs.h> static struct kmem_cache *scs_cache;Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Will Deacon
maintainerPATCH 6/6 scs: Move DEFINE_SCS macro into core code
In reply to Will DeaconDefining static shadow call stacks is not architecture-specific, so move the DEFINE_SCS() macro into the core header file.
--- arch/arm64/kernel/scs.c | 4 ---- include/linux/scs.h | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm64/kernel/scs.c +0 -4
@@ -8,10 +8,6 @@#include <linux/percpu.h> #include <linux/scs.h> -/* Allocate a static per-CPU shadow stack */ -#define DEFINE_SCS (name) \ - DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name) \ -DEFINE_SCS (irq_shadow_call_stack);#ifdef CONFIG_ARM_SDE_INTERFACE
include/linux/scs.h +4 -0
@@ -26,6 +26,10 @@/* An illegal pointer value to mark the end of the shadow stack. */ #define SCS_END_MAGIC (0x5f6UL + POISON_POINTER_DELTA) +/* Allocate a static per-CPU shadow stack */ +#define DEFINE_SCS (name) \ + DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name) \ +#define task_scs (tsk) (task_thread_info(tsk)->scs_base)#define task_scs_sp (tsk) (task_thread_info(tsk)->scs_sp)
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Mark Rutland
reviewerRe: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code
Quoted 4 lines · Will Deacon
Defining static shadow call stacks is not architecture-specific, so move the
DEFINE_SCS()macro into the core header file.Signed-off-by Will Deacon <[email protected]>
I think that we'll have to pull this back into arch code if/when we deal with VMAP'd stacks, so I'm not sure this is worthwhile given the diffstat is balanced.
Mark.
Quoted 38 lines
--- arch/arm64/kernel/scs.c | 4 ---- include/linux/scs.h | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-)arch/arm64/kernel/scs.c +0
-4@@ -8,10 +8,6 @@#include <linux/percpu.h> #include <linux/scs.h> -/* Allocate a static per-CPU shadow stack */ -#define DEFINE_SCS (name) \ - DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name) \ -DEFINE_SCS (irq_shadow_call_stack);#ifdef CONFIG_ARM_SDE_INTERFACE include/linux/scs.h +4
-0@@ -26,6 +26,10 @@/* An illegal pointer value to mark the end of the shadow stack. */ #define SCS_END_MAGIC (0x5f6UL + POISON_POINTER_DELTA) +/* Allocate a static per-CPU shadow stack */ +#define DEFINE_SCS (name) \ + DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name) \ +#define task_scs (tsk) (task_thread_info(tsk)->scs_base)#define task_scs_sp (tsk) (task_thread_info(tsk)->scs_sp)Signature · 1 line
2.26.2.761.g0e0b3e54be-goog
Will Deacon
maintainerRe: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code
Quoted 9 lines · Mark Rutland
Quoted 4 lines · Will Deacon
Defining static shadow call stacks is not architecture-specific, so move the
DEFINE_SCS()macro into the core header file.Signed-off-by Will Deacon <[email protected]>I think that we'll have to pull this back into arch code if/when we deal with VMAP'd stacks, so I'm not sure this is worthwhile given the diffstat is balanced.
I dunno, if another architecture wants to use this then having the stuff in the core code makes sense to me. I also want to kill asm/scs.h entirely and move our asm macros somewhere else where they're not mixed up with the C headers.
Will
Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code
Quoted 7 lines · Will Deacon
Quoted 6 lines · Mark Rutland
Quoted 4 lines · Will Deacon
Defining static shadow call stacks is not architecture-specific, so move the
DEFINE_SCS()macro into the core header file.Signed-off-by Will Deacon <[email protected]>
Quoted 8 lines
Quoted 3 lines
I think that we'll have to pull this back into arch code if/when we deal with VMAP'd stacks, so I'm not sure this is worthwhile given the diffstat is balanced.
I dunno, if another architecture wants to use this then having the stuff in the core code makes sense to me. I also want to kill
asm/scs.hentirely and move our asm macros somewhere else where they're not mixed up with the C headers.
Thinking about it a bit further, we'd have to make bigger changes anyhow (to dynamically allocate), but given we can do that for regular stacks we can probably do something similar here.
So no strong feelings either way on this patch.
Mark.
Quoted 53 lines · Will Deacon
Hi all,
Here's a series of cleanups I hacked together on top of a modified v13 of the Shadow Call Stack patches from Sami:
https://lore.kernel.org/r/20200515172355.GD23334@willie-the-truck
The main changes are:
* Move code out of
arch/arm64and into the core implementation
* Store the full SCS stack pointer instead of the offset
* Code simplification and general style thingsI'd like to queue this on top early next week so that it can spend some quality time in linux-next.
Cheers,
Will
Cc Sami Tolvanen <[email protected]>Cc Kees Cook <[email protected]>Cc Catalin Marinas <[email protected]>Cc Mark Rutland <[email protected]>Cc Jann Horn <[email protected]>Cc Ard Biesheuvel <[email protected]>Cc Peter Zijlstra <[email protected]>Cc <[email protected]>--->8
Will Deacon (6):
arm64: scs: Store absolute SCS stack pointer value inthread_info
scs: Move accounting into alloc/free functions
arm64: scs: Use 'scs_sp' register alias for x18
scs: Movescs_overflow_check()out of architecture code
scs: Remove references toasm/scs.hfrom core code
scs: MoveDEFINE_SCSmacro into core codearch/Kconfig | 4 +-- arch/arm64/include/asm/scs.h | 29 ++++------------ arch/arm64/include/asm/thread_info.h | 4 +-- arch/arm64/kernel/asm-offsets.c | 2 +- arch/arm64/kernel/entry.S | 10 +++--- arch/arm64/kernel/head.S | 2 +- arch/arm64/kernel/process.c | 2 -- arch/arm64/kernel/scs.c | 6 +--- include/linux/scs.h | 16 +++++---- kernel/sched/core.c | 3 ++ kernel/scs.c | 52 +++++++++++++--------------- 11 files changed, 55 insertions(+), 75 deletions(-)
Quoted 2 lines
--
2.26.2.761.g0e0b3e54be-goog
Thanks, Will. I tested these on my SCS tree and didn't run into any issues. Looks good to me.
Sami
Will Deacon
maintainerQuoted 22 lines · Sami Tolvanen
Quoted 21 lines · Will Deacon
Will Deacon (6):
arm64: scs: Store absolute SCS stack pointer value inthread_info
scs: Move accounting into alloc/free functions
arm64: scs: Use 'scs_sp' register alias for x18
scs: Movescs_overflow_check()out of architecture code
scs: Remove references toasm/scs.hfrom core code
scs: MoveDEFINE_SCSmacro into core codearch/Kconfig | 4 +-- arch/arm64/include/asm/scs.h | 29 ++++------------ arch/arm64/include/asm/thread_info.h | 4 +-- arch/arm64/kernel/asm-offsets.c | 2 +- arch/arm64/kernel/entry.S | 10 +++--- arch/arm64/kernel/head.S | 2 +- arch/arm64/kernel/process.c | 2 -- arch/arm64/kernel/scs.c | 6 +--- include/linux/scs.h | 16 +++++---- kernel/sched/core.c | 3 ++ kernel/scs.c | 52 +++++++++++++--------------- 11 files changed, 55 insertions(+), 75 deletions(-)
Quoted 5 lines
Quoted 2 lines
--
2.26.2.761.g0e0b3e54be-googThanks, Will. I tested these on my SCS tree and didn't run into any issues. Looks good to me.
Cheers, Sami. Can I add your 'Tested-by' to the patches, please?
Will
Quoted 24 lines · Will Deacon
Quoted 22 lines · Sami Tolvanen
Quoted 21 lines · Will Deacon
Will Deacon (6):
arm64: scs: Store absolute SCS stack pointer value inthread_info
scs: Move accounting into alloc/free functions
arm64: scs: Use 'scs_sp' register alias for x18
scs: Movescs_overflow_check()out of architecture code
scs: Remove references toasm/scs.hfrom core code
scs: MoveDEFINE_SCSmacro into core codearch/Kconfig | 4 +-- arch/arm64/include/asm/scs.h | 29 ++++------------ arch/arm64/include/asm/thread_info.h | 4 +-- arch/arm64/kernel/asm-offsets.c | 2 +- arch/arm64/kernel/entry.S | 10 +++--- arch/arm64/kernel/head.S | 2 +- arch/arm64/kernel/process.c | 2 -- arch/arm64/kernel/scs.c | 6 +--- include/linux/scs.h | 16 +++++---- kernel/sched/core.c | 3 ++ kernel/scs.c | 52 +++++++++++++--------------- 11 files changed, 55 insertions(+), 75 deletions(-)
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
--
2.26.2.761.g0e0b3e54be-googThanks, Will. I tested these on my SCS tree and didn't run into any issues. Looks good to me.
Cheers, Sami. Can I add your 'Tested-by' to the patches, please?
Sure, please feel free to add Tested-by tags.
Sami
Will Deacon
maintainerQuoted 28 lines · Sami Tolvanen
Quoted 27 lines · Will Deacon
Quoted 25 lines · Sami Tolvanen
Quoted 23 lines · Will Deacon
Will Deacon (6):
arm64: scs: Store absolute SCS stack pointer value inthread_info
scs: Move accounting into alloc/free functions
arm64: scs: Use 'scs_sp' register alias for x18
scs: Movescs_overflow_check()out of architecture code
scs: Remove references toasm/scs.hfrom core code
scs: MoveDEFINE_SCSmacro into core codearch/Kconfig | 4 +-- arch/arm64/include/asm/scs.h | 29 ++++------------ arch/arm64/include/asm/thread_info.h | 4 +-- arch/arm64/kernel/asm-offsets.c | 2 +- arch/arm64/kernel/entry.S | 10 +++--- arch/arm64/kernel/head.S | 2 +- arch/arm64/kernel/process.c | 2 -- arch/arm64/kernel/scs.c | 6 +--- include/linux/scs.h | 16 +++++---- kernel/sched/core.c | 3 ++ kernel/scs.c | 52 +++++++++++++--------------- 11 files changed, 55 insertions(+), 75 deletions(-)--
2.26.2.761.g0e0b3e54be-goog
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Thanks, Will. I tested these on my SCS tree and didn't run into any issues. Looks good to me.
Cheers, Sami. Can I add your 'Tested-by' to the patches, please?
Sure, please feel free to add Tested-by tags.
Thanks. I've updated the for-next/scs branch with that.
Will